You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2010/12/20 19:14:34 UTC
svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
The docs for svn_fs_commit_txn() for read:
* @note Success or failure of the commit of @a txn is determined by
* examining the value of @a *new_rev upon this function's return. If
* the value is a valid revision number, the commit was successful,
* even though a non-@c NULL function return value may indicate that
* something else went wrong.
However, svn_repos_fs_commit_txn() will only run the post-commit hook if
SVN_NO_ERROR was returned:
/* Commit. */
SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
/* Run post-commit hooks. Notice that we're wrapping the error
with a -specific- errorcode, so that our caller knows not to try
and abort the transaction. */
if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
return svn_error_create
(SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
_("Commit succeeded, but post-commit hook failed"));
return SVN_NO_ERROR;
Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if new_rev
is a valid rev?
BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev is
always modified, so the caller doesn't have to set *new_rev to SVN_INVALID_REVNUM.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 19:48:49 -0800:
> On 12/21/10 5:57 PM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 13:47:40 -0800:
>>> On 12/21/10 11:44 AM, Daniel Shahaf wrote:
>>>> Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
>>>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>>>>> svn_fs_commit_txn()'s error as the parent followed by the
>>>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>>>>> the standard ordering of chained errors. On the other hand, it makes it
>>>>>> harder to find a post-commit script error.
>>>>>
>>>>> Actually, it will make it impossible to detect post-commit errors over
>>>>> ra_dav, since that RA layer marshals only the outermost error code in an
>>>>> error chain.
>>>>
>>>> This is now<http://subversion.tigris.org/issues/show_bug.cgi?id=3767>.
>>>>
>>>> (Details are partly from memory, partly from quick testing I re-did today.)
>>>
>>> While we're opening tickets, I found an issue with
>>> svn_repos_parse_fns2_t.close_revision(), it doesn't support the
>>> documented svn_fs_commit_txn() contract, I opened this to track it and
>>> put it in 1.7-consider, as it shouldn't block 1.7, but it would be nice
>>> to have it in there.
>>>
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=3768
>>>
>>
>> svn_repos_parse_fns2_t is about parsing a dumpstream, not about
>> committing the expressed revisions to some repository, so I don't
>> understand why the "NEW_REV" parameter belongs there and not in
>> svn_repos_get_fs_build_parser3().
>
> I haven't used nor looked too closely at this API, but reading the API,
> it can commit multiple revisions, so shouldn't the errors be returned
> from each revision committed?
>
Definitely.
> In svn_repos_parse_dumpstream2(), I see that it commits each revision.
>
As I understand it:
* svn_repos_parse_dumpstream2()
input: dump stream
output: drives a provided svn_repos_parse_fns2_t vtable
* svn_repos_get_fs_build_parser3()
input: svn_repos_t handle
output: svn_repos_parse_fns2_t vtable that, when driven, commits to
the provided repository
Therefore, any notion of "Did the commit succeed" and "What revnum it
resulted in" belongs in the latter, not in the former.
Just consider alternative implementors of the vtable: does NEW_REV make
sense for 'svnrdump load'? Does it make sense for a vtable
implementation that just prints what callbacks were called (akin to the
debug_editor)?
> /* If we already have a rev_baton open, we need to close it
> and clear the per-revision subpool. */
> if (rev_baton != NULL)
> {
> SVN_ERR(parse_fns->close_revision(rev_baton));
>
> This code couldn't handle a successful commit followed by an internal FS
> failure, so close_revision() would need to have a *new_rev argument.
>
> Let me know if I'm reading this correctly.
>
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 5:57 PM, Daniel Shahaf wrote:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 13:47:40 -0800:
>> On 12/21/10 11:44 AM, Daniel Shahaf wrote:
>>> Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
>>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>>>> svn_fs_commit_txn()'s error as the parent followed by the
>>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>>>> the standard ordering of chained errors. On the other hand, it makes it
>>>>> harder to find a post-commit script error.
>>>>
>>>> Actually, it will make it impossible to detect post-commit errors over
>>>> ra_dav, since that RA layer marshals only the outermost error code in an
>>>> error chain.
>>>
>>> This is now<http://subversion.tigris.org/issues/show_bug.cgi?id=3767>.
>>>
>>> (Details are partly from memory, partly from quick testing I re-did today.)
>>
>> While we're opening tickets, I found an issue with
>> svn_repos_parse_fns2_t.close_revision(), it doesn't support the
>> documented svn_fs_commit_txn() contract, I opened this to track it and
>> put it in 1.7-consider, as it shouldn't block 1.7, but it would be nice
>> to have it in there.
>>
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3768
>>
>
> svn_repos_parse_fns2_t is about parsing a dumpstream, not about
> committing the expressed revisions to some repository, so I don't
> understand why the "NEW_REV" parameter belongs there and not in
> svn_repos_get_fs_build_parser3().
I haven't used nor looked too closely at this API, but reading the API, it can
commit multiple revisions, so shouldn't the errors be returned from each
revision committed?
In svn_repos_parse_dumpstream2(), I see that it commits each revision.
/* If we already have a rev_baton open, we need to close it
and clear the per-revision subpool. */
if (rev_baton != NULL)
{
SVN_ERR(parse_fns->close_revision(rev_baton));
This code couldn't handle a successful commit followed by an internal FS
failure, so close_revision() would need to have a *new_rev argument.
Let me know if I'm reading this correctly.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 13:47:40 -0800:
> On 12/21/10 11:44 AM, Daniel Shahaf wrote:
>> Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>>> svn_fs_commit_txn()'s error as the parent followed by the
>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>>> the standard ordering of chained errors. On the other hand, it makes it
>>>> harder to find a post-commit script error.
>>>
>>> Actually, it will make it impossible to detect post-commit errors over
>>> ra_dav, since that RA layer marshals only the outermost error code in an
>>> error chain.
>>
>> This is now<http://subversion.tigris.org/issues/show_bug.cgi?id=3767>.
>>
>> (Details are partly from memory, partly from quick testing I re-did today.)
>
> While we're opening tickets, I found an issue with
> svn_repos_parse_fns2_t.close_revision(), it doesn't support the
> documented svn_fs_commit_txn() contract, I opened this to track it and
> put it in 1.7-consider, as it shouldn't block 1.7, but it would be nice
> to have it in there.
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=3768
>
svn_repos_parse_fns2_t is about parsing a dumpstream, not about
committing the expressed revisions to some repository, so I don't
understand why the "NEW_REV" parameter belongs there and not in
svn_repos_get_fs_build_parser3().
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 11:44 AM, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>> svn_fs_commit_txn()'s error as the parent followed by the
>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>> the standard ordering of chained errors. On the other hand, it makes it
>>> harder to find a post-commit script error.
>>
>> Actually, it will make it impossible to detect post-commit errors over
>> ra_dav, since that RA layer marshals only the outermost error code in an
>> error chain.
>
> This is now<http://subversion.tigris.org/issues/show_bug.cgi?id=3767>.
>
> (Details are partly from memory, partly from quick testing I re-did today.)
While we're opening tickets, I found an issue with
svn_repos_parse_fns2_t.close_revision(), it doesn't support the documented
svn_fs_commit_txn() contract, I opened this to track it and put it in
1.7-consider, as it shouldn't block 1.7, but it would be nice to have it in there.
http://subversion.tigris.org/issues/show_bug.cgi?id=3768
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Dec 21, 2010 at 20:40:02 +0200:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
> > 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
> > svn_fs_commit_txn()'s error as the parent followed by the
> > SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
> > the standard ordering of chained errors. On the other hand, it makes it
> > harder to find a post-commit script error.
>
> Actually, it will make it impossible to detect post-commit errors over
> ra_dav, since that RA layer marshals only the outermost error code in an
> error chain.
This is now <http://subversion.tigris.org/issues/show_bug.cgi?id=3767>.
(Details are partly from memory, partly from quick testing I re-did today.)
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 4:03 PM, Daniel Shahaf wrote:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 15:55:30 -0800:
>> On 12/21/10 3:22 PM, Blair Zajac wrote:
>>> On 12/21/10 10:55 AM, Blair Zajac wrote:
>>>> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>>>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>>>> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>>>>>>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>>>>>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>>>>>>> new_rev is a valid rev?
>>>>>>>
>>>>>>> That does seem reasonable, yes.
>>>>>>
>>>>>> Looking through our code, no existing use of svn_fs_commit_txn() and
>>>>>> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
>>>>>> checks for a non-NULL svn_error_t * and checks if the parent error is a
>>>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
>>>>>> for that error.
>>>>>>
>>>>>
>>>>> svn_error_has_cause() could be used for that.
>>>
>>> Daniel,
>>>
>>> I need a version of svn_error_has_cause() that returns the actual error. How
>>> about replacing svn_error_has_cause() with svn_error_find_cause() that returns a
>>> NULL or the error? It could still be used for the Boolean test case in this way.
>>
>> I checked if you were on IRC, but I've got a deadline on a project I'm
>> working on and needed this, so I went ahead and made the change.
>>
>> r1051702
>>
>
> I wasn't at the computer then.
>
> Patch looks good. My only comment is that the docstring might want to
> warn against using svn_error_clear() on the returned value.
Thanks, done in r1051713.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 15:55:30 -0800:
> On 12/21/10 3:22 PM, Blair Zajac wrote:
>> On 12/21/10 10:55 AM, Blair Zajac wrote:
>>> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>>> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>>>>>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>>>>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>>>>>> new_rev is a valid rev?
>>>>>>
>>>>>> That does seem reasonable, yes.
>>>>>
>>>>> Looking through our code, no existing use of svn_fs_commit_txn() and
>>>>> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
>>>>> checks for a non-NULL svn_error_t * and checks if the parent error is a
>>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
>>>>> for that error.
>>>>>
>>>>
>>>> svn_error_has_cause() could be used for that.
>>
>> Daniel,
>>
>> I need a version of svn_error_has_cause() that returns the actual error. How
>> about replacing svn_error_has_cause() with svn_error_find_cause() that returns a
>> NULL or the error? It could still be used for the Boolean test case in this way.
>
> I checked if you were on IRC, but I've got a deadline on a project I'm
> working on and needed this, so I went ahead and made the change.
>
> r1051702
>
I wasn't at the computer then.
Patch looks good. My only comment is that the docstring might want to
warn against using svn_error_clear() on the returned value.
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 3:22 PM, Blair Zajac wrote:
> On 12/21/10 10:55 AM, Blair Zajac wrote:
>> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>>>>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>>>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>>>>> new_rev is a valid rev?
>>>>>
>>>>> That does seem reasonable, yes.
>>>>
>>>> Looking through our code, no existing use of svn_fs_commit_txn() and
>>>> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
>>>> checks for a non-NULL svn_error_t * and checks if the parent error is a
>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
>>>> for that error.
>>>>
>>>
>>> svn_error_has_cause() could be used for that.
>
> Daniel,
>
> I need a version of svn_error_has_cause() that returns the actual error. How
> about replacing svn_error_has_cause() with svn_error_find_cause() that returns a
> NULL or the error? It could still be used for the Boolean test case in this way.
I checked if you were on IRC, but I've got a deadline on a project I'm working
on and needed this, so I went ahead and made the change.
r1051702
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 10:55 AM, Blair Zajac wrote:
> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>>>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>>>> new_rev is a valid rev?
>>>>
>>>> That does seem reasonable, yes.
>>>
>>> Looking through our code, no existing use of svn_fs_commit_txn() and
>>> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
>>> checks for a non-NULL svn_error_t * and checks if the parent error is a
>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
>>> for that error.
>>>
>>
>> svn_error_has_cause() could be used for that.
Daniel,
I need a version of svn_error_has_cause() that returns the actual error. How
about replacing svn_error_has_cause() with svn_error_find_cause() that returns a
NULL or the error? It could still be used for the Boolean test case in this way.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/21/2010 02:57 PM, Daniel Shahaf wrote:
> C. Michael Pilato wrote on Tue, Dec 21, 2010 at 14:40:01 -0500:
>> Can we fix this? Can we introduce a new error code
>> SVN_ERR_RA_DAV_ERROR_CHAIN which means, "the descriptive message of this
>> error contains a skel which, when parsed, carries a whole chain of real
>> errors? Then we have the client indicate at capabilities-exchange time that
>> it can handle that kind of return.
>>
>
> +0. (not a +1 because I don't know the ra_dav protocol well enough)
>
> Instead of skels, we could re-use svn_ra_svn_write_cmd_failure() and
> svn_ra_svn__handle_failure_status(), though.
... and then make the results all XML-safe. *sigh*
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Tue, Dec 21, 2010 at 14:40:01 -0500:
> On 12/21/2010 02:08 PM, Daniel Shahaf wrote:
> > Blair Zajac wrote on Tue, Dec 21, 2010 at 10:55:37 -0800:
> >> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
> >>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
> >>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
> >>>> svn_fs_commit_txn()'s error as the parent followed by the
> >>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
> >>>> the standard ordering of chained errors. On the other hand, it makes it
> >>>> harder to find a post-commit script error.
> >>>
> >>> Actually, it will make it impossible to detect post-commit errors over
> >>> ra_dav, since that RA layer marshals only the outermost error code in an
> >>> error chain.
> >>
> >> ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could
> >> use svn_error_has_cause() to find it.
> >
> > How can it do that if only the topmost error code is marshalled?
> >
> > I first discovered that ra_dav only marshals the outermost error code
> > (and its error message, but nothing else of the chain) when I worked on
> > the atomic-revprops branch. In dev@ archives there should be some
> > discussions of how error chain marshalling might be implemented, but
> > eventually I solved the problem differently for that branch (by using
> > another error-signalling mechanism).
> >
> > ra_svn marshals full error chains.
>
> Can we fix this? Can we introduce a new error code
> SVN_ERR_RA_DAV_ERROR_CHAIN which means, "the descriptive message of this
> error contains a skel which, when parsed, carries a whole chain of real
> errors? Then we have the client indicate at capabilities-exchange time that
> it can handle that kind of return.
>
+0. (not a +1 because I don't know the ra_dav protocol well enough)
Instead of skels, we could re-use svn_ra_svn_write_cmd_failure() and
svn_ra_svn__handle_failure_status(), though.
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/21/2010 02:08 PM, Daniel Shahaf wrote:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:55:37 -0800:
>> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>>> svn_fs_commit_txn()'s error as the parent followed by the
>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>>> the standard ordering of chained errors. On the other hand, it makes it
>>>> harder to find a post-commit script error.
>>>
>>> Actually, it will make it impossible to detect post-commit errors over
>>> ra_dav, since that RA layer marshals only the outermost error code in an
>>> error chain.
>>
>> ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could
>> use svn_error_has_cause() to find it.
>
> How can it do that if only the topmost error code is marshalled?
>
> I first discovered that ra_dav only marshals the outermost error code
> (and its error message, but nothing else of the chain) when I worked on
> the atomic-revprops branch. In dev@ archives there should be some
> discussions of how error chain marshalling might be implemented, but
> eventually I solved the problem differently for that branch (by using
> another error-signalling mechanism).
>
> ra_svn marshals full error chains.
Can we fix this? Can we introduce a new error code
SVN_ERR_RA_DAV_ERROR_CHAIN which means, "the descriptive message of this
error contains a skel which, when parsed, carries a whole chain of real
errors? Then we have the client indicate at capabilities-exchange time that
it can handle that kind of return.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 11:08 AM, Daniel Shahaf wrote:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:55:37 -0800:
>> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>>> svn_fs_commit_txn()'s error as the parent followed by the
>>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>>> the standard ordering of chained errors. On the other hand, it makes it
>>>> harder to find a post-commit script error.
>>>
>>> Actually, it will make it impossible to detect post-commit errors over
>>> ra_dav, since that RA layer marshals only the outermost error code in an
>>> error chain.
>>
>> ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could
>> use svn_error_has_cause() to find it.
>
> How can it do that if only the topmost error code is marshalled?
>
> I first discovered that ra_dav only marshals the outermost error code
> (and its error message, but nothing else of the chain) when I worked on
> the atomic-revprops branch. In dev@ archives there should be some
> discussions of how error chain marshalling might be implemented, but
> eventually I solved the problem differently for that branch (by using
> another error-signalling mechanism).
>
> ra_svn marshals full error chains.
Sorry, I was thinking of mod_dav_svn, not ra_dav, which checks for
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED and clears the error.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 10:55:37 -0800:
> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>> svn_fs_commit_txn()'s error as the parent followed by the
>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>> the standard ordering of chained errors. On the other hand, it makes it
>>> harder to find a post-commit script error.
>>
>> Actually, it will make it impossible to detect post-commit errors over
>> ra_dav, since that RA layer marshals only the outermost error code in an
>> error chain.
>
> ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could
> use svn_error_has_cause() to find it.
How can it do that if only the topmost error code is marshalled?
I first discovered that ra_dav only marshals the outermost error code
(and its error message, but nothing else of the chain) when I worked on
the atomic-revprops branch. In dev@ archives there should be some
discussions of how error chain marshalling might be implemented, but
eventually I solved the problem differently for that branch (by using
another error-signalling mechanism).
ra_svn marshals full error chains.
> If ra_dav cannot find a
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED but there is an error and the
> commit succeeded, then I think the parent error should be returned to
> warn people there may be an issue with the server.
>
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 10:55:37 -0800:
> On 12/21/10 10:40 AM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>>> svn_fs_commit_txn()'s error as the parent followed by the
>>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>>> the standard ordering of chained errors. On the other hand, it makes it
>>> harder to find a post-commit script error.
>>
>> Actually, it will make it impossible to detect post-commit errors over
>> ra_dav, since that RA layer marshals only the outermost error code in an
>> error chain.
>
> ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could
> use svn_error_has_cause() to find it. If ra_dav cannot find a
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED but there is an error and the
> commit succeeded, then I think the parent error should be returned to
> warn people there may be an issue with the server.
(I'm reading s/ra_dav/mod_dav_svn/)
+1, modulo one change: the "there may be issue with the server" is not
just when SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED isn't found, but also
when that is found but svn_fs_commit_txn() failed as well.
I'm not sure I have a good suggestion, but principly we should be sure
we allow the user/admin to understand exactly what happened ---
post-commit hook failed, post-commit FS juggling failed, or both.
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/21/10 10:40 AM, Daniel Shahaf wrote:
> Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
>> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>>> new_rev is a valid rev?
>>>
>>> That does seem reasonable, yes.
>>
>> Looking through our code, no existing use of svn_fs_commit_txn() and
>> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
>> checks for a non-NULL svn_error_t * and checks if the parent error is a
>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
>> for that error.
>>
>
> svn_error_has_cause() could be used for that.
Thanks, I'll use that. For 1.6.x, have to make a private version of it.
>> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
>> svn_fs_commit_txn()'s error as the parent followed by the
>> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
>> the standard ordering of chained errors. On the other hand, it makes it
>> harder to find a post-commit script error.
>
> Actually, it will make it impossible to detect post-commit errors over
> ra_dav, since that RA layer marshals only the outermost error code in an
> error chain.
ra_dav already checks for SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, it could use
svn_error_has_cause() to find it. If ra_dav cannot find a
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED but there is an error and the commit
succeeded, then I think the parent error should be returned to warn people there
may be an issue with the server.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>> new_rev is a valid rev?
>>
>> That does seem reasonable, yes.
>
> Looking through our code, no existing use of svn_fs_commit_txn() and
> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
> checks for a non-NULL svn_error_t * and checks if the parent error is a
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
> for that error.
>
svn_error_has_cause() could be used for that.
> Running this by the group before I commit, I think the following should be done.
>
> 1) In fs_fs.c's commit_body(), move the call to set *new_rev up, this
> matches the documented behavior and is technically the correct thing to
> do. If purging the txn fails, then the transaction was still committed
> and is visible to other processes.
>
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -6206,13 +6206,19 @@
> /* Update the 'current' file. */
> SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,
> start_copy_id, pool));
> +
> + /* At this point the new revision is committed and globally visible
> + so let the caller know it succeeded by giving it the new revision
> + number, which fullfils svn_fs_commit_txn() contract. Any errors
> + after this point do not change the fact that a new revision was
> + created. */
> + *cb->new_rev_p = new_rev;
> +
> ffd->youngest_rev_cache = new_rev;
>
> /* Remove this transaction directory. */
> SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
>
> - *cb->new_rev_p = new_rev;
> -
> return SVN_NO_ERROR;
> }
>
>
> Taking a quick look at BDB, it looks like it sets *new_rev immediately.
>
> 2) Each call to svn_fs_commit_txn() and svn_repos_fs_commit_txn() in our
> source should be updated to check for a valid new revision. If it needs
> to care about a hook error it can.
>
> 3) svn_repos_fs_commit_txn() should just check for a valid new revision
> to run the hook script.
>
> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
> svn_fs_commit_txn()'s error as the parent followed by the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
> the standard ordering of chained errors. On the other hand, it makes it
> harder to find a post-commit script error.
Actually, it will make it impossible to detect post-commit errors over
ra_dav, since that RA layer marshals only the outermost error code in an
error chain.
> Or should the ordering have
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the parent so callers just need
> to check the parent error?
>
Regardless of the ordering, I think it's fine to have
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the top-most error code,
since clients currently look for that specific error code.
But my intuition is that svn_fs_commit_txn()'s error should be first...
> It seems useful to add to svn_error_t the concept of starting a new chain
> of errors. Right now when we compose, we don't know when an unrelated
> error starts.
Agreed. However, I don't think of it as "start a new chain"; I think of
it as "two separate chains". (Possibly because of how we use
svn_error_compose_create() all the time.)
/** Like svn_error_t, but with SECONDARY added. */
struct svn_error2_t {
apr_status_t apr_err;
const char *message;
struct svn_error2_t *child;
apr_pool_t *pool;
const char *file;
long line;
struct svn_error2_t *secondary;
};
> Actually, this suggests that we use the normal ordering of
> errors, svn_fs_commit_txn() as the parent with
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED's as the child, this way the error
> chain can be scanned and the unrelated error easily determined. In the
> reverse ordering, one cannot tell when svn_fs_commit_txn()'s error
> starts.
>
> Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/20/10 11:32 AM, C. Michael Pilato wrote:
> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>> new_rev is a valid rev?
>
> That does seem reasonable, yes.
Looking through our code, no existing use of svn_fs_commit_txn() and
svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code checks for
a non-NULL svn_error_t * and checks if the parent error is a
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain for that
error.
Running this by the group before I commit, I think the following should be done.
1) In fs_fs.c's commit_body(), move the call to set *new_rev up, this matches
the documented behavior and is technically the correct thing to do. If purging
the txn fails, then the transaction was still committed and is visible to other
processes.
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -6206,13 +6206,19 @@
/* Update the 'current' file. */
SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,
start_copy_id, pool));
+
+ /* At this point the new revision is committed and globally visible
+ so let the caller know it succeeded by giving it the new revision
+ number, which fullfils svn_fs_commit_txn() contract. Any errors
+ after this point do not change the fact that a new revision was
+ created. */
+ *cb->new_rev_p = new_rev;
+
ffd->youngest_rev_cache = new_rev;
/* Remove this transaction directory. */
SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
- *cb->new_rev_p = new_rev;
-
return SVN_NO_ERROR;
}
Taking a quick look at BDB, it looks like it sets *new_rev immediately.
2) Each call to svn_fs_commit_txn() and svn_repos_fs_commit_txn() in our source
should be updated to check for a valid new revision. If it needs to care about
a hook error it can.
3) svn_repos_fs_commit_txn() should just check for a valid new revision to run
the hook script.
4) In svn_repos_fs_commit_txn(), which order should errors be composed?
svn_fs_commit_txn()'s error as the parent followed by the
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be the
standard ordering of chained errors. On the other hand, it makes it harder to
find a post-commit script error. Or should the ordering have
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the parent so callers just need to
check the parent error?
It seems useful to add to svn_error_t the concept of starting a new chain of
errors. Right now when we compose, we don't know when an unrelated error
starts. Actually, this suggests that we use the normal ordering of errors,
svn_fs_commit_txn() as the parent with SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED's
as the child, this way the error chain can be scanned and the unrelated error
easily determined. In the reverse ordering, one cannot tell when
svn_fs_commit_txn()'s error starts.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/20/2010 02:14 PM, Blair Zajac wrote:
> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
> new_rev is a valid rev?
That does seem reasonable, yes.
> BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev is
> always modified, so the caller doesn't have to set *new_rev to
> SVN_INVALID_REVNUM.
We normally don't make that sort of doc statement. But, in a case such as
this -- where success/fail is determined by something other than an error
value -- I agree that it makes sense.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Philip Martin <ph...@wandisco.com>.
Blair Zajac <bl...@orcaware.com> writes:
> The docs for svn_fs_commit_txn() for read:
>
> * @note Success or failure of the commit of @a txn is determined by
> * examining the value of @a *new_rev upon this function's return. If
> * the value is a valid revision number, the commit was successful,
> * even though a non-@c NULL function return value may indicate that
> * something else went wrong.
>
> However, svn_repos_fs_commit_txn() will only run the post-commit hook
> if SVN_NO_ERROR was returned:
>
> /* Commit. */
> SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
>
> /* Run post-commit hooks. Notice that we're wrapping the error
> with a -specific- errorcode, so that our caller knows not to try
> and abort the transaction. */
> if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
> return svn_error_create
> (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
> _("Commit succeeded, but post-commit hook failed"));
>
> return SVN_NO_ERROR;
>
> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
> new_rev is a valid rev?
I think you are correct, it should.
> BTW, we should have the docs for svn_fs_commit_txn mention that
> *new_rev is always modified, so the caller doesn't have to set
> *new_rev to SVN_INVALID_REVNUM.
It's implied by the note comment. I suppose it could be clarified.
--
Philip
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Mon, Dec 20, 2010 at 19:59:37 +0000:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
> >>
> >> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
> >> new_rev is a valid rev?
> >>
> >
> > That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
> > return value. When can that happen?
>
> Packing or rep-caching processing after the new rev is created.
>
Agreed that (in both of these cases) running the post-commit hook anyway
would make sense.
> --
> Philip
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
>>
>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>> new_rev is a valid rev?
>>
>
> That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
> return value. When can that happen?
Packing or rep-caching processing after the new rev is created.
--
Philip
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Mon, Dec 20, 2010 at 12:03:09 -0800:
>
> On Dec 20, 2010, at 11:48 AM, Daniel Shahaf wrote:
>
> > Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
> >> The docs for svn_fs_commit_txn() for read:
> >>
> >> * @note Success or failure of the commit of @a txn is determined by
> >> * examining the value of @a *new_rev upon this function's return. If
> >> * the value is a valid revision number, the commit was successful,
> >> * even though a non-@c NULL function return value may indicate that
> >> * something else went wrong.
> >>
> >> However, svn_repos_fs_commit_txn() will only run the post-commit hook if
> >> SVN_NO_ERROR was returned:
> >>
> >> /* Commit. */
> >> SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
> >>
> >> /* Run post-commit hooks. Notice that we're wrapping the error
> >> with a -specific- errorcode, so that our caller knows not to try
> >> and abort the transaction. */
> >> if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
> >> return svn_error_create
> >> (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
> >> _("Commit succeeded, but post-commit hook failed"));
> >>
> >> return SVN_NO_ERROR;
> >>
> >> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
> >> new_rev is a valid rev?
> >>
> >
> > That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
> > return value. When can that happen?
> >
> > (just on Saturday I drafted a patch to make failing to update
> > rep-cache.db after the commit itself succeeded not be considered
> > a fatal error; that would be one case when that can happen.)
>
> Was the patch going to swallow the error? If there is a deployment issue causing rep-cache.db not to be updated, I would like to know about it.
>
It wasn't going to silently swallow the error:
http://mid.gmane.org/20101218184143.GA8544@daniel3.local
> Blair
>
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Blair Zajac <bl...@orcaware.com>.
On Dec 20, 2010, at 11:48 AM, Daniel Shahaf wrote:
> Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
>> The docs for svn_fs_commit_txn() for read:
>>
>> * @note Success or failure of the commit of @a txn is determined by
>> * examining the value of @a *new_rev upon this function's return. If
>> * the value is a valid revision number, the commit was successful,
>> * even though a non-@c NULL function return value may indicate that
>> * something else went wrong.
>>
>> However, svn_repos_fs_commit_txn() will only run the post-commit hook if
>> SVN_NO_ERROR was returned:
>>
>> /* Commit. */
>> SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
>>
>> /* Run post-commit hooks. Notice that we're wrapping the error
>> with a -specific- errorcode, so that our caller knows not to try
>> and abort the transaction. */
>> if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
>> return svn_error_create
>> (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
>> _("Commit succeeded, but post-commit hook failed"));
>>
>> return SVN_NO_ERROR;
>>
>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>> new_rev is a valid rev?
>>
>
> That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
> return value. When can that happen?
>
> (just on Saturday I drafted a patch to make failing to update
> rep-cache.db after the commit itself succeeded not be considered
> a fatal error; that would be one case when that can happen.)
Was the patch going to swallow the error? If there is a deployment issue causing rep-cache.db not to be updated, I would like to know about it.
Blair
Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Blair Zajac wrote on Mon, Dec 20, 2010 at 11:14:34 -0800:
> The docs for svn_fs_commit_txn() for read:
>
> * @note Success or failure of the commit of @a txn is determined by
> * examining the value of @a *new_rev upon this function's return. If
> * the value is a valid revision number, the commit was successful,
> * even though a non-@c NULL function return value may indicate that
> * something else went wrong.
>
> However, svn_repos_fs_commit_txn() will only run the post-commit hook if
> SVN_NO_ERROR was returned:
>
> /* Commit. */
> SVN_ERR(svn_fs_commit_txn(conflict_p, new_rev, txn, pool));
>
> /* Run post-commit hooks. Notice that we're wrapping the error
> with a -specific- errorcode, so that our caller knows not to try
> and abort the transaction. */
> if ((err = svn_repos__hooks_post_commit(repos, *new_rev, pool)))
> return svn_error_create
> (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
> _("Commit succeeded, but post-commit hook failed"));
>
> return SVN_NO_ERROR;
>
> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
> new_rev is a valid rev?
>
That matters when NEW_REV is a valid rev but there is a non-SVN_NO_ERROR
return value. When can that happen?
(just on Saturday I drafted a patch to make failing to update
rep-cache.db after the commit itself succeeded not be considered
a fatal error; that would be one case when that can happen.)
> BTW, we should have the docs for svn_fs_commit_txn mention that *new_rev
> is always modified, so the caller doesn't have to set *new_rev to
> SVN_INVALID_REVNUM.
>
> Blair