On 12/22/10 5:57 AM, Daniel Shahaf wrote:
> blair_at_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
Received on 2010-12-22 21:24:39 CET