On 12/22/10 5:57 AM, Daniel Shahaf wrote:
bl...@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
