Re: svn commit: r1583977 - in /subversion/trunk/subversion: libsvn_repos/fs-wrap.c tests/cmdline/commit_tests.py

On 4 April 2014 14:11, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----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..
>
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