You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2010/06/08 10:35:21 UTC
revprop changes and hooks
Revision properties are unversioned that's well documented. There are
two hooks associated with a revprop change, the pre-revprop-change and
the post-revprop-change. What's not well documented is that these
hooks and the change are not executed within a Subversion transaction,
which means that the ACTION parameter passed to the hooks is not
reliable. The example pre-revprop-change hook created with every
repository includes:
if [ "$ACTION" = "M" -a "$PROPNAME" = "svn:log" ]; then exit 0; fi
which could lead one to believe that ACTION is reliable.
At the very least that's a documentation bug, but is it a code bug as
well? Is there any point in providing ACTION if it is not reliable?
Should we run the pre-revprop-change and the change itself in a
Subversion transaction? Something like:
Process 1 Process 2
--------- ---------
start txn
get old value
run pre-revprop-change
start txn
get old value
run pre-revprop-change
change-value
commit txn
run post-revprop-change
change value
fail to commit txn
return error
return success
--
Philip
Re: revprop changes and hooks
Posted by Philip Martin <ph...@wandisco.com>.
Peter Samuelson <pe...@p12n.org> writes:
> pre-revprop-change is mainly useful for access control,
That's because it's not reliable and so can't be used for much else.
If it was reliable it could be used for more, such as solving issue
3546.
post-revprop-change also gets the same unreliable value.
--
Philip
Re: revprop changes and hooks
Posted by Peter Samuelson <pe...@p12n.org>.
[Philip Martin]
> 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.
pre-revprop-change is mainly useful for access control, and I think for
the most part the three cases that can happen here ('A' becoming 'M'
due to another add; 'M' becoming 'A' due to a delete; or any action
becoming a no-op due to someone else doing the same action) are
harmless in that sense.
Unless, I suppose, you have an attacker who has permission to modify
but not add a property, and he wishes to make sure the property is not
deleted, so he modifies the property in a tight loop in hopes of racing
with whoever wants to delete it, hoping to end up re-adding it without
permission....?
Seems a bit far-fetched. I dunno.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, 18 Jun 2010 at 22:02 -0000:
> Daniel Shahaf wrote:
> > You mean, to extend the dav_db structure to hold that information? If
> > that's possible, I suppose the only candidate for filling in that
> > information is db_open(), who can examine its dav_resource object. But
> > does the dav_resource object contain the needed information? Or do we
> > need the request_rec object for that?
>
> Hrm, come to think of it, this might be something that isn't easy to do by
> merely extending the PROPPATCH request. mod_dav may simply not give us
> enough information. Is this something we need to employ HTTPv2 for? (I
> truly don't know at this point.)
Update: after talking to Greg and Mike on IRC, the current plan is to
use db_store()'s ELEM parameter to access the PROPPATCH XML document
tree, before trying other approaches (HTTPv2, POST, etc).
Re: revprop changes and hooks
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Shahaf wrote:
> C. Michael Pilato wrote on Fri, 18 Jun 2010 at 09:48 -0400:
>> Does mod_dav open and close the DAV properties database exactly once
>> per PROPPATCH?
>
> Yes. (I checked dav_method_proppatch() in <main/mod_dav.c>.) Why is
> this important to know?
Well, if it opened the props database once per connection, say, I just think
it would be more difficult to conceptually bind a value squirreled away in
that structure to a particular PROPPATCH invocation. If we can say that the
lifetime of the props DB matches the lifetime of the request that modifies
it, there's a cleaner argument for treating it as a request-specific context
baton.
> You mean, to extend the dav_db structure to hold that information? If
> that's possible, I suppose the only candidate for filling in that
> information is db_open(), who can examine its dav_resource object. But
> does the dav_resource object contain the needed information? Or do we
> need the request_rec object for that?
Hrm, come to think of it, this might be something that isn't easy to do by
merely extending the PROPPATCH request. mod_dav may simply not give us
enough information. Is this something we need to employ HTTPv2 for? (I
truly don't know at this point.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, 18 Jun 2010 at 09:48 -0400:
> Daniel Shahaf wrote:
> > * ra_dav: I don't know about this one.
> >
> > From the neon/serf side, adding the optional "expected preexisting
> > value" should be straightforward (just another tag/attribute).
> >
> > From the mod_dav_svn side, however, the propchanges/propdeletes come
> > from db_store/db_remove (respectively), and I don't know/see how
> > mod_dav_svn could obtain the optional "expected preexisting value"
> > (even if it were present in the request).
> >
> > Could someone teach me how mod_dav_svn might be able to obtain the
> > optional "expected preexisting value"?
>
> Could it get stuffed into the dav_db structure?
You mean, to extend the dav_db structure to hold that information? If
that's possible, I suppose the only candidate for filling in that
information is db_open(), who can examine its dav_resource object. But
does the dav_resource object contain the needed information? Or do we
need the request_rec object for that?
(Disclaimer: I could be talking complete nonsense; it's my first time
inside the httpd/mod_dav code.)
> Does mod_dav open and close the DAV properties database exactly once
> per PROPPATCH?
>
Yes. (I checked dav_method_proppatch() in <main/mod_dav.c>.) Why is
this important to know?
>
Re: revprop changes and hooks
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Shahaf wrote:
> * ra_dav: I don't know about this one.
>
> From the neon/serf side, adding the optional "expected preexisting
> value" should be straightforward (just another tag/attribute).
>
> From the mod_dav_svn side, however, the propchanges/propdeletes come
> from db_store/db_remove (respectively), and I don't know/see how
> mod_dav_svn could obtain the optional "expected preexisting value"
> (even if it were present in the request).
>
> Could someone teach me how mod_dav_svn might be able to obtain the
> optional "expected preexisting value"?
Could it get stuffed into the dav_db structure? Does mod_dav open and close
the DAV properties database exactly once per PROPPATCH?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ summary: a question about the ra_dav protocol ]
Daniel Shahaf wrote on Thu, 17 Jun 2010 at 16:21 +0300:
> Philip Martin wrote on Thu, 17 Jun 2010 at 12:18 -0000:
> > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> > > 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...
> >
Before just applying the libsvn_repos change, I'm looking into this.
* ra_local: trivial
* ra_svn: change-rev-prop's tuple may end before the value, so either
a new command or a new capability would be needed.
* ra_dav: I don't know about this one.
From the neon/serf side, adding the optional "expected preexisting
value" should be straightforward (just another tag/attribute).
From the mod_dav_svn side, however, the propchanges/propdeletes come
from db_store/db_remove (respectively), and I don't know/see how
mod_dav_svn could obtain the optional "expected preexisting value"
(even if it were present in the request).
Could someone teach me how mod_dav_svn might be able to obtain the
optional "expected preexisting value"?
Thanks!
Daniel
> > 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.
>
>
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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.
Re: revprop changes and hooks
Posted by Philip Martin <ph...@wandisco.com>.
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.
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
> 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).
> 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.
--
Philip
RE: revprop changes and hooks
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
Daniel Shahaf wrote:
> 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...
That would also fix the known race condition in svnsync.
Svnsync locks the remote repository by creating a revprop.
We really want that to be an atomic "create revprop if not
exist" operation, which would be possible with this new API.
The current implementation does a non-atomic test-then-set,
which is racy.
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000:
> Philip Martin <ph...@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...
>
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Wed, 16 Jun 2010 at 01:19 -0000:
> Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000:
> > Philip Martin <ph...@wandisco.com> writes:
> > > 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.
> >
>
> Testing a patch for this ^.
>
r955136. Comments welcome :-)
> Daniel
>
> > 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.
> >
> >
>
Re: revprop changes and hooks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Tue, 15 Jun 2010 at 11:16 -0000:
> Philip Martin <ph...@wandisco.com> writes:
> > 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.
>
Testing a patch for this ^.
Daniel
> 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.
>
>
Re: revprop changes and hooks
Posted by "C. Michael Pilato" <cm...@collab.net>.
Philip Martin wrote:
> Philip Martin <ph...@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);
I'm late to the party, here, but wanted to affirm this decision: nice way
to solve the problem, Philip.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: revprop changes and hooks
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@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.
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.
--
Philip
Re: revprop changes and hooks
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> Philip Martin wrote:
>> Revision properties are unversioned that's well documented. There are
>> two hooks associated with a revprop change, the pre-revprop-change and
>> the post-revprop-change. What's not well documented is that these
>> hooks and the change are not executed within a Subversion transaction,
>> which means that the ACTION parameter passed to the hooks is not
>> reliable.
>
> By "reliable", are you talking about a race condition that can be triggered
> by multiple clients trying to modify the same revision's properties or
> something? Do you have an example that explains your concern?
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).
--
Philip
Re: revprop changes and hooks
Posted by "C. Michael Pilato" <cm...@collab.net>.
Philip Martin wrote:
> Revision properties are unversioned that's well documented. There are
> two hooks associated with a revprop change, the pre-revprop-change and
> the post-revprop-change. What's not well documented is that these
> hooks and the change are not executed within a Subversion transaction,
> which means that the ACTION parameter passed to the hooks is not
> reliable.
By "reliable", are you talking about a race condition that can be triggered
by multiple clients trying to modify the same revision's properties or
something? Do you have an example that explains your concern?
> At the very least that's a documentation bug, but is it a code bug as
> well? Is there any point in providing ACTION if it is not reliable?
> Should we run the pre-revprop-change and the change itself in a
> Subversion transaction? Something like:
>
> Process 1 Process 2
> --------- ---------
> start txn
> get old value
> run pre-revprop-change
> start txn
> get old value
> run pre-revprop-change
> change-value
> commit txn
> run post-revprop-change
> change value
> fail to commit txn
> return error
> return success
This rather depends on the idea of Subversion having a concept of a
transaction apart from "revision in the making"? And implemented in the
repository layer, even.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand