subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: revprop changes and hooks
Date Wed, 16 Jun 2010 23:29:14 GMT
Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000:
> Philip Martin <philip.martin@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(&current_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...

> 

Mime
View raw message