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. > > * 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.
