On 22 August 2012 23:19, <cmpil...@apache.org> wrote:
> Author: cmpilato
> Date: Wed Aug 22 19:19:48 2012
> New Revision: 1376201
>
> URL: http://svn.apache.org/viewvc?rev=1376201&view=rev
> Log:
> Tweak the 'start-commit' hook to run *after* txn creation and initial
> revprop population, and to receive as a new command-line argument the
> txn-id (so that scripts can examine those revprops).
>
> * subversion/libsvn_repos/repos.h,
> * subversion/libsvn_repos/hooks.c
> (svn_repos__hooks_start_commit): Add 'txn_name' parameter, the value
> of which is passed as the new final argument to the 'start-commit'
> hook.
>
> * subversion/libsvn_repos/commit.c
> (svn_repos__get_commit_ev2): Update call to svn_repos__hooks_start_commit(),
> and now run it after setting txnprops.
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_begin_txn_for_commit2): Fetch the transaction name.
> Update call to svn_repos__hooks_start_commit(), and now run it
> after setting txnprops.
>
> * subversion/libsvn_repos/repos.c
> (create_hooks): Update 'start-commit' hook template text to reflect
> changes.
>
Hi Mike,
This commit broke several things:
1. The dead transaction will remain if start commit fails for any
reason, while it should not.
2. The docstring promises that *TXN_P unaffected in case of hook
failure, while it changes it. We rely on documented behavior in
open_root at libsvn_repos/commit.c:401
3. Docstring for svn_repos_fs_begin_txn_for_commit2() explicitly
states that start-commit hook run *before* transaction created:
[[[
* Before a txn is created, the repository's start-commit hooks are
* run; if any of them fail, no txn is created, @a *txn_p is unaffected,
* and #SVN_ERR_REPOS_HOOK_FAILURE is returned.
]]]
I doubt that our policy allow to change such API promise. Anyway this
code is already released, so we have to deal with this somehow.
I'm working on patch to solve (1) and (2), see attached file.
Btw I wonder why you decided to change existing hook behavior, instead
of create separate hook like 'post-start-commit' or 'validate-txn' to
give opportunity to validate client capabilities early as possible.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Index: subversion/libsvn_repos/fs-wrap.c
===================================================================
--- subversion/libsvn_repos/fs-wrap.c (revision 1577313)
+++ subversion/libsvn_repos/fs-wrap.c (working copy)
@@ -136,6 +136,8 @@
const char *txn_name;
svn_string_t *author = svn_hash_gets(revprop_table,
SVN_PROP_REVISION_AUTHOR);
apr_hash_t *hooks_env;
+ svn_error_t *err;
+ svn_fs_txn_t *txn;
/* Parse the hooks-env file (if any). */
SVN_ERR(svn_repos__parse_hooks_env(&hooks_env, repos->hooks_env_path,
@@ -143,21 +145,30 @@
/* Begin the transaction, ask for the fs to do on-the-fly lock checks.
We fetch its name, too, so the start-commit hook can use it. */
- SVN_ERR(svn_fs_begin_txn2(txn_p, repos->fs, rev,
+ SVN_ERR(svn_fs_begin_txn2(&txn, repos->fs, rev,
SVN_FS_TXN_CHECK_LOCKS, pool));
- SVN_ERR(svn_fs_txn_name(&txn_name, *txn_p, pool));
+ err = svn_fs_txn_name(&txn_name, txn, pool);
+ if (err)
+ return svn_error_compose_create(err, svn_fs_abort_txn(txn, pool));
/* We pass the revision properties to the filesystem by adding them
as properties on the txn. Later, when we commit the txn, these
properties will be copied into the newly created revision. */
revprops = svn_prop_hash_to_array(revprop_table, pool);
- SVN_ERR(svn_repos_fs_change_txn_props(*txn_p, revprops, pool));
+ err = svn_repos_fs_change_txn_props(txn, revprops, pool);
+ if (err)
+ return svn_error_compose_create(err, svn_fs_abort_txn(txn, pool));
/* Run start-commit hooks. */
- SVN_ERR(svn_repos__hooks_start_commit(repos, hooks_env,
- author ? author->data : NULL,
- repos->client_capabilities, txn_name,
- pool));
+ err = svn_repos__hooks_start_commit(repos, hooks_env,
+ author ? author->data : NULL,
+ repos->client_capabilities, txn_name,
+ pool);
+ if (err)
+ return svn_error_compose_create(err, svn_fs_abort_txn(txn, pool));
+
+ /* We have API promise that *TXN_P is unaffected on faulure. */
+ *txn_p = txn;
