Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000: > Philip Martin <philip.mar...@wandisco.com> writes: > > > Multiple clients. It arises from my idea to solve issue 3546 > > > > http://subversion.tigris.org/issues/show_bug.cgi?id=3546 > > > > In svn_repos_fs_change_rev_prop3 the code first gets the old property > > value which it uses to calculate the action: 'A', 'M' or 'D'. Then it > > passes the action to the pre-revprop-change hook, then it changes the > > property and finally it runs the post-revprop-change hook. Some other > > process can change the revprop at any time so although the > > pre-revprop-change hook might get passed an 'A' say, when the change > > is made it could be effectively an 'M'. The action passed to the hook > > is not a reliable indication of the change to be made. > > > > Putting the get, pre hook and set into a transaction would allow the > > action in the hook to accurately reflect the change made (or not made > > if the transaction fails). > > We don't need a new transaction to fix this, we can rev the > svn_fs_change_rev_prop interface instead: > > svn_error_t * > svn_fs_change_rev_prop(svn_fs_t *fs, > svn_revnum_t rev, > const char *name, > const svn_string_t *value, > apr_pool_t *pool); > > to include the current value of the revprop, and then reject the > change if the current value does not match. That should be simple > because the FSFS implementation already takes a write lock and the BDB > implementation already uses a transaction. >
(this is now done) > 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), 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.) 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... >
