Daniel Shahaf wrote on Fri, Sep 09, 2016 at 06:44:51 +0000:
> 3. Add to svn_commit_info_t an svn_error_t representation of the error
> chain that svn_commit_callback_t::post_commit_err summarizes,
Patch attached.
The new test fails over ra_svn; I haven't investigated that yet.
Does the approach look sane? This has more moving parts than a normal
add-member-to-struct patch because the new member is an svn_error_t so
it needs explicit clearing.
Thanks,
Daniel
P.S. How to run make check with an out-of-tree build? I couldn't get
the python tests to run in that configuration.
[[[
ra_local: Report post-commit hook and post-commit FS processing errors.
### TODO: fix commit_tests 38 over ra_svn
To this end, add an svn_error_t member to svn_commit_info_t to semi-deprecate
the existing string one.
A simpler fix would have been to get ra_plugin.c to wrap
svn_commit_info_t::post_commit_err (a string) in an svn_error_t object
and add that to its return value. However, that approach would not have
preserved the original apr_err error codes.
* subversion/tests/cmdline/commit_tests.py
(post_commit_hook_test): Expect non-empty stderr.
* subversion/include/svn_types.h
(svn_commit_info_t::post_commit_err2):
New member. Add public and internal dcumentation.
(svn_commit_info_t::post_commit_err):
Update documentation to point to new member.
* subversion/libsvn_subr/types.c
(error_clear): New.
(svn_create_commit_info, svn_commit_info_dup): Arrange for the error member
to be cleared.
* subversion/libsvn_ra_svn/protocol
('commit-info' response to 'commit' command):
* subversion/mod_dav_svn/merge.c
(dav_svn__merge_response):
Add a TODO to marshal the new svn_commit_info_t member.
* subversion/libsvn_repos/commit.c
(invoke_commit_cb): Change signature. Populate new svn_commit_info_t member.
(close_edit, complete_cb): Track signature change.
* subversion/libsvn_ra_local/ra_plugin.c
(deltify_etc): Report the post-commit error to the caller.
]]]
[[[
Index: subversion/include/svn_types.h
===================================================================
--- subversion/include/svn_types.h (revision 1760921)
+++ subversion/include/svn_types.h (working copy)
@@ -784,7 +784,10 @@ typedef struct svn_commit_info_t
/** author of the commit. */
const char *author;
- /** error message from post-commit hook, or NULL. */
+ /** An error message summarizing #post_commit_err2. This is more detailed
+ * than svn_err_best_message() when #post_commit_err2 represents several
+ * different errors. */
+ /* See svn_repos__post_commit_error_str() */
const char *post_commit_err;
/** repository root, may be @c NULL if unknown.
@@ -791,6 +794,31 @@ typedef struct svn_commit_info_t
@since New in 1.7. */
const char *repos_root;
+ /** Post-commit errors, if any.
+ *
+ * These can be from the post-commit hook or from post-commit FS processing.
+ * (This is not an exhaustive list.)
+ *
+ * @note #svn_commit_callback2_t implementations must not clear this error
+ * object. (This is because the callback type predates this struct member:
+ * for compatibility, the error object is cleared by the core library that
+ * invokes the callback.) They may neither modify the pointer's value.
+ *
+ * @since New in 1.10. */
+#ifdef DOXYGEN
+ /* The restrictions in the note are because the implementation installs
+ * a pool cleanup handler that clears the error. It does so because pre-1.10
+ * svn_commit_callback2_t implementations are not aware of the svn_error_t
+ * member and won't clear it. A pool cleanup is required in case the
+ * callback implementation dup()s this struct, to avoid error leaks. The
+ * non-dup() constructor doesn't have to use a pool cleanup, but presently it
+ * does.
+ */
+ svn_error_t *const post_commit_err2;
+#else
+ svn_error_t *post_commit_err2;
+#endif
+
} svn_commit_info_t;
/**
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 1760921)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -403,6 +403,7 @@ deltify_etc(const svn_commit_info_t *commit_info,
apr_pool_t *scratch_pool)
{
struct deltify_etc_baton *deb = baton;
+ svn_error_t *err0 = svn_error_dup(commit_info->post_commit_err2);
svn_error_t *err1 = SVN_NO_ERROR;
svn_error_t *err2;
@@ -445,7 +446,7 @@ deltify_etc(const svn_commit_info_t *commit_info,
deltification. */
err2 = svn_fs_deltify_revision(deb->fs, commit_info->revision, scratch_pool);
- return svn_error_compose_create(err1, err2);
+ return svn_error_compose_create(err0, svn_error_compose_create(err1, err2));
}
Index: subversion/libsvn_ra_svn/protocol
===================================================================
--- subversion/libsvn_ra_svn/protocol (revision 1760921)
+++ subversion/libsvn_ra_svn/protocol (working copy)
@@ -300,6 +300,8 @@ second place for auth-request point as noted below
? ( post-commit-err:string ) )
NOTE: when revving this, make 'logmsg' optional, or delete that parameter
and have the log message specified in 'rev-props'.
+ NOTE: when revving this, upgrade 'post-commit-err' to a full error chain,
+ following svn_commit_info_t::post_commit_err2.
get-file
params: ( path:string [ rev:number ] want-props:bool want-contents:bool
Index: subversion/libsvn_repos/commit.c
===================================================================
--- subversion/libsvn_repos/commit.c (revision 1760921)
+++ subversion/libsvn_repos/commit.c (working copy)
@@ -203,7 +203,7 @@ invoke_commit_cb(svn_commit_callback2_t commit_cb,
void *commit_baton,
svn_fs_t *fs,
svn_revnum_t revision,
- const char *post_commit_errstr,
+ svn_error_t *post_commit_err,
apr_pool_t *scratch_pool)
{
/* FS interface returns non-const values. */
@@ -227,7 +227,12 @@ invoke_commit_cb(svn_commit_callback2_t commit_cb,
commit_info->revision = revision;
commit_info->date = date ? date->data : NULL;
commit_info->author = author ? author->data : NULL;
- commit_info->post_commit_err = post_commit_errstr;
+ if (post_commit_err)
+ {
+ commit_info->post_commit_err
+ = svn_repos__post_commit_error_str(post_commit_err, scratch_pool);
+ commit_info->post_commit_err2 = post_commit_err;
+ }
/* commit_info->repos_root is not set by the repos layer, only by RA layers
*/
return svn_error_trace(commit_cb(commit_info, commit_baton, scratch_pool));
@@ -800,7 +805,7 @@ close_edit(void *edit_baton,
svn_revnum_t new_revision = SVN_INVALID_REVNUM;
svn_error_t *err;
const char *conflict;
- const char *post_commit_err = NULL;
+ svn_error_t *post_commit_err;
/* If no transaction has been created (ie. if open_root wasn't
called before close_edit), abort the operation here with an
@@ -822,15 +827,7 @@ close_edit(void *edit_baton,
if (eb->txn_root)
svn_fs_close_root(eb->txn_root);
- if (err)
- {
- /* If the error was in post-commit, then the commit itself
- 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. */
- post_commit_err = svn_repos__post_commit_error_str(err, pool);
- svn_error_clear(err);
- }
+ post_commit_err = err;
/* Make sure a future abort doesn't perform
any work. This may occur if the commit
@@ -1281,7 +1278,6 @@ complete_cb(void *baton,
svn_error_t *post_commit_err;
const char *conflict_path;
svn_error_t *err;
- const char *post_commit_errstr;
apr_hash_t *hooks_env;
/* Parse the hooks-env file (if any). */
@@ -1313,21 +1309,12 @@ complete_cb(void *baton,
err = svn_error_create(SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
_("Commit succeeded, but post-commit hook failed"));
- /* Combine the FS errors with the hook errors, and stringify. */
- err = svn_error_compose_create(post_commit_err, err);
- if (err)
- {
- post_commit_errstr = svn_repos__post_commit_error_str(err, scratch_pool);
- svn_error_clear(err);
- }
- else
- {
- post_commit_errstr = NULL;
- }
+ /* Combine the FS errors with the hook errors. */
+ post_commit_err = svn_error_compose_create(post_commit_err, err);
return svn_error_trace(invoke_commit_cb(eb->commit_cb, eb->commit_baton,
eb->repos->fs, revision,
- post_commit_errstr,
+ post_commit_err,
scratch_pool));
}
Index: subversion/libsvn_subr/types.c
===================================================================
--- subversion/libsvn_subr/types.c (revision 1760921)
+++ subversion/libsvn_subr/types.c (working copy)
@@ -207,6 +207,18 @@ svn_tristate__from_word(const char *word)
return svn_tristate_unknown;
}
+/* Pool cleanup handler for svn_commit_info_dup(), which see for details.
+ *
+ * This function implements apr_pool_cleanup_register()'s anonymous callback
+ * type. */
+static apr_status_t
+error_clear(void *pool_cleanup_baton)
+{
+ svn_commit_info_t *info = pool_cleanup_baton;
+ svn_error_clear(info->post_commit_err2);
+ return APR_SUCCESS;
+}
+
svn_commit_info_t *
svn_create_commit_info(apr_pool_t *pool)
{
@@ -216,6 +228,11 @@ svn_create_commit_info(apr_pool_t *pool)
commit_info->revision = SVN_INVALID_REVNUM;
/* All other fields were initialized to NULL above. */
+ /* The struct type did not have an svn_error_t in its first public version,
+ * so we must outselves arrange for it to be cleared. */
+ apr_pool_cleanup_register(pool, commit_info, error_clear,
+ apr_pool_cleanup_null);
+
return commit_info;
}
@@ -235,7 +252,14 @@ svn_commit_info_dup(const svn_commit_info_t *src_c
? apr_pstrdup(pool, src_commit_info->post_commit_err) : NULL;
dst_commit_info->repos_root = src_commit_info->repos_root
? apr_pstrdup(pool, src_commit_info->repos_root) : NULL;
+ dst_commit_info->post_commit_err2 = src_commit_info->post_commit_err2
+ ? svn_error_dup(src_commit_info->post_commit_err2) : SVN_NO_ERROR;
+ /* The struct type did not have an svn_error_t in its first public version,
+ * so we must outselves arrange for it to be cleared. */
+ apr_pool_cleanup_register(pool, dst_commit_info, error_clear,
+ apr_pool_cleanup_null);
+
return dst_commit_info;
}
Index: subversion/mod_dav_svn/merge.c
===================================================================
--- subversion/mod_dav_svn/merge.c (revision 1760921)
+++ subversion/mod_dav_svn/merge.c (working copy)
@@ -259,6 +259,9 @@ dav_svn__merge_response(dav_svn__output *output,
post_commit_header_info = apr_psprintf(pool,
" xmlns:S=\"%s\"",
SVN_XML_NAMESPACE);
+ /* ### TODO: marshal a complete error chain rather than just an error
+ * ### string, following svn_commit_info_t::post_commit_err2.
+ */
post_commit_err_elem = apr_psprintf(pool,
"<S:post-commit-err>%s"
"</S:post-commit-err>",
Index: subversion/tests/cmdline/commit_tests.py
===================================================================
--- subversion/tests/cmdline/commit_tests.py (revision 1760921)
+++ subversion/tests/cmdline/commit_tests.py (working copy)
@@ -2109,10 +2109,22 @@ def post_commit_hook_test(sbox):
svntest.actions.hook_failure_message('post-commit'),
error_msg + "\n",
]
+ expected_error = 'svn: E165001: ' + \
+ svntest.actions.hook_failure_message('post-commit')
+ # 'svn: E165001: post-commit hook failed (exit code 1) with output:\n'
- svntest.actions.run_and_verify_svn(expected_output, [],
- 'ci', '-m', 'log msg', iota_path)
+ _, _, err = \
+ svntest.actions.run_and_verify_svn(expected_output,
svntest.verify.AnyOutput,
+ 'ci', '-m', 'log msg', iota_path)
+ if expected_error not in err:
+ raise svntest.Failure("expected stderr to include %r but it doesn't" %
+ (expected_error,))
+
+ if (error_msg+"\n") not in err:
+ raise svntest.Failure("expected stderr to include %r but it doesn't" %
+ (error_msg,))
+
#----------------------------------------------------------------------
# Commit two targets non-recursively, but both targets should be the
# same folder (in multiple variations). Test that svn handles this correctly.
]]]
