Branko Čibej <br...@wandisco.com> writes:
> Go ahead; I won't be able to for the next few days. Please remember to
> remove the new error code, too, if we won't be using it any more.
With a bit of trial and error, I was able to come up with a complete and
tested solution that I find suitable in terms of the API design and the
calling site (svnadmin.c). The ideas are partly borrowed from the already
mentioned svn_fs_lock_many() and svn_fs_lock_callback_t implementations.
See the attached patch. Please note that the patch is not a continuation of
what I posted in my last e-mail (verify-fixup-squashed-v1.patch.txt), but from
my point of view is better and should address the raised concerns, such as
having a non-trivial compatibility wrapper.
Here is the log message:
[[[
Reimplement svn_repos_verify_fs3() to support an arbitrary callback that
receives the information about an encountered problem and lets the caller
decide on what happens next. This supersedes the keep_going argument for
this API. A callback is optional; the behavior of this API if the callback
is not provided is equivalent to how svn_repos_verify_fs2() deals with
encountered errors. This allows seamless migration to the new API, if the
callback is not necessary. The idea is partly taken from how our existing
svn_fs_lock_many() API works with a svn_fs_lock_callback_t and passes error
information to the caller.
Immediately use the new API to provide an alternative solution for the
encountered problem with 'svnadmin verify --keep-going -q' (see r1684940)
being useless in terms that it was only giving an indication of whether a
particular repository passes the verification or not, without providing a
root cause (details) of what's wrong.
Discussion can be found in http://svn.haxx.se/dev/archive-2015-05/0141.shtml
(Subject: "Possible incompatibility of svn_repos_verify_fs2() in 1.9.0-rc1")
* subversion/include/svn_error_codes.h
(SVN_ERR_REPOS_VERIFY_FAILED): Remove this error code, as we no longer
need to send a specific error from within svn_repos_verify_fs3().
(SVN_ERR_CL_REPOS_VERIFY_FAILED): New.
* subversion/include/svn_repos.h
(svn_repos_notify_action_t): Remove svn_repos_notify_failure.
(svn_repos_notify_t): Remove 'err' field, as it is no longer needed.
(svn_repos_verify_callback_t): New optional callback type to be used with
svn_repos_verify_fs3().
(svn_repos_verify_fs3): Drop 'keep_going' argument in favor of accepting a
svn_repos_verify_callback_t. Update the docstring accordingly.
(svn_repos_verify_fs3): Update the docstring for this deprecated function.
* subversion/libsvn_repos/deprecated.c
(svn_repos_verify_fs2): Update the call to svn_repos_verify_fs3() in this
compatibility wrapper. Don't pass the verify callback.
* subversion/libsvn_repos/dump.c
(notify_verification_error): Remove; this function is no longer required.
(report_error): New helper function.
(svn_repos_verify_fs3): In case we've got a svn_repos_verify_callback_t,
call it upon receiving an FS-specific structure failure or a revision
verification failure. Delegate this action to the new report_error()
helper function. Doing so makes the caller responsible for what's going
to happen with the error. The caller can choose to store the error,
ignore it or use it in any other necessary way. If a callback returns an
error, stop the verification process and immediately return that error.
If no callback is provided, mimic the behavior of svn_repos_verify_fs2()
and return the first encountered error. Drop the logic related to error
formatting, as we no longer need it at this layer. We are going to make
a simpler replacement for it is the UI code (svnadmin.c), where it is
supposed to live.
* subversion/svnadmin/svnadmin.c
(struct repos_verify_callback_baton): New. Contains the fields that are
required to track the --keep-going errors taken from ...
(struct repos_notify_handler_baton): ...this baton. After the previous
step, this baton only contains the 'feedback_stream' field, so inline it
into every calling site.
(repos_notify_handler): Baton is now simply an svn_stream_t. Remove the
boolean-based filtering logic from this handler and drop the handling of
svn_repos_notify_failure. The latter is moved, with a bit of tweaking,
into ...
(repos_verify_callback): ...this new function, that implements a callback
for svn_repos_verify_fs3(). Depending on whether we are in --keep-going
mode or not, either dump the failure details to stderr and track them to
produce a summary, or immediately return it through the callback, thus
ending the verification process. Remember all errors in the --keep-going
mode, not only those that are associated with a particular revision.
Prior to handling the error itself, tell that we failed to verify the
revision or metadata by writing corresponding messages to stderr.
(subcommand_dump, subcommand_load, subcommand_recover, subcommand_upgrade,
subcommand_hotcopy, subcommand_pack): Inline repos_notify_handler_baton
here, as it now contains a single svn_stream_t field.
(subcommand_verify): Inline repos_notify_handler_baton here, as it now
contains a single svn_stream_t field. Avoid manipulations with boolean
fields like b->silent_errors and b->silent_running, because we no longer
need them, and the fields themselves are gone. Create a feedback stream
only in non-quiet mode, as we do in other subcommand implementations.
Create a baton for repos_verify_callback() and adjust the calling site of
svn_repos_verify_fs3(), that now needs a callback. Adjust --keep-going
summary printing to the new approach with the verification callback.
Finally, provide a simple error if we encountered at least one failure
in the --keep-going mode.
* subversion/tests/cmdline/svnadmin_tests.py
(verify_keep_going, verify_keep_going_quiet, verify_invalid_path_changes):
Adjust the expectations, because now errors go straight to stderr in both
--keep-going and ordinary modes. Where possible, make the expectations a
bit stricter by extending the lines that we check with RegexListOutput().
* subversion/tests/libsvn_fs_fs/fs-fs-private-test.c
(load_index, load_index_keep_going): Squash two tests into one; basically,
undo the corresponding hunk from r1683311. As we no longer have separate
keep_going mode in svn_repos_verify_fs3(), and the caller decides if the
verification continues or not, we don't have to check two different
scenarios.
(test_funcs): Track the test changes.
* subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
(fuzzing_1_byte_1_rev): Adjust the call to svn_repos_verify_fs3().
]]]
What do you think?
Regards,
Evgeny Kotkov
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 1687456)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -922,11 +922,6 @@ SVN_ERROR_START
SVN_ERR_REPOS_CATEGORY_START + 10,
"Repository upgrade is not supported")
- /** @since New in 1.9. */
- SVN_ERRDEF(SVN_ERR_REPOS_VERIFY_FAILED,
- SVN_ERR_REPOS_CATEGORY_START + 11,
- "Repository verification failed")
-
/* generic RA errors */
SVN_ERRDEF(SVN_ERR_RA_ILLEGAL_URL,
@@ -1497,6 +1492,11 @@ SVN_ERROR_START
SVN_ERR_CL_CATEGORY_START + 11,
"Failed processing one or more externals definitions")
+ /** @since New in 1.9. */
+ SVN_ERRDEF(SVN_ERR_CL_REPOS_VERIFY_FAILED,
+ SVN_ERR_CL_CATEGORY_START + 12,
+ "Repository verification failed")
+
/* ra_svn errors */
SVN_ERRDEF(SVN_ERR_RA_SVN_CMD_ERR,
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1687456)
+++ subversion/include/svn_repos.h (working copy)
@@ -234,9 +234,6 @@ typedef enum svn_repos_notify_action_t
/** The structure of a revision is being verified. @since New in 1.8. */
svn_repos_notify_verify_rev_structure,
- /** A revision is found with corruption/errors. @since New in 1.9. */
- svn_repos_notify_failure,
-
/** A revprop shard got packed. @since New in 1.9. */
svn_repos_notify_pack_revprops,
@@ -348,11 +345,6 @@ typedef struct svn_repos_notify_t
/** For #svn_repos_notify_load_node_start, the path of the node. */
const char *path;
- /** For #svn_repos_notify_failure, this error chain indicates what
- went wrong during verification.
- @since New in 1.9. */
- svn_error_t *err;
-
/** For #svn_repos_notify_hotcopy_rev_range, the start of the copied
revision range.
@since New in 1.9. */
@@ -2825,6 +2817,22 @@ enum svn_repos_load_uuid
svn_repos_load_uuid_force
};
+/** Callback type for use with svn_repos_verify_fs3(). @a revision
+ * and @a verify_err are the details of a single verification failure
+ * that occurred during the svn_repos_verify_fs3() call. @a baton is
+ * the same baton given to svn_repos_verify_fs3(). @a scratch_pool is
+ * provided for the convenience of the implementor, who should not
+ * expect it to live longer than a single callback call.
+ *
+ * @see svn_repos_verify_fs3
+ *
+ * @since New in 1.9.
+ */
+typedef svn_error_t *(*svn_repos_verify_callback_t)(void *baton,
+ svn_revnum_t revision,
+ svn_error_t *verify_err,
+ apr_pool_t *scratch_pool);
+
/**
* Verify the contents of the file system in @a repos.
*
@@ -2835,9 +2843,6 @@ enum svn_repos_load_uuid
* range, then also verify "global invariants" of the repository, as
* described in svn_fs_verify().
*
- * When a failure is found, if @a keep_going is @c TRUE then continue
- * verification from the next revision, otherwise stop.
- *
* If @a check_normalization is @c TRUE, report any name collisions
* within the same directory or svn:mergeinfo property where the names
* differ only in character representation, but are otherwise
@@ -2848,25 +2853,31 @@ enum svn_repos_load_uuid
* file context reconstruction and verification. For FSFS format 7+ and
* FSX, this allows for a very fast check against external corruption.
*
+ * If @a verify_callback is not @c NULL, call it with @a verify_baton upon
+ * receiving an FS-specific structure failure or a revision verification
+ * failure. Set @c revision callback argument to #SVN_INVALID_REVNUM or
+ * to the revision number respectively. Set @c verify_err to svn_error_t
+ * describing the reason of the failure. @c verify_err will be cleared
+ * after the callback returns, use svn_error_dup() to preserve the error.
+ * If @a verify_callback returns an error different from #SVN_NO_ERROR,
+ * stop verifying the repository and immediately return the error from
+ * @a verify_callback.
+ *
+ * If @a verify_callback is @c NULL, this function returns the first
+ * encountered verification error or #SVN_NO_ERROR if there were no failures
+ * during the verification. Errors that prevent the verification process
+ * from continuing, such as #SVN_ERR_CANCELLED, are returned immediately
+ * and do not trigger an invocation of @a verify_callback.
+ *
* If @a notify_func is not null, then call it with @a notify_baton and
* with a notification structure in which the fields are set as follows.
- * (For a warning or error notification that does not apply to a specific
- * revision, the revision number is #SVN_INVALID_REVNUM.)
+ * (For a warning that does not apply to a specific revision, the revision
+ * number is #SVN_INVALID_REVNUM.)
*
* For each FS-specific structure warning:
* @c action = svn_repos_notify_verify_rev_structure
* @c revision = the revision or #SVN_INVALID_REVNUM
*
- * For a FS-specific structure failure:
- * @c action = #svn_repos_notify_failure
- * @c revision = #SVN_INVALID_REVNUM
- * @c err = the corresponding error chain
- *
- * For each revision verification failure:
- * @c action = #svn_repos_notify_failure
- * @c revision = the revision
- * @c err = the corresponding error chain
- *
* For each revision verification warning:
* @c action = #svn_repos_notify_warning
* @c warning and @c warning_str fields set accordingly
@@ -2888,11 +2899,6 @@ enum svn_repos_load_uuid
*
* Use @a scratch_pool for temporary allocation.
*
- * Return an error if there were any failures during verification, or
- * #SVN_NO_ERROR if there were no failures. A failure means an event that,
- * if a notification callback were provided, would send a notification
- * with @c action = #svn_repos_notify_failure.
- *
* @since New in 1.9.
*/
svn_error_t *
@@ -2899,18 +2905,20 @@ svn_error_t *
svn_repos_verify_fs3(svn_repos_t *repos,
svn_revnum_t start_rev,
svn_revnum_t end_rev,
- svn_boolean_t keep_going,
svn_boolean_t check_normalization,
svn_boolean_t metadata_only,
svn_repos_notify_func_t notify_func,
void *notify_baton,
+ svn_repos_verify_callback_t verify_callback,
+ void *verify_baton,
svn_cancel_func_t cancel,
void *cancel_baton,
apr_pool_t *scratch_pool);
/**
- * Like svn_repos_verify_fs3(), but with @a keep_going,
- * @a check_normalization and @a metadata_only set to @c FALSE.
+ * Like svn_repos_verify_fs3(), but with @a verify_callback and
+ * @a verify_baton set to @c NULL and with @a check_normalization
+ * and @a metadata_only set to @c FALSE.
*
* @since New in 1.7.
* @deprecated Provided for backward compatibility with the 1.8 API.
Index: subversion/libsvn_repos/deprecated.c
===================================================================
--- subversion/libsvn_repos/deprecated.c (revision 1687456)
+++ subversion/libsvn_repos/deprecated.c (working copy)
@@ -774,9 +774,9 @@ svn_repos_verify_fs2(svn_repos_t *repos,
end_rev,
FALSE,
FALSE,
- FALSE,
notify_func,
notify_baton,
+ NULL, NULL,
cancel_func,
cancel_baton,
pool));
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 1687456)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -2265,24 +2265,6 @@ verify_close_directory(void *dir_baton, apr_pool_t
return close_directory(dir_baton, pool);
}
-static void
-notify_verification_error(svn_revnum_t rev,
- svn_error_t *err,
- svn_repos_notify_func_t notify_func,
- void *notify_baton,
- apr_pool_t *pool)
-{
- svn_repos_notify_t *notify_failure;
-
- if (notify_func == NULL)
- return;
-
- notify_failure = svn_repos_notify_create(svn_repos_notify_failure, pool);
- notify_failure->err = err;
- notify_failure->revision = rev;
- notify_func(notify_baton, notify_failure, pool);
-}
-
/* Verify revision REV in file system FS. */
static svn_error_t *
verify_one_revision(svn_fs_t *fs,
@@ -2359,15 +2341,42 @@ verify_fs_notify_func(svn_revnum_t revision,
notify_baton->notify, pool);
}
+static svn_error_t *
+report_error(svn_revnum_t revision,
+ svn_error_t *verify_err,
+ svn_repos_verify_callback_t verify_callback,
+ void *verify_baton,
+ apr_pool_t *pool)
+{
+ if (verify_callback)
+ {
+ svn_error_t *cb_err;
+
+ /* The caller provided us with a callback, so make him responsible
+ for what's going to happen with the error. */
+ cb_err = verify_callback(verify_baton, revision, verify_err, pool);
+ svn_error_clear(verify_err);
+ SVN_ERR(cb_err);
+
+ return SVN_NO_ERROR;
+ }
+ else
+ {
+ /* No callback -- no second guessing. Just return the error. */
+ return svn_error_trace(verify_err);
+ }
+}
+
svn_error_t *
svn_repos_verify_fs3(svn_repos_t *repos,
svn_revnum_t start_rev,
svn_revnum_t end_rev,
- svn_boolean_t keep_going,
svn_boolean_t check_normalization,
svn_boolean_t metadata_only,
svn_repos_notify_func_t notify_func,
void *notify_baton,
+ svn_repos_verify_callback_t verify_callback,
+ void *verify_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool)
@@ -2380,8 +2389,6 @@ svn_repos_verify_fs3(svn_repos_t *repos,
svn_fs_progress_notify_func_t verify_notify = NULL;
struct verify_fs_notify_func_baton_t *verify_notify_baton = NULL;
svn_error_t *err;
- svn_boolean_t failed_metadata = FALSE;
- svn_revnum_t failed_revisions = 0;
/* Determine the current youngest revision of the filesystem. */
SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
@@ -2430,20 +2437,8 @@ svn_repos_verify_fs3(svn_repos_t *repos,
}
else if (err)
{
- notify_verification_error(SVN_INVALID_REVNUM, err, notify_func,
- notify_baton, iterpool);
-
- if (!keep_going)
- {
- /* Return the error, the caller doesn't want us to continue. */
- return svn_error_trace(err);
- }
- else
- {
- /* Clear the error and keep going. */
- failed_metadata = TRUE;
- svn_error_clear(err);
- }
+ SVN_ERR(report_error(SVN_INVALID_REVNUM, err, verify_callback,
+ verify_baton, iterpool));
}
if (!metadata_only)
@@ -2463,20 +2458,8 @@ svn_repos_verify_fs3(svn_repos_t *repos,
}
else if (err)
{
- notify_verification_error(rev, err, notify_func, notify_baton,
- iterpool);
-
- if (!keep_going)
- {
- /* Return the error, the caller doesn't want us to continue. */
- return svn_error_trace(err);
- }
- else
- {
- /* Clear the error and keep going. */
- ++failed_revisions;
- svn_error_clear(err);
- }
+ SVN_ERR(report_error(rev, err, verify_callback, verify_baton,
+ iterpool));
}
else if (notify_func)
{
@@ -2495,40 +2478,5 @@ svn_repos_verify_fs3(svn_repos_t *repos,
svn_pool_destroy(iterpool);
- /* Summarize the results. */
- if (failed_metadata || 0 != failed_revisions)
- {
- const char *const repos_path =
- svn_dirent_local_style(svn_repos_path(repos, pool), pool);
-
- if (0 == failed_revisions)
- {
- return svn_error_createf(
- SVN_ERR_REPOS_VERIFY_FAILED, NULL,
- _("Metadata verification failed on repository '%s'"),
- repos_path);
- }
- else
- {
- const char* format_string;
-
- if (failed_metadata)
- format_string = apr_psprintf(
- pool, _("Verification of metadata and"
- " %%%s out of %%%s revisions"
- " failed on repository '%%s'"),
- SVN_REVNUM_T_FMT, SVN_REVNUM_T_FMT);
- else
- format_string = apr_psprintf(
- pool, _("Verification of %%%s out of %%%s revisions"
- " failed on repository '%%s'"),
- SVN_REVNUM_T_FMT, SVN_REVNUM_T_FMT);
-
- return svn_error_createf(
- SVN_ERR_REPOS_VERIFY_FAILED, NULL, format_string,
- failed_revisions, end_rev - start_rev + 1, repos_path);
- }
- }
-
return SVN_NO_ERROR;
}
Index: subversion/svnadmin/svnadmin.c
===================================================================
--- subversion/svnadmin/svnadmin.c (revision 1687456)
+++ subversion/svnadmin/svnadmin.c (working copy)
@@ -864,25 +864,60 @@ err_cleanup(void *data)
return APR_SUCCESS;
}
-struct repos_notify_handler_baton {
- /* Stream to write progress and other non-error output to. */
- svn_stream_t *feedback_stream;
+struct repos_verify_callback_baton
+{
+ /* Should we continue after receiving a first verification error? */
+ svn_boolean_t keep_going;
- /* Suppress notifications that are neither errors nor warnings. */
- svn_boolean_t silent_running;
-
- /* Whether errors contained in notifications should be printed along
- with the notification. If FALSE, any errors will only be
- summarized. */
- svn_boolean_t silent_errors;
-
/* List of errors encountered during 'svnadmin verify --keep-going'. */
apr_array_header_t *error_summary;
- /* Pool for data collected during notifications. */
+ /* Pool for data collected during callback invocations. */
apr_pool_t *result_pool;
};
+/* Implementation of svn_repos_verify_callback_t to handle errors coming
+ from svn_repos_verify_fs3(). */
+static svn_error_t *
+repos_verify_callback(void *baton,
+ svn_revnum_t revision,
+ svn_error_t *verify_err,
+ apr_pool_t *scratch_pool)
+{
+ struct repos_verify_callback_baton *b = baton;
+
+ if (revision == SVN_INVALID_REVNUM)
+ {
+ SVN_ERR(svn_cmdline_fputs(_("* Error verifying repository metadata.\n"),
+ stderr, scratch_pool));
+ }
+ else
+ {
+ SVN_ERR(svn_cmdline_fprintf(stderr, scratch_pool,
+ _("* Error verifying revision %ld.\n"),
+ revision));
+ }
+
+ if (b->keep_going)
+ {
+ struct verification_error *verr;
+
+ svn_handle_error2(verify_err, stderr, FALSE, "svnadmin: ");
+
+ /* Remember the error in B->ERROR_SUMMARY. */
+ verr = apr_palloc(b->result_pool, sizeof(*verr));
+ verr->rev = revision;
+ verr->err = svn_error_dup(verify_err);
+ apr_pool_cleanup_register(b->result_pool, verr->err, err_cleanup,
+ apr_pool_cleanup_null);
+ APR_ARRAY_PUSH(b->error_summary, struct verification_error *) = verr;
+
+ return SVN_NO_ERROR;
+ }
+ else
+ return svn_error_trace(svn_error_dup(verify_err));
+}
+
/* Implementation of svn_repos_notify_func_t to wrap the output to a
response stream for svn_repos_dump_fs2(), svn_repos_verify_fs(),
svn_repos_hotcopy3() and others. */
@@ -891,17 +926,8 @@ repos_notify_handler(void *baton,
const svn_repos_notify_t *notify,
apr_pool_t *scratch_pool)
{
- struct repos_notify_handler_baton *b = baton;
- svn_stream_t *feedback_stream = b->feedback_stream;
+ svn_stream_t *feedback_stream = baton;
- /* Don't print anything if the feedback stream isn't provided.
- Only print errors and warnings in silent mode. */
- if (!feedback_stream
- || (b->silent_running
- && notify->action != svn_repos_notify_warning
- && notify->action != svn_repos_notify_failure))
- return;
-
switch (notify->action)
{
case svn_repos_notify_warning:
@@ -910,32 +936,6 @@ repos_notify_handler(void *baton,
notify->warning_str));
return;
- case svn_repos_notify_failure:
- if (notify->revision != SVN_INVALID_REVNUM)
- svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
- _("* Error verifying revision %ld.\n"),
- notify->revision));
- if (notify->err)
- {
- if (!b->silent_errors)
- svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
- "svnadmin: ");
-
- if (b->error_summary && notify->revision != SVN_INVALID_REVNUM)
- {
- struct verification_error *verr;
-
- verr = apr_palloc(b->result_pool, sizeof(*verr));
- verr->rev = notify->revision;
- verr->err = svn_error_dup(notify->err);
- apr_pool_cleanup_register(b->result_pool, verr->err, err_cleanup,
- apr_pool_cleanup_null);
- APR_ARRAY_PUSH(b->error_summary,
- struct verification_error *) = verr;
- }
- }
- return;
-
case svn_repos_notify_dump_rev_end:
svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
_("* Dumped revision %ld.\n"),
@@ -1183,7 +1183,7 @@ subcommand_dump(apr_getopt_t *os, void *baton, apr
svn_stream_t *stdout_stream;
svn_revnum_t lower = SVN_INVALID_REVNUM, upper = SVN_INVALID_REVNUM;
svn_revnum_t youngest;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
@@ -1217,12 +1217,12 @@ subcommand_dump(apr_getopt_t *os, void *baton, apr
/* Progress feedback goes to STDERR, unless they asked to suppress it. */
if (! opt_state->quiet)
- notify_baton.feedback_stream = recode_stream_create(stderr, pool);
+ feedback_stream = recode_stream_create(stderr, pool);
SVN_ERR(svn_repos_dump_fs3(repos, stdout_stream, lower, upper,
opt_state->incremental, opt_state->use_deltas,
!opt_state->quiet ? repos_notify_handler : NULL,
- ¬ify_baton, check_cancel, NULL, pool));
+ feedback_stream, check_cancel, NULL, pool));
return SVN_NO_ERROR;
}
@@ -1372,7 +1372,7 @@ subcommand_load(apr_getopt_t *os, void *baton, apr
svn_repos_t *repos;
svn_revnum_t lower = SVN_INVALID_REVNUM, upper = SVN_INVALID_REVNUM;
svn_stream_t *stdin_stream;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
@@ -1406,7 +1406,7 @@ subcommand_load(apr_getopt_t *os, void *baton, apr
/* Progress feedback goes to STDOUT, unless they asked to suppress it. */
if (! opt_state->quiet)
- notify_baton.feedback_stream = recode_stream_create(stdout, pool);
+ feedback_stream = recode_stream_create(stdout, pool);
err = svn_repos_load_fs5(repos, stdin_stream, lower, upper,
opt_state->uuid_action, opt_state->parent_dir,
@@ -1415,7 +1415,7 @@ subcommand_load(apr_getopt_t *os, void *baton, apr
!opt_state->bypass_prop_validation,
opt_state->ignore_dates,
opt_state->quiet ? NULL : repos_notify_handler,
- ¬ify_baton, check_cancel, NULL, pool);
+ feedback_stream, check_cancel, NULL, pool);
if (err && err->apr_err == SVN_ERR_BAD_PROPERTY_VALUE)
return svn_error_quick_wrap(err,
_("Invalid property value found in "
@@ -1462,12 +1462,12 @@ subcommand_recover(apr_getopt_t *os, void *baton,
svn_repos_t *repos;
svn_error_t *err;
struct svnadmin_opt_state *opt_state = baton;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
- SVN_ERR(svn_stream_for_stdout(¬ify_baton.feedback_stream, pool));
+ SVN_ERR(svn_stream_for_stdout(&feedback_stream, pool));
/* Restore default signal handlers until after we have acquired the
* exclusive lock so that the user interrupt before we actually
@@ -1475,7 +1475,7 @@ subcommand_recover(apr_getopt_t *os, void *baton,
setup_cancellation_signals(SIG_DFL);
err = svn_repos_recover4(opt_state->repository_path, TRUE,
- repos_notify_handler, ¬ify_baton,
+ repos_notify_handler, feedback_stream,
check_cancel, NULL, pool);
if (err)
{
@@ -1493,7 +1493,7 @@ subcommand_recover(apr_getopt_t *os, void *baton,
" another process has it open?\n")));
SVN_ERR(svn_cmdline_fflush(stdout));
SVN_ERR(svn_repos_recover4(opt_state->repository_path, FALSE,
- repos_notify_handler, ¬ify_baton,
+ repos_notify_handler, feedback_stream,
check_cancel, NULL, pool));
}
@@ -1779,7 +1779,7 @@ subcommand_pack(apr_getopt_t *os, void *baton, apr
{
struct svnadmin_opt_state *opt_state = baton;
svn_repos_t *repos;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
@@ -1788,11 +1788,11 @@ subcommand_pack(apr_getopt_t *os, void *baton, apr
/* Progress feedback goes to STDOUT, unless they asked to suppress it. */
if (! opt_state->quiet)
- notify_baton.feedback_stream = recode_stream_create(stdout, pool);
+ feedback_stream = recode_stream_create(stdout, pool);
return svn_error_trace(
svn_repos_fs_pack2(repos, !opt_state->quiet ? repos_notify_handler : NULL,
- ¬ify_baton, check_cancel, NULL, pool));
+ feedback_stream, check_cancel, NULL, pool));
}
@@ -1804,10 +1804,8 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
svn_repos_t *repos;
svn_fs_t *fs;
svn_revnum_t youngest, lower, upper;
- struct repos_notify_handler_baton notify_baton = { 0 };
- struct repos_notify_handler_baton *notify_baton_p = ¬ify_baton;
- svn_repos_notify_func_t notify_func = repos_notify_handler;
- svn_error_t *verify_err;
+ svn_stream_t *feedback_stream = NULL;
+ struct repos_verify_callback_baton verify_baton = { 0 };
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
@@ -1851,42 +1849,27 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
upper = lower;
}
- /* Set up the notification handler. */
- if (!opt_state->quiet || opt_state->keep_going)
- {
- if (opt_state->quiet)
- {
- notify_baton.silent_running = TRUE;
- notify_baton.feedback_stream = recode_stream_create(stderr, pool);
- }
- else
- notify_baton.feedback_stream = recode_stream_create(stdout, pool);
+ if (!opt_state->quiet)
+ feedback_stream = recode_stream_create(stdout, pool);
- if (opt_state->keep_going)
- notify_baton.error_summary =
- apr_array_make(pool, 0, sizeof(struct verification_error *));
- else
- notify_baton.silent_errors = TRUE;
+ verify_baton.keep_going = opt_state->keep_going;
+ verify_baton.error_summary =
+ apr_array_make(pool, 0, sizeof(struct verification_error *));
+ verify_baton.result_pool = pool;
- notify_baton.result_pool = pool;
- }
- else
- {
- notify_func = NULL;
- notify_baton_p = NULL;
- }
+ SVN_ERR(svn_repos_verify_fs3(repos, lower, upper,
+ opt_state->check_normalization,
+ opt_state->metadata_only,
+ !opt_state->quiet
+ ? repos_notify_handler : NULL,
+ feedback_stream,
+ repos_verify_callback, &verify_baton,
+ check_cancel, NULL, pool));
- verify_err = svn_repos_verify_fs3(repos, lower, upper,
- opt_state->keep_going,
- opt_state->check_normalization,
- opt_state->metadata_only,
- notify_func, notify_baton_p,
- check_cancel, NULL, pool);
-
/* Show the --keep-going error summary. */
if (!opt_state->quiet
&& opt_state->keep_going
- && notify_baton.error_summary->nelts > 0)
+ && verify_baton.error_summary->nelts > 0)
{
int rev_maxlength;
svn_revnum_t end_revnum;
@@ -1894,7 +1877,7 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
int i;
svn_error_clear(
- svn_stream_puts(notify_baton.feedback_stream,
+ svn_stream_puts(feedback_stream,
_("\n-----Summary of corrupt revisions-----\n")));
/* The standard column width for the revision number is 6 characters.
@@ -1901,8 +1884,8 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
If the revision number can potentially be larger (i.e. if end_revnum
is larger than 1000000), we increase the column width as needed. */
rev_maxlength = 6;
- end_revnum = APR_ARRAY_IDX(notify_baton.error_summary,
- notify_baton.error_summary->nelts - 1,
+ end_revnum = APR_ARRAY_IDX(verify_baton.error_summary,
+ verify_baton.error_summary->nelts - 1,
struct verification_error *)->rev;
while (end_revnum >= 1000000)
{
@@ -1911,7 +1894,7 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
}
iterpool = svn_pool_create(pool);
- for (i = 0; i < notify_baton.error_summary->nelts; i++)
+ for (i = 0; i < verify_baton.error_summary->nelts; i++)
{
struct verification_error *verr;
svn_error_t *err;
@@ -1919,22 +1902,25 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
svn_pool_clear(iterpool);
- verr = APR_ARRAY_IDX(notify_baton.error_summary, i,
+ verr = APR_ARRAY_IDX(verify_baton.error_summary, i,
struct verification_error *);
- rev_str = apr_psprintf(iterpool, "r%ld", verr->rev);
- rev_str = apr_psprintf(iterpool, "%*s", rev_maxlength, rev_str);
- for (err = svn_error_purge_tracing(verr->err);
- err != SVN_NO_ERROR; err = err->child)
+
+ if (verr->rev != SVN_INVALID_REVNUM)
{
- char buf[512];
- const char *message;
+ rev_str = apr_psprintf(iterpool, "r%ld", verr->rev);
+ rev_str = apr_psprintf(iterpool, "%*s", rev_maxlength, rev_str);
+ for (err = svn_error_purge_tracing(verr->err);
+ err != SVN_NO_ERROR; err = err->child)
+ {
+ char buf[512];
+ const char *message;
- message = svn_err_best_message(err, buf, sizeof(buf));
- svn_error_clear(svn_stream_printf(notify_baton.feedback_stream,
- iterpool,
- "%s: E%06d: %s\n",
- rev_str, err->apr_err,
- message));
+ message = svn_err_best_message(err, buf, sizeof(buf));
+ svn_error_clear(svn_stream_printf(feedback_stream, iterpool,
+ "%s: E%06d: %s\n",
+ rev_str, err->apr_err,
+ message));
+ }
}
}
@@ -1941,7 +1927,15 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
svn_pool_destroy(iterpool);
}
- return svn_error_trace(verify_err);
+ if (verify_baton.error_summary->nelts > 0)
+ {
+ return svn_error_createf(SVN_ERR_CL_REPOS_VERIFY_FAILED, NULL,
+ _("Failed to verify repository '%s'"),
+ svn_dirent_local_style(
+ opt_state->repository_path, pool));
+ }
+
+ return SVN_NO_ERROR;
}
/* This implements `svn_opt_subcommand_t'. */
@@ -1949,7 +1943,7 @@ svn_error_t *
subcommand_hotcopy(apr_getopt_t *os, void *baton, apr_pool_t *pool)
{
struct svnadmin_opt_state *opt_state = baton;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
apr_array_header_t *targets;
const char *new_repos_path;
@@ -1960,12 +1954,12 @@ subcommand_hotcopy(apr_getopt_t *os, void *baton,
/* Progress feedback goes to STDOUT, unless they asked to suppress it. */
if (! opt_state->quiet)
- notify_baton.feedback_stream = recode_stream_create(stdout, pool);
+ feedback_stream = recode_stream_create(stdout, pool);
return svn_repos_hotcopy3(opt_state->repository_path, new_repos_path,
opt_state->clean_logs, opt_state->incremental,
!opt_state->quiet ? repos_notify_handler : NULL,
- ¬ify_baton, check_cancel, NULL, pool);
+ feedback_stream, check_cancel, NULL, pool);
}
svn_error_t *
@@ -2349,18 +2343,18 @@ subcommand_upgrade(apr_getopt_t *os, void *baton,
{
svn_error_t *err;
struct svnadmin_opt_state *opt_state = baton;
- struct repos_notify_handler_baton notify_baton = { 0 };
+ svn_stream_t *feedback_stream = NULL;
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
- SVN_ERR(svn_stream_for_stdout(¬ify_baton.feedback_stream, pool));
+ SVN_ERR(svn_stream_for_stdout(&feedback_stream, pool));
/* Restore default signal handlers. */
setup_cancellation_signals(SIG_DFL);
err = svn_repos_upgrade2(opt_state->repository_path, TRUE,
- repos_notify_handler, ¬ify_baton, pool);
+ repos_notify_handler, feedback_stream, pool);
if (err)
{
if (APR_STATUS_IS_EAGAIN(err->apr_err))
@@ -2378,7 +2372,7 @@ subcommand_upgrade(apr_getopt_t *os, void *baton,
" another process has it open?\n")));
SVN_ERR(svn_cmdline_fflush(stdout));
SVN_ERR(svn_repos_upgrade2(opt_state->repository_path, FALSE,
- repos_notify_handler, ¬ify_baton,
+ repos_notify_handler, feedback_stream,
pool));
}
else if (err->apr_err == SVN_ERR_FS_UNSUPPORTED_UPGRADE)
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py (revision 1687456)
+++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
@@ -2070,8 +2070,6 @@ def verify_keep_going(sbox):
exp_out = svntest.verify.RegexListOutput([".*Verified revision 0.",
".*Verified revision 1.",
- ".*Error verifying revision 2.",
- ".*Error verifying revision 3.",
".*",
".*Summary.*",
".*r2: E160004:.*",
@@ -2082,8 +2080,18 @@ def verify_keep_going(sbox):
if (svntest.main.fs_has_rep_sharing()):
exp_out.insert(0, ".*Verifying.*metadata.*")
- exp_err = svntest.verify.RegexListOutput(["svnadmin: E160004:.*",
- "svnadmin: E165011:.*"], False)
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*",
+ ".*Error verifying revision 3.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*",
+ "svnadmin: E205012:.*"], False)
+
+ if (svntest.main.is_fs_log_addressing()):
+ exp_err.insert(0, ".*Error verifying repository metadata.")
+ exp_err.insert(1, "svnadmin: E160004:.*")
+
if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin
verify'.",
output, errput, exp_out, exp_err):
raise svntest.Failure
@@ -2095,12 +2103,19 @@ def verify_keep_going(sbox):
exp_out = svntest.verify.RegexListOutput([".*Verifying metadata at
revision 0"])
else:
exp_out = svntest.verify.RegexListOutput([".*Verified revision 0.",
- ".*Verified revision 1.",
- ".*Error verifying revision 2."])
+ ".*Verified revision 1."])
if (svntest.main.fs_has_rep_sharing()):
exp_out.insert(0, ".*Verifying repository metadata.*")
- exp_err = svntest.verify.RegexListOutput(["svnadmin: E160004:.*"], False)
+ if (svntest.main.is_fs_log_addressing()):
+ exp_err = svntest.verify.RegexListOutput([
+ ".*Error verifying repository metadata.",
+ "svnadmin: E160004:.*"], False)
+ else:
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*"], False)
+
if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin
verify'.",
output, errput, exp_out, exp_err):
raise svntest.Failure
@@ -2110,8 +2125,17 @@ def verify_keep_going(sbox):
"--quiet",
sbox.repo_dir)
+ if (svntest.main.is_fs_log_addressing()):
+ exp_err = svntest.verify.RegexListOutput([
+ ".*Error verifying repository metadata.",
+ "svnadmin: E160004:.*"], False)
+ else:
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*"], False)
+
if svntest.verify.verify_outputs("Output of 'svnadmin verify' is
unexpected.",
- None, errput, None, "svnadmin: E160004:.*"):
+ None, errput, None, exp_err):
raise svntest.Failure
# Don't leave a corrupt repository
@@ -2152,11 +2176,12 @@ def verify_keep_going_quiet(sbox):
".*Error verifying revision 3.",
"svnadmin: E160004:.*",
"svnadmin: E160004:.*",
- "svnadmin: E165011:.*"], False)
+ "svnadmin: E205012:.*"], False)
# Insert another expected error from checksum verification
if (svntest.main.is_fs_log_addressing()):
- exp_err.insert(0, "svnadmin: E160004:.*")
+ exp_err.insert(0, ".*Error verifying repository metadata.")
+ exp_err.insert(1, "svnadmin: E160004:.*")
if svntest.verify.verify_outputs(
"Unexpected error while running 'svnadmin verify'.",
@@ -2231,23 +2256,15 @@ def verify_invalid_path_changes(sbox):
exp_out = svntest.verify.RegexListOutput([".*Verified revision 0.",
".*Verified revision 1.",
- ".*Error verifying revision 2.",
".*Verified revision 3.",
- ".*Error verifying revision 4.",
".*Verified revision 5.",
- ".*Error verifying revision 6.",
".*Verified revision 7.",
".*Verified revision 8.",
".*Verified revision 9.",
- ".*Error verifying revision 10.",
".*Verified revision 11.",
- ".*Error verifying revision 12.",
".*Verified revision 13.",
- ".*Error verifying revision 14.",
".*Verified revision 15.",
- ".*Error verifying revision 16.",
".*Verified revision 17.",
- ".*Error verifying revision 18.",
".*Verified revision 19.",
".*",
".*Summary.*",
@@ -2271,9 +2288,30 @@ def verify_invalid_path_changes(sbox):
if svntest.main.is_fs_log_addressing():
exp_out.insert(1, ".*Verifying.*metadata.*")
- exp_err = svntest.verify.RegexListOutput(["svnadmin: E160020:.*",
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160020:.*",
+ "svnadmin: E160020:.*",
+ ".*Error verifying revision 4.",
+ "svnadmin: E160013:.*",
+ ".*Error verifying revision 6.",
+ "svnadmin: E160013:.*",
+ "svnadmin: E160013:.*",
+ ".*Error verifying revision 10.",
+ "svnadmin: E160013:.*",
+ "svnadmin: E160013:.*",
+ ".*Error verifying revision 12.",
"svnadmin: E145001:.*",
- "svnadmin: E160013:.*"], False)
+ "svnadmin: E145001:.*",
+ ".*Error verifying revision 14.",
+ "svnadmin: E160013:.*",
+ "svnadmin: E160013:.*",
+ ".*Error verifying revision 16.",
+ "svnadmin: E145001:.*",
+ "svnadmin: E145001:.*",
+ ".*Error verifying revision 18.",
+ "svnadmin: E160013:.*",
+ "svnadmin: E160013:.*",
+ "svnadmin: E205012:.*"], False)
if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin
verify'.",
@@ -2284,9 +2322,10 @@ def verify_invalid_path_changes(sbox):
sbox.repo_dir)
exp_out = svntest.verify.RegexListOutput([".*Verified revision 0.",
- ".*Verified revision 1.",
- ".*Error verifying revision 2."])
- exp_err = svntest.verify.RegexListOutput(["svnadmin: E160020:.*"], False)
+ ".*Verified revision 1."])
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160020:.*",
+ "svnadmin: E160020:.*"], False)
if (svntest.main.fs_has_rep_sharing()):
exp_out.insert(0, ".*Verifying.*metadata.*")
@@ -2301,8 +2340,13 @@ def verify_invalid_path_changes(sbox):
"--quiet",
sbox.repo_dir)
+ exp_out = []
+ exp_err = svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160020:.*",
+ "svnadmin: E160020:.*"], False)
+
if svntest.verify.verify_outputs("Output of 'svnadmin verify' is
unexpected.",
- None, errput, None, "svnadmin: E160020:.*"):
+ output, errput, exp_out, exp_err):
raise svntest.Failure
# Don't leave a corrupt repository
Index: subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
===================================================================
--- subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c (revision 1687456)
+++ subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c (working copy)
@@ -117,8 +117,9 @@ fuzzing_1_byte_1_rev(const char *repo_name,
svn_fs_set_warning_func(svn_repos_fs(repos), dont_filter_warnings, NULL);
/* This shall detect the corruption and return an error. */
- err = svn_repos_verify_fs3(repos, revision, revision, TRUE, FALSE, FALSE,
- NULL, NULL, NULL, NULL, iterpool);
+ err = svn_repos_verify_fs3(repos, revision, revision, FALSE, FALSE,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ iterpool);
/* Case-only changes in checksum digests are not an error.
* We allow upper case chars to be used in MD5 checksums in all other
Index: subversion/tests/libsvn_fs_fs/fs-fs-private-test.c
===================================================================
--- subversion/tests/libsvn_fs_fs/fs-fs-private-test.c (revision 1687456)
+++ subversion/tests/libsvn_fs_fs/fs-fs-private-test.c (working copy)
@@ -361,9 +361,10 @@ receive_index(const svn_fs_fs__p2l_entry_t *entry,
return SVN_NO_ERROR;
}
+#define REPO_NAME "test-repo-load-index-test"
+
static svn_error_t *
-load_index_test(const svn_test_opts_t *opts, apr_pool_t *pool,
- const char *repo_name, svn_boolean_t keep_going)
+load_index(const svn_test_opts_t *opts, apr_pool_t *pool)
{
svn_repos_t *repos;
svn_revnum_t rev;
@@ -381,7 +382,7 @@ static svn_error_t *
"pre-1.9 SVN doesn't have FSFS indexes");
/* Create a filesystem */
- SVN_ERR(create_greek_repo(&repos, &rev, opts, repo_name, pool, pool));
+ SVN_ERR(create_greek_repo(&repos, &rev, opts, REPO_NAME, pool, pool));
/* Read the original index contents for REV in ENTRIES. */
SVN_ERR(svn_fs_fs__dump_index(svn_repos_fs(repos), rev, receive_index,
@@ -397,34 +398,21 @@ static svn_error_t *
APR_ARRAY_PUSH(alt_entries, svn_fs_fs__p2l_entry_t *) = &entry;
SVN_ERR(svn_fs_fs__load_index(svn_repos_fs(repos), rev, alt_entries, pool));
- SVN_TEST_ASSERT_ERROR(svn_repos_verify_fs3(repos, rev, rev,
- keep_going, FALSE, FALSE,
- NULL, NULL, NULL, NULL, pool),
- (keep_going
- ? SVN_ERR_REPOS_VERIFY_FAILED
- : SVN_ERR_FS_INDEX_CORRUPTION));
+ SVN_TEST_ASSERT_ERROR(svn_repos_verify_fs3(repos, rev, rev, FALSE, FALSE,
+ NULL, NULL, NULL, NULL, NULL,
+ NULL, pool),
+ SVN_ERR_FS_INDEX_CORRUPTION);
/* Restore the original index. */
SVN_ERR(svn_fs_fs__load_index(svn_repos_fs(repos), rev, entries, pool));
- SVN_ERR(svn_repos_verify_fs3(repos, rev, rev, keep_going, FALSE, FALSE,
+ SVN_ERR(svn_repos_verify_fs3(repos, rev, rev, FALSE, FALSE, NULL, NULL,
NULL, NULL, NULL, NULL, pool));
return SVN_NO_ERROR;
}
-static svn_error_t *
-load_index(const svn_test_opts_t *opts,
- apr_pool_t *pool)
-{
- return load_index_test(opts, pool, "test-repo-load-index-test", FALSE);
-}
+#undef REPO_NAME
-static svn_error_t *
-load_index_keep_going(const svn_test_opts_t *opts,
- apr_pool_t *pool)
-{
- return load_index_test(opts, pool, "test-repo-load-index-full-test", TRUE);
-}
/* The test table. */
@@ -440,8 +428,6 @@ static struct svn_test_descriptor_t test_funcs[] =
"dump the P2L index"),
SVN_TEST_OPTS_PASS(load_index,
"load the P2L index"),
- SVN_TEST_OPTS_PASS(load_index_keep_going,
- "load the P2L index (full verification)"),
SVN_TEST_NULL
};
