[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Wed, 22 Dec 2010 15:00:19 +0200

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
Received on 2010-12-22 14:05:58 CET

This is an archived mail posted to the Subversion Dev mailing list.