[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Wed, 22 Dec 2010 15:57:02 +0200

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.
>
> * 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.
Received on 2010-12-22 14:59:59 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.