On 4 April 2014 14:11, Bert Huijben <b...@qqmail.nl> wrote: > > >> -----Original Message----- >> From: i...@apache.org [mailto:i...@apache.org] >> Sent: woensdag 2 april 2014 13:45 >> To: comm...@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.. > Strictly speaking we cannot make assumption that svn_fs_txn_name() never fails. Function should not have svn_error_t return value if we don't expect error to be returned, so I decided to check it to make code more clear.
> If you switched the entire function to a central cleanup handler I would > agree that this one should use the same pattern. > Yes, I've switched entire function to abort transaction on any error after transaction was created. >> >> /* 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. > Thanks, fixed in r1584597. -- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
