You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2012/04/25 13:28:33 UTC
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c
libsvn_repos/commit.c
gstein@apache.org wrote on Tue, Apr 24, 2012 at 23:33:24 -0000:
> Author: gstein
> Date: Tue Apr 24 23:33:23 2012
> New Revision: 1330058
>
> URL: http://svn.apache.org/viewvc?rev=1330058&view=rev
> Log:
> Rejigger how FS editors are completed and how the commit process works.
>
> Modified: subversion/trunk/subversion/libsvn_fs/editor.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/editor.c?rev=1330058&r1=1330057&r2=1330058&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs/editor.c (original)
> +++ subversion/trunk/subversion/libsvn_fs/editor.c Tue Apr 24 23:33:23 2012
> @@ -38,8 +38,8 @@ struct edit_baton {
> /* The transaction associated with this editor. */
> svn_fs_txn_t *txn;
>
> - /* Should the transaction be committed when complete_cb() is invoked? */
> - svn_boolean_t autocommit;
> + /* Has this editor been completed? */
> + svn_boolean_t completed;
>
> /* We sometimes need the cancellation beyond what svn_editor_t provides */
> svn_cancel_func_t cancel_func;
> @@ -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.
(Nice handling of all the various branches in svn_fs_editor_commit(),
BTW. Tricky code.)
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c
libsvn_repos/commit.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Wed, Apr 25, 2012 at 16:16:11 -0400:
> On Wed, Apr 25, 2012 at 07:28, Daniel Shahaf <da...@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
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 25, 2012 at 07:28, Daniel Shahaf <da...@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?
> (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.
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.
Cheers,
-g