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 Thu, 17 Jun 2010 13:21:39 GMT
Philip Martin wrote on Thu, 17 Jun 2010 at 12:18 -0000:
> Daniel Shahaf <d.s@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(&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),
> 
> 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.


Mime
View raw message