Branko Čibej <br...@wandisco.com> writes:
> Evgeny, please take a look at r1684940.
>
> I don't really like the fact that with this change and 'svnadmin
> --keep-going --quiet', the text "Error verifying revision N" gets printed
> to stderr; but I couldn't think of a better way to join the error to the
> revision it was emitted for. I'd love to find a better solution.
After giving this change a look, I cannot get rid of the feeling that what we
are doing here is just adding different workarounds for the API issues, e.g.,
booleans like b->silent_errors or b->silent_running. I should also say that
I find the b->feedback_stream manipulations questionable, because now, for
instance, we write warnings to either stdout or stderr depending on the --quiet
argument. I always thought that --quiet should only suppress a part of the
output, but shouldn't really retarget it to a different standard stream.
With that in mind, I am -0 on the corresponding backport proposal.
I sketched the other possible option with redesigning svn_repos_verify_fs3()
API to only report errors via the notify_func(), please see the attached patch.
Although I won't insist on going this way, I like it better, as it allows us
to get rid of things like b->silent_errors, b->silent_running, juggling with
standard streams and API that can yield the same error in two different ways.
What do you think?
Regards,
Evgeny Kotkov
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1685592)
+++ subversion/include/svn_repos.h (working copy)
@@ -2848,10 +2848,11 @@ 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 notify_func is not null, then call it with @a notify_baton and
+ * The @a notify_func function will be called 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.)
+ * revision, the revision number is #SVN_INVALID_REVNUM.) @a notify_func
+ * is mandatory and cannot be @c NULL.
*
* For each FS-specific structure warning:
* @c action = svn_repos_notify_verify_rev_structure
@@ -2888,10 +2889,11 @@ 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.
+ * Returned error indicates an error associated with the verification
+ * process itself (typically #SVN_ERR_CANCELLED). These errors are always
+ * returned immediately. Return value of #SVN_NO_ERROR indicates that
+ * the verification process has completed and that the corresponding errors,
+ * if any, were reported via the @a notify_func callback.
*
* @since New in 1.9.
*/
Index: subversion/libsvn_repos/deprecated.c
===================================================================
--- subversion/libsvn_repos/deprecated.c (revision 1685592)
+++ subversion/libsvn_repos/deprecated.c (working copy)
@@ -759,6 +759,31 @@ svn_repos_dump_fs2(svn_repos_t *repos,
pool));
}
+typedef struct verify_compat_notify_baton_t
+{
+ svn_error_t *notify_err;
+ svn_repos_notify_func_t notify_func;
+ void *notify_baton;
+} verify_compat_notify_baton_t;
+
+static void
+verify_compat_notify_func(void *baton,
+ const svn_repos_notify_t *notify,
+ apr_pool_t *scratch_pool)
+{
+ verify_compat_notify_baton_t *b = baton;
+
+ if (notify->action == svn_repos_notify_failure)
+ {
+ b->notify_err = svn_error_compose_create(b->notify_err,
+ svn_error_dup(notify->err));
+ }
+ else if (b->notify_func)
+ {
+ b->notify_func(b->notify_baton, notify, scratch_pool);
+ }
+}
+
svn_error_t *
svn_repos_verify_fs2(svn_repos_t *repos,
svn_revnum_t start_rev,
@@ -769,17 +794,18 @@ svn_repos_verify_fs2(svn_repos_t *repos,
void *cancel_baton,
apr_pool_t *pool)
{
- return svn_error_trace(svn_repos_verify_fs3(repos,
- start_rev,
- end_rev,
- FALSE,
- FALSE,
- FALSE,
- notify_func,
- notify_baton,
- cancel_func,
- cancel_baton,
- pool));
+ verify_compat_notify_baton_t baton;
+ svn_error_t *err;
+
+ baton.notify_err = SVN_NO_ERROR;
+ baton.notify_func = notify_func;
+ baton.notify_baton = notify_baton;
+
+ err = svn_repos_verify_fs3(repos, start_rev, end_rev,
+ FALSE, FALSE, FALSE, verify_compat_notify_func,
+ &baton, cancel_func, cancel_baton, pool);
+
+ return svn_error_compose_create(err, baton.notify_err);
}
svn_error_t *
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 1685592)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -2274,9 +2274,6 @@ notify_verification_error(svn_revnum_t rev,
{
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;
@@ -2375,14 +2372,13 @@ svn_repos_verify_fs3(svn_repos_t *repos,
svn_fs_t *fs = svn_repos_fs(repos);
svn_revnum_t youngest;
svn_revnum_t rev;
- apr_pool_t *iterpool = svn_pool_create(pool);
svn_repos_notify_t *notify;
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;
+ SVN_ERR_ASSERT(notify_func);
+
/* Determine the current youngest revision of the filesystem. */
SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
@@ -2406,18 +2402,14 @@ svn_repos_verify_fs3(svn_repos_t *repos,
/* Create a notify object that we can reuse within the loop and a
forwarding structure for notifications from inside svn_fs_verify(). */
- if (notify_func)
- {
- notify = svn_repos_notify_create(svn_repos_notify_verify_rev_end, pool);
+ notify = svn_repos_notify_create(svn_repos_notify_verify_rev_end, pool);
+ verify_notify = verify_fs_notify_func;
+ verify_notify_baton = apr_palloc(pool, sizeof(*verify_notify_baton));
+ verify_notify_baton->notify_func = notify_func;
+ verify_notify_baton->notify_baton = notify_baton;
+ verify_notify_baton->notify
+ = svn_repos_notify_create(svn_repos_notify_verify_rev_structure, pool);
- verify_notify = verify_fs_notify_func;
- verify_notify_baton = apr_palloc(pool, sizeof(*verify_notify_baton));
- verify_notify_baton->notify_func = notify_func;
- verify_notify_baton->notify_baton = notify_baton;
- verify_notify_baton->notify
- = svn_repos_notify_create(svn_repos_notify_verify_rev_structure, pool);
- }
-
/* Verify global metadata and backend-specific data first. */
err = svn_fs_verify(svn_fs_path(fs, pool), svn_fs_config(fs, pool),
start_rev, end_rev,
@@ -2431,104 +2423,60 @@ svn_repos_verify_fs3(svn_repos_t *repos,
else if (err)
{
notify_verification_error(SVN_INVALID_REVNUM, err, notify_func,
- notify_baton, iterpool);
+ notify_baton, pool);
+ svn_error_clear(err);
if (!keep_going)
{
- /* Return the error, the caller doesn't want us to continue. */
- return svn_error_trace(err);
+ /* The caller doesn't want us to continue. */
+ return SVN_NO_ERROR;
}
- else
- {
- /* Clear the error and keep going. */
- failed_metadata = TRUE;
- svn_error_clear(err);
- }
}
if (!metadata_only)
- for (rev = start_rev; rev <= end_rev; rev++)
- {
- svn_pool_clear(iterpool);
-
- /* Wrapper function to catch the possible errors. */
- err = verify_one_revision(fs, rev, notify_func, notify_baton,
- start_rev, check_normalization,
- cancel_func, cancel_baton,
- iterpool);
-
- if (err && err->apr_err == SVN_ERR_CANCELLED)
- {
- return svn_error_trace(err);
- }
- 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);
- }
- }
- else if (notify_func)
- {
- /* Tell the caller that we're done with this revision. */
- notify->revision = rev;
- notify_func(notify_baton, notify, iterpool);
- }
- }
-
- /* We're done. */
- if (notify_func)
{
- notify = svn_repos_notify_create(svn_repos_notify_verify_end, iterpool);
- notify_func(notify_baton, notify, iterpool);
- }
+ apr_pool_t *iterpool = svn_pool_create(pool);
- svn_pool_destroy(iterpool);
+ for (rev = start_rev; rev <= end_rev; rev++)
+ {
+ svn_pool_clear(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);
+ /* Wrapper function to catch the possible errors. */
+ err = verify_one_revision(fs, rev, notify_func, notify_baton,
+ start_rev, check_normalization,
+ cancel_func, cancel_baton,
+ iterpool);
- 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 (err && err->apr_err == SVN_ERR_CANCELLED)
+ {
+ return svn_error_trace(err);
+ }
+ else if (err)
+ {
+ notify_verification_error(rev, err, notify_func, notify_baton,
+ iterpool);
+ svn_error_clear(err);
- 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);
+ if (!keep_going)
+ {
+ /* The caller doesn't want us to continue. */
+ svn_pool_destroy(iterpool);
+ return SVN_NO_ERROR;
+ }
+ }
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);
+ {
+ /* Tell the caller that we're done with this revision. */
+ notify->revision = rev;
+ notify_func(notify_baton, notify, iterpool);
+ }
}
+ svn_pool_destroy(iterpool);
}
+ /* We're done. */
+ notify = svn_repos_notify_create(svn_repos_notify_verify_end, pool);
+ notify_func(notify_baton, notify, pool);
+
return SVN_NO_ERROR;
}
Index: subversion/svnadmin/svnadmin.c
===================================================================
--- subversion/svnadmin/svnadmin.c (revision 1685592)
+++ subversion/svnadmin/svnadmin.c (working copy)
@@ -846,46 +846,14 @@ subcommand_deltify(apr_getopt_t *os, void *baton,
return SVN_NO_ERROR;
}
-/* Structure for errors encountered during 'svnadmin verify --keep-going'. */
-struct verification_error
-{
- svn_revnum_t rev;
- svn_error_t *err;
-};
-
-/* Pool cleanup function to clear an svn_error_t *. */
-static apr_status_t
-err_cleanup(void *data)
-{
- svn_error_t *err = data;
-
- svn_error_clear(err);
-
- return APR_SUCCESS;
-}
-
struct repos_notify_handler_baton {
/* Stream to write progress and other non-error output to. */
svn_stream_t *feedback_stream;
-
- /* 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. */
- apr_pool_t *result_pool;
};
/* 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. */
+ response stream for svn_repos_dump_fs2(), svn_repos_hotcopy3() and
+ others. */
static void
repos_notify_handler(void *baton,
const svn_repos_notify_t *notify,
@@ -894,14 +862,6 @@ repos_notify_handler(void *baton,
struct repos_notify_handler_baton *b = baton;
svn_stream_t *feedback_stream = b->feedback_stream;
- /* 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 +870,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"),
@@ -942,22 +876,6 @@ repos_notify_handler(void *baton,
notify->revision));
return;
- case svn_repos_notify_verify_rev_end:
- svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
- _("* Verified revision %ld.\n"),
- notify->revision));
- return;
-
- case svn_repos_notify_verify_rev_structure:
- if (notify->revision == SVN_INVALID_REVNUM)
- svn_error_clear(svn_stream_puts(feedback_stream,
- _("* Verifying repository metadata ...\n")));
- else
- svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
- _("* Verifying metadata at revision %ld ...\n"),
- notify->revision));
- return;
-
case svn_repos_notify_pack_shard_start:
{
const char *shardstr = apr_psprintf(scratch_pool,
@@ -1132,7 +1050,108 @@ repos_notify_handler(void *baton,
}
}
+struct verify_notify_handler_baton
+{
+ /* Stream to write progress and other non-error output to. */
+ svn_stream_t *feedback_stream;
+ /* List of encountered errors. */
+ apr_array_header_t *errors;
+
+ /* Pool for data collected during notifications. */
+ apr_pool_t *result_pool;
+};
+
+/* Structure for errors encountered during 'svnadmin verify'. */
+struct verification_error
+{
+ svn_revnum_t rev;
+ svn_error_t *err;
+};
+
+/* Pool cleanup function to clear an svn_error_t *. */
+static apr_status_t err_cleanup(void *data)
+{
+ svn_error_t *err = data;
+
+ svn_error_clear(err);
+
+ return APR_SUCCESS;
+}
+
+/* Implementation of svn_repos_notify_func_t to handle notifications
+ originating from svn_repos_verify_fs(). */
+static void
+verify_notify_handler(void *baton,
+ const svn_repos_notify_t *notify,
+ apr_pool_t *scratch_pool)
+{
+ struct verify_notify_handler_baton *b = baton;
+
+ switch (notify->action)
+ {
+ case svn_repos_notify_warning:
+ svn_error_clear(svn_cmdline_fprintf(stderr, scratch_pool,
+ "WARNING 0x%04x: %s\n",
+ notify->warning,
+ notify->warning_str));
+ return;
+
+ case svn_repos_notify_failure:
+ {
+ struct verification_error *verr;
+
+ if (notify->revision == SVN_INVALID_REVNUM)
+ {
+ svn_error_clear(svn_cmdline_fputs(
+ _("* Error verifying repository metadata.\n"),
+ stderr, scratch_pool));
+ }
+ else
+ {
+ svn_error_clear(svn_cmdline_fprintf(stderr, scratch_pool,
+ _("* Error verifying revision %ld.\n"),
+ notify->revision));
+ }
+ svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
+ "svnadmin: ");
+
+ /* Remember the error in B->ERRORS. */
+ verr = apr_pcalloc(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->errors, struct verification_error *) = verr;
+
+ return;
+ }
+
+ case svn_repos_notify_verify_rev_end:
+ svn_error_clear(svn_stream_printf(b->feedback_stream, scratch_pool,
+ _("* Verified revision %ld.\n"),
+ notify->revision));
+ return;
+
+ case svn_repos_notify_verify_rev_structure:
+ if (notify->revision == SVN_INVALID_REVNUM)
+ {
+ svn_error_clear(svn_stream_puts(b->feedback_stream,
+ _("* Verifying repository metadata ...\n")));
+ }
+ else
+ {
+ svn_error_clear(svn_stream_printf(b->feedback_stream, scratch_pool,
+ _("* Verifying metadata at revision %ld ...\n"),
+ notify->revision));
+ }
+ return;
+
+ default:
+ return;
+ }
+}
+
/* Baton for recode_write(). */
struct recode_write_baton
{
@@ -1795,7 +1814,63 @@ subcommand_pack(apr_getopt_t *os, void *baton, apr
¬ify_baton, check_cancel, NULL, pool));
}
+static void
+print_error_summary(const apr_array_header_t *errors,
+ FILE *stream, apr_pool_t *pool)
+{
+ int rev_maxlength;
+ svn_revnum_t end_revnum;
+ apr_pool_t *iterpool;
+ int i;
+ svn_error_clear(
+ svn_cmdline_fputs(_("\n-----Summary of corrupt revisions-----\n"),
+ stream, pool));
+
+ /* The standard column width for the revision number is 6 characters.
+ 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(errors, errors->nelts - 1,
+ struct verification_error *)->rev;
+ while (end_revnum >= 1000000)
+ {
+ rev_maxlength++;
+ end_revnum = end_revnum / 10;
+ }
+
+ iterpool = svn_pool_create(pool);
+ for (i = 0; i < errors->nelts; i++)
+ {
+ struct verification_error *verr;
+ svn_error_t *err;
+ const char *rev_str;
+
+ svn_pool_clear(iterpool);
+
+ verr = APR_ARRAY_IDX(errors, i, struct verification_error *);
+
+ if (verr->rev != SVN_INVALID_REVNUM)
+ {
+ 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_cmdline_fprintf(stream, iterpool,
+ "%s: E%06d: %s\n", rev_str,
+ err->apr_err, message));
+ }
+ }
+ }
+ svn_pool_destroy(iterpool);
+}
+
/* This implements `svn_opt_subcommand_t'. */
static svn_error_t *
subcommand_verify(apr_getopt_t *os, void *baton, apr_pool_t *pool)
@@ -1804,10 +1879,7 @@ 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;
+ struct verify_notify_handler_baton notify_baton = { 0 };
/* Expect no more arguments. */
SVN_ERR(parse_args(NULL, os, 0, 0, pool));
@@ -1851,97 +1923,38 @@ 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)
{
- 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->keep_going)
- notify_baton.error_summary =
- apr_array_make(pool, 0, sizeof(struct verification_error *));
- else
- notify_baton.silent_errors = TRUE;
-
- notify_baton.result_pool = pool;
+ notify_baton.feedback_stream = svn_stream_empty(pool);
}
else
{
- notify_func = NULL;
- notify_baton_p = NULL;
+ notify_baton.feedback_stream = recode_stream_create(stdout, 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);
+ notify_baton.errors = apr_array_make(pool, 0,
+ sizeof(struct verification_error *));
+ notify_baton.result_pool = pool;
- /* Show the --keep-going error summary. */
- if (!opt_state->quiet
- && opt_state->keep_going
- && notify_baton.error_summary->nelts > 0)
+ SVN_ERR(svn_repos_verify_fs3(repos, lower, upper,
+ opt_state->keep_going,
+ opt_state->check_normalization,
+ opt_state->metadata_only,
+ verify_notify_handler, ¬ify_baton,
+ check_cancel, NULL, pool));
+
+ if (notify_baton.errors->nelts > 0)
{
- int rev_maxlength;
- svn_revnum_t end_revnum;
- apr_pool_t *iterpool;
- int i;
+ if (opt_state->keep_going && !opt_state->quiet)
+ print_error_summary(notify_baton.errors, stdout, pool);
- svn_error_clear(
- svn_stream_puts(notify_baton.feedback_stream,
- _("\n-----Summary of corrupt revisions-----\n")));
-
- /* The standard column width for the revision number is 6 characters.
- 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,
- struct verification_error *)->rev;
- while (end_revnum >= 1000000)
- {
- rev_maxlength++;
- end_revnum = end_revnum / 10;
- }
-
- iterpool = svn_pool_create(pool);
- for (i = 0; i < notify_baton.error_summary->nelts; i++)
- {
- struct verification_error *verr;
- svn_error_t *err;
- const char *rev_str;
-
- svn_pool_clear(iterpool);
-
- verr = APR_ARRAY_IDX(notify_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)
- {
- 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));
- }
- }
-
- svn_pool_destroy(iterpool);
+ return svn_error_createf(SVN_ERR_REPOS_VERIFY_FAILED, NULL,
+ _("Repository '%s' failed to verify"),
+ svn_dirent_local_style(
+ opt_state->repository_path, pool));
}
- return svn_error_trace(verify_err);
+ return SVN_NO_ERROR;
}
/* This implements `svn_opt_subcommand_t'. */
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py (revision 1685592)
+++ 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: E165011:.*"])
+
+ 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,22 @@ 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:.*",
+ "svnadmin: E165011:.*"])
+ else:
+ exp_err = \
+ svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*",
+ "svnadmin: E165011:.*"])
+
if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin
verify'.",
output, errput, exp_out, exp_err):
raise svntest.Failure
@@ -2110,8 +2128,20 @@ 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:.*",
+ "svnadmin: E165011:.*"])
+ else:
+ exp_err = \
+ svntest.verify.RegexListOutput([".*Error verifying revision 2.",
+ "svnadmin: E160004:.*",
+ "svnadmin: E160004:.*",
+ "svnadmin: E165011:.*"])
+
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 +2182,12 @@ def verify_keep_going_quiet(sbox):
".*Error verifying revision 3.",
"svnadmin: E160004:.*",
"svnadmin: E160004:.*",
- "svnadmin: E165011:.*"], False)
+ "svnadmin: E165011:.*"])
# 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 +2262,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,6 +2294,9 @@ def verify_invalid_path_changes(sbox):
if svntest.main.is_fs_log_addressing():
exp_out.insert(1, ".*Verifying.*metadata.*")
+ # ### I think that this test and verify_denormalized_names() both would
+ # benefit from explicit STDERR expectations, i.e., with match_all=True,
+ # but I skipped this part, as the patch is mostly an illustration.
exp_err = svntest.verify.RegexListOutput(["svnadmin: E160020:.*",
"svnadmin: E145001:.*",
"svnadmin: E160013:.*"], False)
@@ -2284,8 +2310,7 @@ def verify_invalid_path_changes(sbox):
sbox.repo_dir)
exp_out = svntest.verify.RegexListOutput([".*Verified revision 0.",
- ".*Verified revision 1.",
- ".*Error verifying revision 2."])
+ ".*Verified revision 1."])
exp_err = svntest.verify.RegexListOutput(["svnadmin: E160020:.*"], False)
if (svntest.main.fs_has_rep_sharing()):
@@ -2327,14 +2352,8 @@ def verify_denormalized_names(sbox):
".*Verified revision 1.",
".*Verified revision 2.",
".*Verified revision 3.",
- # A/{Eacute}/{aring}lpha
- "WARNING 0x0003: Duplicate representation of path 'A/.*/.*lpha'",
".*Verified revision 4.",
".*Verified revision 5.",
- # Q/{aring}lpha
- "WARNING 0x0004: Duplicate representation of path '/Q/.*lpha'"
- # A/{Eacute}
- " in svn:mergeinfo property of 'A/.*'",
".*Verified revision 6.",
".*Verified revision 7."]
@@ -2346,7 +2365,13 @@ def verify_denormalized_names(sbox):
expected_output_regex_list.insert(0, ".* Verifying metadata at revision
0.*")
exp_out = svntest.verify.RegexListOutput(expected_output_regex_list)
- exp_err = svntest.verify.ExpectedOutput([])
+ exp_err = svntest.verify.RegexListOutput(
+ # A/{Eacute}/{aring}lpha
+ ["WARNING 0x0003: Duplicate representation of path 'A/.*/.*lpha'",
+ # Q/{aring}lpha
+ "WARNING 0x0004: Duplicate representation of path '/Q/.*lpha'"
+ # A/{Eacute}
+ " in svn:mergeinfo property of 'A/.*'"])
svntest.verify.verify_outputs(
"Unexpected error while running 'svnadmin verify'.",
Index: subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
===================================================================
--- subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c (revision 1685592)
+++ subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c (working copy)
@@ -49,6 +49,20 @@ dont_filter_warnings(void *baton, svn_error_t *err
return;
}
+static void
+verify_notify_func(void *baton,
+ const svn_repos_notify_t *notify,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t **verify_err_p = baton;
+
+ if (notify->action == svn_repos_notify_failure)
+ {
+ *verify_err_p = svn_error_compose_create(*verify_err_p,
+ svn_error_dup(notify->err));
+ }
+}
+
/*** Test core code ***/
@@ -116,9 +130,10 @@ fuzzing_1_byte_1_rev(const char *repo_name,
SVN_ERR(svn_repos_open3(&repos, repo_name, fs_config, iterpool,
iterpool));
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);
+ /* This shall detect the corruption. */
+ SVN_ERR(svn_repos_verify_fs3(repos, revision, revision, TRUE, FALSE,
+ FALSE, verify_notify_func, &err,
+ 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 1685592)
+++ subversion/tests/libsvn_fs_fs/fs-fs-private-test.c (working copy)
@@ -361,6 +361,20 @@ receive_index(const svn_fs_fs__p2l_entry_t *entry,
return SVN_NO_ERROR;
}
+static void
+verify_notify_func(void *baton,
+ const svn_repos_notify_t *notify,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t **verify_err_p = baton;
+
+ if (notify->action == svn_repos_notify_failure)
+ {
+ *verify_err_p = svn_error_compose_create(*verify_err_p,
+ svn_error_dup(notify->err));
+ }
+}
+
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)
@@ -370,6 +384,7 @@ load_index_test(const svn_test_opts_t *opts, apr_p
apr_array_header_t *entries = apr_array_make(pool, 41, sizeof(void *));
apr_array_header_t *alt_entries = apr_array_make(pool, 1, sizeof(void *));
svn_fs_fs__p2l_entry_t entry;
+ svn_error_t *err;
/* Bail (with success) on known-untestable scenarios */
if (strcmp(opts->fs_type, "fsfs") != 0)
@@ -396,18 +411,18 @@ load_index_test(const svn_test_opts_t *opts, apr_p
entry.item.revision = SVN_INVALID_REVNUM;
APR_ARRAY_PUSH(alt_entries, svn_fs_fs__p2l_entry_t *) = &entry;
+ err = SVN_NO_ERROR;
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_ERR(svn_repos_verify_fs3(repos, rev, rev, keep_going, FALSE, FALSE,
+ verify_notify_func, &err, NULL, NULL, pool));
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_FS_INDEX_CORRUPTION);
/* Restore the original index. */
+ err = SVN_NO_ERROR;
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,
- NULL, NULL, NULL, NULL, pool));
+ verify_notify_func, &err, NULL, NULL, pool));
+ SVN_ERR(err);
return SVN_NO_ERROR;
}
