Greg Stein wrote on Fri, Apr 27, 2012 at 17:33:18 -0400:
> I'm growing weary of this.
>
> Daniel and I spent a couple hours discussing calling patterns, error
> states, and everything. This felt the best approach.
>
> If you have a concrete suggestion, and a patch, then I'll take a look.
>
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1331716)
+++ subversion/include/svn_fs.h (working copy)
@@ -1090,19 +1090,19 @@ svn_fs_editor_create_for(svn_editor_t **editor,
* reason, then @a *revision will be set to #SVN_INVALID_REVNUM.
*
* If a conflict occurs during the commit, then @a *conflict_path will
- * be set to a path that caused the conflict. #SVN_NO_ERROR will be returned.
- * Callers may want to construct an #SVN_ERR_FS_CONFLICT error with a
- * message that incorporates @a *conflict_path.
+ * be set to a path that caused the conflict, and @a *copc_err will be
+ * be set to an error chain describing that error in a human-readable manner.
+ * #SVN_NO_ERROR will be returned.
*
* If a non-conflict error occurs during the commit, then that error will
* be returned.
- * As is standard with any Subversion API, @a revision, @a post_commit_err,
+ * As is standard with any Subversion API, @a revision, @a copc_err,
* and @a conflict_path (the OUT parameters) have an indeterminate value if
* an error is returned.
*
* If the commit completes (and a revision is returned in @a *revision), then
* it is still possible for an error to occur during the cleanup process.
- * Any such error will be returned in @a *post_commit_err. The caller must
+ * Any such error will be returned in @a *copc_err. The caller must
* properly use or clear that error.
*
* If svn_editor_complete() has already been called on @a editor, then
@@ -1117,15 +1117,16 @@ svn_fs_editor_create_for(svn_editor_t **editor,
* @a scratch_pool will be used for all temporary allocations.
*
* @note To summarize, there are three possible outcomes of this function:
- * successful commit (with or without an associated @a *post_commit_err);
- * failed commit due to a conflict (reported via @a *conflict_path); and
- * failed commit for some other reason (reported via the returned error.)
+ * successful commit (with or without an associated @a *copc_err);
+ * failed commit due to a conflict (reported via both @a *conflict_path
+ * and @a *copc_err); and failed commit for some other reason (reported via
+ * the returned error, with undefined values for the OUT parameters.)
*
* @since New in 1.8.
*/
svn_error_t *
svn_fs_editor_commit(svn_revnum_t *revision,
- svn_error_t **post_commit_err,
+ svn_error_t **copc_err, /* conflict or post-commit */
const char **conflict_path,
svn_editor_t *editor,
apr_pool_t *result_pool,
Index: subversion/libsvn_fs/editor.c
===================================================================
--- subversion/libsvn_fs/editor.c (revision 1331716)
+++ subversion/libsvn_fs/editor.c (working copy)
@@ -450,7 +450,7 @@ svn_fs_editor_create_for(svn_editor_t **editor,
svn_error_t *
svn_fs_editor_commit(svn_revnum_t *revision,
- svn_error_t **post_commit_err,
+ svn_error_t **copc_err,
const char **conflict_path,
svn_editor_t *editor,
apr_pool_t *result_pool,
@@ -466,7 +466,7 @@ svn_fs_editor_commit(svn_revnum_t *revision,
NULL, NULL);
*revision = SVN_INVALID_REVNUM;
- *post_commit_err = NULL;
+ *copc_err = NULL;
*conflict_path = NULL;
/* Clean up internal resources (eg. eb->root). This also allows the
@@ -489,7 +489,7 @@ svn_fs_editor_commit(svn_revnum_t *revision,
/* Case 3. ERR is a post-commit (cleanup) error. */
/* Pass responsibility via POST_COMMIT_ERR. */
- *post_commit_err = err;
+ *copc_err = err;
err = SVN_NO_ERROR;
}
/* else: Case 1. */
@@ -504,9 +504,8 @@ svn_fs_editor_commit(svn_revnum_t *revision,
/* Copy this into the correct pool (see note above). */
*conflict_path = apr_pstrdup(result_pool, inner_conflict_path);
- /* Return sucess. The caller should inspect CONFLICT_PATH to
- determine this particular case. */
- svn_error_clear(err);
+ /* Pass responsibility via POST_COMMIT_ERR. */
+ *copc_err = err;
err = SVN_NO_ERROR;
}
/* else: Case 4. */
Index: subversion/libsvn_repos/commit.c
===================================================================
--- subversion/libsvn_repos/commit.c (revision 1331716)
+++ subversion/libsvn_repos/commit.c (working copy)
@@ -1214,6 +1214,7 @@ complete_cb(void *baton,
{
struct ev2_baton *eb = baton;
svn_revnum_t revision;
+ svn_error_t *copc_err;
svn_error_t *post_commit_err;
const char *conflict_path;
svn_error_t *err;
@@ -1224,18 +1225,19 @@ complete_cb(void *baton,
SVN_ERR(svn_repos__hooks_pre_commit(eb->repos, eb->txn_name, scratch_pool));
/* Hook is done. Let's do the actual commit. */
- SVN_ERR(svn_fs_editor_commit(&revision, &post_commit_err, &conflict_path,
+ SVN_ERR(svn_fs_editor_commit(&revision, &copc_err, &conflict_path,
eb->inner, scratch_pool, scratch_pool));
/* Did a conflict occur during the commit process? */
if (conflict_path != NULL)
- return svn_error_createf(SVN_ERR_FS_CONFLICT, NULL,
+ return svn_error_createf(SVN_ERR_FS_CONFLICT, copc_err,,
_("Conflict at '%s'"), conflict_path);
/* Since did not receive an error during the commit process, and no
conflict was specified... we committed a revision. Run the hooks.
Other errors may have occurred within the FS (specified by the
POST_COMMIT_ERR localvar), but we need to run the hooks. */
+ post_commit_err = copc_err;
SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(revision));
err = svn_repos__hooks_post_commit(eb->repos, revision, eb->txn_name,
scratch_pool);
