Greg Stein wrote on Wed, Apr 25, 2012 at 16:16:11 -0400:
> On Wed, Apr 25, 2012 at 07:28, Daniel Shahaf <danielsh_at_elego.de> wrote:
> >...
> >> @@ -326,15 +326,17 @@ complete_cb(void *baton,
> >> {
> >> struct edit_baton *eb = baton;
> >>
> >> + /* Watch out for a following call to svn_fs_editor_commit(). Note that
> >> + we are likely here because svn_fs_editor_commit() was called, and it
> >> + invoked svn_editor_complete(). */
> >> + eb->completed = TRUE;
> >> +
> >
> > The code doesn't prevent people from calling svn_editor_complete() twice
> > when the second call is not via svn_fs_editor_commit(), but the
> > docstring says it would error in that case.
>
> When SVN_DEBUG, svn_editor prevents double-calls to complete(). And
> since svn_fs_editor_commit() calls complete(), if a caller followed
> with a call to complete, they'd get an editor assertion.
>
> Now. That isn't quite what the docstring describes, but the important
> part is "don't call both", and that is prevented when SVN_DEBUG. I
> didn't think it important to enforce strictly, and even considered
> moving the above "completed" flag into SVN_DEBUG.
>
> Thoughts?
>
The docstring is specific about "An error will be returned if
this-and-that" (as opposed to just saying "it's not permited to do
this-and-that"), so I'd rather see the docstring and the code in sync.
Whether that means rephrasing the docs or adding an if() to the code is
a good question.. which I'm going to apply lazy evaluation to. :-)
> > (Nice handling of all the various branches in svn_fs_editor_commit(),
> > BTW. Tricky code.)
>
> Thanks :-) ... I think the current outputs/definition of
> svn_fs_editor_commit() is cleaner than some of the various handling we
> have scattered about to deal with some of this stuff.
>
Yes. Overall, callers can just SVN_ERR() the function, and if it then
example the return values: either *REVISION or *CONFLICT_PATH, and in
the former case potentially a *POST_COMMIT_ERR too. (Hmm -- we should
put that last sentence in the docstring?)
> I appreciate the summary you put into svn_fs.h. I just realized that I
> need to further update that docstring: the txn will be committed or
> aborted upon exit of the function. Callers don't need to worry about
> cleanup.
>
And callers can't keep the transaction open to re-try it, either.
(Callers that want that would need to use the 1.7 svn_fs API.)
> Cheers,
> -g
Received on 2012-04-25 22:58:45 CEST