Greg Stein wrote on Wed, Apr 25, 2012 at 16:16:11 -0400: > On Wed, Apr 25, 2012 at 07:28, Daniel Shahaf <[email protected]> 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
