[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH] Re: ra_local doesn't report post-commit errors

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Fri, 16 Sep 2016 03:42:58 +0000

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.
]]]
Received on 2016-09-16 05:44:04 CEST

This is an archived mail posted to the Subversion Dev mailing list.