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:
> Add a private function that takes the error returned from
> svn_repos_fs_commit_txn() and builds a error message string containing
> either or both of the svn_fs_commit_txn() error and the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped post-commit error. The
> function skips over tracing errors.
>
> * subversion/include/private/svn_repos_private.h
> (svn_repos__post_commit_error_str):
> New private function.
>
> * subversion/libsvn_repos/commit.c
> (svn_repos__post_commit_error_str):
> Implement.
> (close_edit):
> Use svn_repos__post_commit_error_str() instead of processing the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error.
>
> * subversion/mod_dav_svn/version.c
> (merge):
> Use svn_repos__post_commit_error_str() instead of processing the
> SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error.
>
> Modified:
> subversion/trunk/subversion/include/private/svn_repos_private.h
> subversion/trunk/subversion/libsvn_repos/commit.c
> subversion/trunk/subversion/mod_dav_svn/version.c
>
> Modified: subversion/trunk/subversion/include/private/svn_repos_private.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_repos_private.h?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/private/svn_repos_private.h (original)
> +++ subversion/trunk/subversion/include/private/svn_repos_private.h Wed Dec 22 05:46:45 2010
> @@ -84,6 +84,25 @@ svn_repos__validate_prop(const char *nam
> const svn_string_t *value,
> apr_pool_t *pool);
>
> +/**
> + * Given the error @a err from svn_repos_fs_commit_txn(), return an
> + * string containing either or both of the svn_fs_commit_txn() error
> + * and the SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error from
> + * the post-commit hook. Any error tracing placeholders in the error
> + * chain are skipped over.
> + *
> + * ### This method should not be necessary, but there are a few
> + * ### places, e.g. mod_dav_svn, where only a single error message
> + * ### string is returned to the caller and it is useful to have both
> + * ### error messages included in the message.
> + *
> + * Use @a pool to allocate the string in.
> + *
> + * @since New in 1.7.
> + */
> +const char *
> +svn_repos__post_commit_error_str(svn_error_t *err,
> + apr_pool_t *pool);
>
> #ifdef __cplusplus
> }
>
> 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).
> + 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?
> + /* 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.
> + else
> + return "(no error message)";
> + }
> +}
>
> static svn_error_t *
> close_edit(void *edit_baton,
> @@ -675,17 +724,7 @@ close_edit(void *edit_baton,
> succeeded. In which case, save the post-commit warning
> (to be reported back to the client, who will probably
> display it as a warning) and clear the error. */
> - if (err->child && err->child->message)
> - {
> - svn_error_t *warning_err = err->child;
> -#ifdef SVN_ERR__TRACING
> - /* Skip over any trace records. */
> - while (warning_err->message != NULL
> - && strcmp(warning_err->message, SVN_ERR__TRACED) == 0)
> - warning_err = warning_err->child;
> -#endif
> - post_commit_err = apr_pstrdup(pool, warning_err->message);
> - }
> + post_commit_err = svn_repos__post_commit_error_str(err, pool);
>
> svn_error_clear(err);
> err = SVN_NO_ERROR;
>
> Modified: subversion/trunk/subversion/mod_dav_svn/version.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/version.c?rev=1051763&r1=1051762&r2=1051763&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/mod_dav_svn/version.c (original)
> +++ subversion/trunk/subversion/mod_dav_svn/version.c Wed Dec 22 05:46:45 2010
> @@ -37,6 +37,7 @@
> #include "svn_props.h"
> #include "svn_dav.h"
> #include "svn_base64.h"
> +#include "private/svn_repos_private.h"
> #include "private/svn_dav_protocol.h"
> #include "private/svn_log.h"
>
> @@ -1407,9 +1408,7 @@ merge(dav_resource *target,
> }
> else if (serr)
> {
> - serr = svn_error_purge_tracing(serr);
> - if (serr->child && serr->child->message)
> - post_commit_err = apr_pstrdup(pool, serr->child->message);
> + post_commit_err = svn_repos__post_commit_error_str(serr, pool);
> svn_error_clear(serr);
> }
>
>
>
Received on 2010-12-22 14:50:27 CET