On 12/22/10 5:47 AM, Daniel Shahaf wrote:
> blair_at_apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -0000:
>> Author: blair
>> Date: Wed Dec 22 05:46:45 2010
>> New Revision: 1051763
>>
>> URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
>> Log:
>> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
>> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
>> @@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
>> name, value, pool);
>> }
>>
>> +const char *
>> +svn_repos__post_commit_error_str(svn_error_t *err,
>> + apr_pool_t *pool)
>> +{
>> + svn_error_t *hook_err1, *hook_err2;
>> +
>> + if (! err)
>> + return "(no error)";
>> +
>> + err = svn_error_purge_tracing(err);
>>
>
> This modifies the provided error. Either the doc string should warn
> that this is being done (i.e., the provided ERR is not safe for use
> after calling this helper), or this call should be removed and another
> means used to locate the first non-tracing error message.
>
> Where do you clear ERR? You must do so in this function, since the
> caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
> promise).
Is this promise a defensive promise, as the original err still looks usable by
reviewing svn_error_purge_tracing()'s implementation? It looks like only one of
the errors should ever be cleared and it doesn't matter which one. True, it's
conceptually easier to remember that one should only clear the error returned
from svn_error_purge_tracing().
This function is meant to consume the error, so I've updated the docs to say
that it shouldn't be used after it and clear it.
r1051978.
>> + hook_err1 = svn_error_find_cause(err, SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED);
>> + if (hook_err1&& hook_err1->child)
>> + hook_err2 = hook_err1->child;
>> + else
>> + hook_err2 = hook_err1;
>> +
>
> So, ERR is the svn_fs_commit_txn() error (which has the hook error
> somewhere down the chain), and HOOK_ERR1 is the post-commit hook error?
Yes, and HOOK_ERR2 is the un-wrapped error from the hook script.
>> + /* This implementation counts on svn_repos_fs_commit_txn() returning
>> + svn_fs_commit_txn() as the parent error with a child
>> + SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED error. If the parent error
>> + is SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED then there was no error
>> + in svn_fs_commit_txn(). */
>> + if (hook_err1)
>> + {
>> + if (err == hook_err1)
>> + {
>> + if (hook_err2->message)
>> + return apr_pstrdup(pool, hook_err2->message);
>> + else
>> + return "(no error message)";
>> + }
>> + else
>> + {
>> + return apr_psprintf(pool,
>> + "Post commit processing had error and '%s' "
>
> s/and '%s'/'%s' and/
>
>> + "post-commit hook had error '%s'.",
>> + err->message ? err->message
>> + : "(no error message)",
>> + hook_err2->message ? hook_err2->message
>> + : "(no error message)");
>> + }
>> + }
>> + else
>> + {
>> + if (err->message)
>> + return apr_pstrdup(pool, err->message);
>
> In this case, and in the apr_pstrdup() above, might it be useful to say
> explicitly whether the given error is from the post-commit hook or from
> some FS post-commit fiddling? You've skipped over the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED link, so the "post-commit hook
> failed" message that used to be there would be gone.
Fixes in r1051988.
Should we have an official term for FS fiddling, or call it post-commit
processing? So we'll have to distinguish between "hook" and "processing".
Blair
Received on 2010-12-22 18:03:19 CET