> -----Original Message-----
> From: ivan_at_apache.org [mailto:ivan_at_apache.org]
> Sent: woensdag 2 april 2014 13:45
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1583977 - in /subversion/trunk/subversion:
> libsvn_repos/fs-wrap.c tests/cmdline/commit_tests.py
>
> Author: ivan
> Date: Wed Apr 2 11:45:06 2014
> New Revision: 1583977
>
> URL: http://svn.apache.org/r1583977
> Log:
> Do not leave dead transaction if commit is blocked by start-commit hook.
> Also
> fix svn_repos_fs_begin_txn_for_commit2() API promise regression that was
> broken in r1376201.
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_begin_txn_for_commit2): Abort created transaction if error
> happens between transaction creation and successful return from
> function as promised in docstring. Also keep output *TXN_P
> parameter unaffected on failure as promised in docstring.
>
> * subversion/tests/cmdline/commit_tests.py
> (start_commit_hook_test): Verify that there is no dead transaction left
> when commit was blocked by start-commit hook.
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/fs-wrap.c
> subversion/trunk/subversion/tests/cmdline/commit_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_repos/fs-wrap.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/fs
> -wrap.c?rev=1583977&r1=1583976&r2=1583977&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_repos/fs-wrap.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/fs-wrap.c Wed Apr 2
> 11:45:06 2014
> @@ -136,6 +136,8 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
> 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 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
>
> /* 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));
Just noting: While I see the reason for the other hunks, I can't really see why this one is necessary. I don't think there can be cases where obtaining a string from ram fails and deleting something based on that same name fails..
If you switched the entire function to a central cleanup handler I would agree that this one should use the same pattern.
>
> /* 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;
^^ small typo.
<snip>
Bert
Received on 2014-04-04 12:12:24 CEST