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
