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 <d....@daniel.shahaf.name> on 2010/12/22 13:57:02 UTC
Re: svn commit: r1051778 - in /subversion/trunk/subversion:
libsvn_repos/commit.c libsvn_repos/load-fs-vtable.c
mod_dav_svn/lock.c mod_dav_svn/version.c
blair@apache.org wrote on Wed, Dec 22, 2010 at 07:10:06 -0000:
> Author: blair
> Date: Wed Dec 22 07:10:05 2010
> New Revision: 1051778
>
> URL: http://svn.apache.org/viewvc?rev=1051778&view=rev
> Log:
> Have all remaining calls of svn_fs_commit_txn() and
> svn_repos_fs_commit_txn() use the contract that a commit was
> successful if the returned revision is a valid revision number. The
> returned error, if any, is no longer used as an indication of commit
> success.
>
> * subversion/mod_dav_svn/version.c
> (dav_svn__checkin),
> (merge):
> Use revision number returned from svn_repos_fs_commit_txn() to
> test for a successful commit.
>
> * subversion/mod_dav_svn/lock.c
> (append_locks):
> Ditto.
>
> * subversion/libsvn_repos/load-fs-vtable.c
> (close_revision):
> Ditto.
>
> * subversion/libsvn_repos/commit.c
> (close_edit):
> Ditto.
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/commit.c
> subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
> subversion/trunk/subversion/mod_dav_svn/lock.c
> subversion/trunk/subversion/mod_dav_svn/version.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051778&r1=1051777&r2=1051778&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 07:10:05 2010
> @@ -716,9 +716,9 @@ close_edit(void *edit_baton,
> err = svn_repos_fs_commit_txn(&conflict, eb->repos,
> &new_revision, eb->txn, pool);
>
> - if (err)
> + if (SVN_IS_VALID_REVNUM(new_revision))
> {
> - if (err->apr_err == SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED)
> + if (err)
> {
> /* If the error was in post-commit, then the commit itself
> succeeded. In which case, save the post-commit warning
> @@ -729,28 +729,28 @@ close_edit(void *edit_baton,
> svn_error_clear(err);
> err = SVN_NO_ERROR;
> }
> - else /* Got a real error -- one that stopped the commit */
> - {
> - /* ### todo: we should check whether it really was a conflict,
> - and return the conflict info if so? */
> + }
> + else
> + {
> + /* ### todo: we should check whether it really was a conflict,
> + and return the conflict info if so? */
>
> - /* If the commit failed, it's *probably* due to a conflict --
> - that is, the txn being out-of-date. The filesystem gives us
> - the ability to continue diddling the transaction and try
> - again; but let's face it: that's not how the cvs or svn works
> - from a user interface standpoint. Thus we don't make use of
> - this fs feature (for now, at least.)
> -
> - So, in a nutshell: svn commits are an all-or-nothing deal.
> - Each commit creates a new fs txn which either succeeds or is
> - aborted completely. No second chances; the user simply
> - needs to update and commit again :)
> -
> - We ignore the possible error result from svn_fs_abort_txn();
> - it's more important to return the original error. */
> - svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
> - return svn_error_return(err);
> - }
> + /* If the commit failed, it's *probably* due to a conflict --
> + that is, the txn being out-of-date. The filesystem gives us
> + the ability to continue diddling the transaction and try
> + again; but let's face it: that's not how the cvs or svn works
> + from a user interface standpoint. Thus we don't make use of
> + this fs feature (for now, at least.)
> +
> + So, in a nutshell: svn commits are an all-or-nothing deal.
> + Each commit creates a new fs txn which either succeeds or is
> + aborted completely. No second chances; the user simply
> + needs to update and commit again :)
> +
> + We ignore the possible error result from svn_fs_abort_txn();
> + it's more important to return the original error. */
> + svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
> + return svn_error_return(err);
Shouldn't we return a non-NULL error even if ERR is SVN_NO_ERROR here?
> }
>
> /* Pass new revision information to the caller's callback. */
>
> Modified: subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c?rev=1051778&r1=1051777&r2=1051778&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c Wed Dec 22 07:10:05 2010
> @@ -840,7 +840,18 @@ close_revision(void *baton)
> }
>
> /* Commit. */
> - if ((err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool)))
> + err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool);
> + if (SVN_IS_VALID_REVNUM(*new_rev))
> + {
> + if (err)
> + {
> + /* ### Log any error, but better yet is to rev
> + ### close_revision()'s API to allow both new_rev and err
> + ### to be returned, see #3768. */
> + svn_error_clear(err);
> + }
> + }
> + else
> {
> svn_error_clear(svn_fs_abort_txn(rb->txn, rb->pool));
> if (conflict_msg)
Same question, though here we probably need to revv the API or use
some callback?
>
> Modified: subversion/trunk/subversion/mod_dav_svn/lock.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/lock.c?rev=1051778&r1=1051777&r2=1051778&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/lock.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/lock.c Wed Dec 22 07:10:05 2010
The remaining hunks do check for the case that *NEW_REV == -1
and ERR == SVN_NO_ERROR, look good.
Re: svn commit: r1051778 - in /subversion/trunk/subversion: libsvn_repos/commit.c
libsvn_repos/load-fs-vtable.c mod_dav_svn/lock.c mod_dav_svn/version.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/22/10 5:57 AM, Daniel Shahaf wrote:
> blair@apache.org wrote on Wed, Dec 22, 2010 at 07:10:06 -0000:
>> Author: blair
>> Date: Wed Dec 22 07:10:05 2010
>> New Revision: 1051778
>>
>> URL: http://svn.apache.org/viewvc?rev=1051778&view=rev
>> Log:
>> Have all remaining calls of svn_fs_commit_txn() and
>> svn_repos_fs_commit_txn() use the contract that a commit was
>> successful if the returned revision is a valid revision number. The
>> returned error, if any, is no longer used as an indication of commit
>> success.
>> + /* If the commit failed, it's *probably* due to a conflict --
>> + that is, the txn being out-of-date. The filesystem gives us
>> + the ability to continue diddling the transaction and try
>> + again; but let's face it: that's not how the cvs or svn works
>> + from a user interface standpoint. Thus we don't make use of
>> + this fs feature (for now, at least.)
>> +
>> + So, in a nutshell: svn commits are an all-or-nothing deal.
>> + Each commit creates a new fs txn which either succeeds or is
>> + aborted completely. No second chances; the user simply
>> + needs to update and commit again :)
>> +
>> + We ignore the possible error result from svn_fs_abort_txn();
>> + it's more important to return the original error. */
>> + svn_error_clear(svn_fs_abort_txn(eb->txn, pool));
>> + return svn_error_return(err);
>
> Shouldn't we return a non-NULL error even if ERR is SVN_NO_ERROR here?
If the commit fails, then there should always be an error. The documentation
for svn_fs_txn_commit() and svn_repos_fs_txn_commit() isn't too clear on this,
so I'll add a note. But all the unit tests I added assert on this.
BTW, if the commit failed, then the checks that I added for SVN_NO_ERROR were
defensive to prevent core dumps because some code was dereferencing the error.
>> Modified: subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c?rev=1051778&r1=1051777&r2=1051778&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c (original)
>> +++ subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c Wed Dec 22 07:10:05 2010
>> @@ -840,7 +840,18 @@ close_revision(void *baton)
>> }
>>
>> /* Commit. */
>> - if ((err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool)))
>> + err = svn_fs_commit_txn(&conflict_msg, new_rev, rb->txn, rb->pool);
>> + if (SVN_IS_VALID_REVNUM(*new_rev))
>> + {
>> + if (err)
>> + {
>> + /* ### Log any error, but better yet is to rev
>> + ### close_revision()'s API to allow both new_rev and err
>> + ### to be returned, see #3768. */
>> + svn_error_clear(err);
>> + }
>> + }
>> + else
>> {
>> svn_error_clear(svn_fs_abort_txn(rb->txn, rb->pool));
>> if (conflict_msg)
>
> Same question, though here we probably need to revv the API or use
> some callback?
Yes, this goes to the other thread about the API changes we're discussing.
>> Modified: subversion/trunk/subversion/mod_dav_svn/lock.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/lock.c?rev=1051778&r1=1051777&r2=1051778&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/mod_dav_svn/lock.c (original)
>> +++ subversion/trunk/subversion/mod_dav_svn/lock.c Wed Dec 22 07:10:05 2010
>
> The remaining hunks do check for the case that *NEW_REV == -1
> and ERR == SVN_NO_ERROR, look good.
Again, this case shouldn't ever happen, but I coded against it anyway.
Blair