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

Re: svn_fs_commit_txn and svn_repos_fs_commit_txn inconsistency

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 21 Dec 2010 20:40:02 +0200

Blair Zajac wrote on Tue, Dec 21, 2010 at 10:16:56 -0800:
> On 12/20/10 11:32 AM, C. Michael Pilato wrote:
>> On 12/20/2010 02:14 PM, Blair Zajac wrote:
>>> Shouldn't svn_repos_fs_commit_txn() always run the post-commit hook if
>>> new_rev is a valid rev?
>> That does seem reasonable, yes.
> Looking through our code, no existing use of svn_fs_commit_txn() and
> svn_repos_fs_commit_txn() use SVN_IS_VALID_REVNUM(new_rev), the code
> checks for a non-NULL svn_error_t * and checks if the parent error is a
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED. It also doesn't scan the chain
> for that error.

svn_error_has_cause() could be used for that.

> Running this by the group before I commit, I think the following should be done.
> 1) In fs_fs.c's commit_body(), move the call to set *new_rev up, this
> matches the documented behavior and is technically the correct thing to
> do. If purging the txn fails, then the transaction was still committed
> and is visible to other processes.
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -6206,13 +6206,19 @@
> /* Update the 'current' file. */
> SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,
> start_copy_id, pool));
> +
> + /* At this point the new revision is committed and globally visible
> + so let the caller know it succeeded by giving it the new revision
> + number, which fullfils svn_fs_commit_txn() contract. Any errors
> + after this point do not change the fact that a new revision was
> + created. */
> + *cb->new_rev_p = new_rev;
> +
> ffd->youngest_rev_cache = new_rev;
> /* Remove this transaction directory. */
> SVN_ERR(svn_fs_fs__purge_txn(cb->fs, cb->txn->id, pool));
> - *cb->new_rev_p = new_rev;
> -
> return SVN_NO_ERROR;
> }
> Taking a quick look at BDB, it looks like it sets *new_rev immediately.
> 2) Each call to svn_fs_commit_txn() and svn_repos_fs_commit_txn() in our
> source should be updated to check for a valid new revision. If it needs
> to care about a hook error it can.
> 3) svn_repos_fs_commit_txn() should just check for a valid new revision
> to run the hook script.
> 4) In svn_repos_fs_commit_txn(), which order should errors be composed?
> svn_fs_commit_txn()'s error as the parent followed by the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error as a child? This seems to be
> the standard ordering of chained errors. On the other hand, it makes it
> harder to find a post-commit script error.

Actually, it will make it impossible to detect post-commit errors over
ra_dav, since that RA layer marshals only the outermost error code in an
error chain.

> Or should the ordering have
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the parent so callers just need
> to check the parent error?

Regardless of the ordering, I think it's fine to have
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the top-most error code,
since clients currently look for that specific error code.

But my intuition is that svn_fs_commit_txn()'s error should be first...

> It seems useful to add to svn_error_t the concept of starting a new chain
> of errors. Right now when we compose, we don't know when an unrelated
> error starts.

Agreed. However, I don't think of it as "start a new chain"; I think of
it as "two separate chains". (Possibly because of how we use
svn_error_compose_create() all the time.)

        /** Like svn_error_t, but with SECONDARY added. */
        struct svn_error2_t {
          apr_status_t apr_err;
          const char *message;
          struct svn_error2_t *child;
          apr_pool_t *pool;
          const char *file;
          long line;
          struct svn_error2_t *secondary;

> Actually, this suggests that we use the normal ordering of
> errors, svn_fs_commit_txn() as the parent with
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED's as the child, this way the error
> chain can be scanned and the unrelated error easily determined. In the
> reverse ordering, one cannot tell when svn_fs_commit_txn()'s error
> starts.
> Blair
Received on 2010-12-21 19:43:05 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.