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.
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. Or should the ordering have
SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED as the parent so callers just need to
check the parent error?
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. 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
