Philip Martin wrote on Thu, 17 Jun 2010 at 12:18 -0000: > Daniel Shahaf <d...@daniel.shahaf.name> writes: > > > Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000: > > > >> Then the repos layer can loop (in practice only if the > >> use_pre_revprop_change_hook flag is set): > >> > >> do > >> svn_fs_revision_prop(¤t_value) > >> action = ... > >> svn_repos__hooks_pre_revprop_change(action) > >> error = svn_fs_change_rev_prop2(current_value, new_value) > >> while error is current value doesn't match > >> > >> This doesn't alter the fact that the revprop can change at any time > >> during the loop but that doesn't matter. The revprop is unversioned > >> so only the current state matters and the above will guarantee that > >> the current state when the change is made is equal to the state > >> validated by the pre-revprop-change hook. > >> > > > > If we just upgrade the svn_fs_change_rev_prop() call in > > svn_repos_fs_change_rev_prop3() to svn_fs_change_rev_prop2() (a > > single-line change), > > Slightly more than one line if we make libsvn_repos absorb the > BAD_PROPERTY and loop. >
Indeed. (In this case, I'm leaning to adding a "retry_count" parameter to the pre- hook, if we'll end up invoking it more than once.) > If we don't loop then if neither of use_pre_revprop_change_hook and > use_post_revprop_change_hook is set then perhaps we should not pass > the old value. That preserves the existing behaviour and the checking > is pointless if there are no hook scripts > Possibly. This should only matter when there are no hooks (i.e., we're being invoked by whom? svnadmin?) and there's a race condition (two simultaneous propchanges). Not a common situation. > > then we will be guaranteed that the 'action' > > parameter (A/D/M) will be accurate. (However, the pre- hook script doesn't > > know the old property value.) > > And getting the old value into the script is tricky. It's binary so > it cannot be passed as a parameter directly. It either needs to be > encoded, or passed via a file descriptor (and not stdin since that is > used for the new value). > As a file descriptor? Or just as a tempfile name? (Any security issues with the latter approach?) > > This is sufficient for the svnsync use case (issue 3546), where "allow > > adds/deletes only" will provide the necessary mutual exclusion. > > > > But I wonder if, while here, we could go further and obtain the > > "expected old property value" from the RA layer (and pass it to the pre- > > hook). (This probably means revving svn_ra_change_rev_prop() the same > > way svn_fs_change_rev_prop() was revved.) That will allow "svn propset > > k v --if-old-value-is=vprime" to work... > > Perhaps. It would require both client and server upgrades and I don't > think there is any way we can provide backward compatibility, so a new > client would have to detect old servers and report some not-supported > error. > Agreed.
