Hi Prabhu,
below is my review of the changes on the verify-keep-going branch.
Most of my comments point out minor formatting issues. I believe
the test part is the only code that might need some adjustment.
Otherwise it looks very good to me.
> Index: BRANCH-README
> ===================================================================
> --- BRANCH-README (.../trunk) (revision 0)
> +++ BRANCH-README (.../branches/verify-keep-going) (revision 1492172)
> @@ -0,0 +1,62 @@
> +This branch exists to add --keep-going to 'svnadmin verify'.
> +The goal of this flag is to continue the repository verification process even
> +after detecting a failure/corruption.
> +
> +USAGE:
> +------
> +svnadmin verify --keep-going /path/to/repo
> +
> +TODO:
> +-----
> +1. Add a few more test cases.
> +
> +
> +2. Show a summary of corruptions by default. So the svnadmin verify
> + --keep-going would look something like,
> +
> +<MOCK>
> +
> +$ svnadmin verify --keep-going myrepo
> +* Verifying repository metadata ...
> +* Verified revision 0.
> +* Verified revision 1.
> +* Verified revision 2.
> +.
> +.
> +.
> +* Error verifying revision 11.
> +svnadmin: E160013: File not found: revision 11, path '/Trunk'
> +* Verified revision 12.
> +* Verified revision 13.
> +.
> +.
> +.
> +* Error verifying revision 112.
> +svnadmin: E160004: Invalid change kind in rev file
> +* Verified revision 113.
> +
> +Summary of corruptions
> +----------------------
> +Total corruptions: 2
> +rev11: svnadmin: E160013: File not found: revision 1, path '/Trunk'
> +rev112: svnadmin: E160004: Invalid change kind in rev file
> +svnadmin: E165005: Repository '/tmp/myrepo' failed to verify
> +
> +</MOCK>
> +
> +
> +3. If --keep-going is used along with --quiet option, show only the corruption
> + summary so that it would look like,
> +
> +<MOCK>
> +
> +$ svnadmin verify --keep-going --quiet myrepo
> +
> +Summary of corruptions
> +----------------------
> +Total corruptions: 2
> +rev11: svnadmin: E160013: File not found: revision 1, path '/Trunk'
>
> I think the output should say 'r11' or 'Revision 11', not 'rev11'.
>
> +rev112: svnadmin: E160004: Invalid change kind in rev file
> +svnadmin: E165005: Repository '/tmp/myrepo' failed to verify
> +
> +</MOCK>
> Index: subversion/tests/cmdline/svnadmin_tests.py
> ===================================================================
> --- subversion/tests/cmdline/svnadmin_tests.py (.../trunk) (revision 1491841)
> +++ subversion/tests/cmdline/svnadmin_tests.py (.../branches/verify-keep-going) (revision 1492172)
> @@ -1826,6 +1826,101 @@ def recover_old(sbox):
> svntest.main.run_svnadmin("recover", sbox.repo_dir)
>
>
> +def verify_keep_going(sbox):
> + "svnadmin verify --keep-going test"
> +
> + sbox.build(create_wc = False)
Creates a repository with current format...
> + repo_url = sbox.repo_url
> + B_url = sbox.repo_url + '/B'
> + C_url = sbox.repo_url + '/C'
> +
> + # Create A/B/E/bravo in r2.
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'mkdir', '-m', 'log_msg',
> + B_url)
> +
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'mkdir', '-m', 'log_msg',
> + C_url)
> +
> + r2 = fsfs_file(sbox.repo_dir, 'revs', '2')
> + fp = open(r2, 'wb')
... writes rev file from format 5 (or 6)?
The rev file may not match future formats.
Perhaps corrupt r2 in a different way, like writing a small block
of random data or zeroes to it somewhere?
> + fp.write("""id: 0-2.0.r2/0
> +type: dir
> +count: 0
> +cpath: /B
> +copyroot: 0 /
> +
> +PLAIN
> +K 1
> +A
> +V 17
> +dir 0-1.0.r1/3837
> +K 1
> +B
> +V 14
> +dir 0-2.0.r2/0
> +K 4
> +iota
> +V 19
> +file 11-1.0.r1/3951
> +END
> +ENDREP
> +id: 0.0.r2/165
> +type: dir
> +pred: 0.0.r1/4198
> +count: 2
> +text: 2 59 93 0 ae352a67fd07433f009f7234d2ea47ac
> +cpath: /
> +copyroot: 0 /
> +
> +_0.0.t1-1 Add-dir false false /B
> +
> +
> +165 290
> +""")
> + fp.close()
> + exit_code, output, errput = svntest.main.run_svnadmin("verify",
> + "--keep-going",
> + sbox.repo_dir)
> +
> + exp_out = svntest.verify.RegexListOutput([".*Verifying repository metadata",
> + ".*Verified revision 0.",
> + ".*Verified revision 1.",
> + ".*Error verifying revision 2.",
> + ".*Verified revision 3."])
> +
> + exp_err = svntest.verify.RegexListOutput(["svnadmin: E160004:.*",
> + "svnadmin: E165011:.*"])
> +
> + if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin verify'.",
> + output, errput, exp_out, exp_err):
> + raise svntest.Failure
> +
> + exit_code, output, errput = svntest.main.run_svnadmin("verify",
> + sbox.repo_dir)
> +
> + exp_out = svntest.verify.RegexListOutput([".*Verifying repository metadata",
> + ".*Verified revision 0.",
> + ".*Verified revision 1.",
> + ".*Error verifying revision 2."])
> +
> + exp_err = svntest.verify.RegexListOutput(["svnadmin: E160004:.*",
> + "svnadmin: E165011:.*"])
> +
> + if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin verify'.",
> + output, errput, exp_out, exp_err):
> + raise svntest.Failure
> +
> +
> + exit_code, output, errput = svntest.main.run_svnadmin("verify",
> + "--quiet",
> + sbox.repo_dir)
> +
> + if svntest.verify.verify_outputs("Output of 'svnadmin verify' is unexpected.",
> + None, errput, None, "svnadmin: E165011:.*"):
> + raise svntest.Failure
> +
> ########################################################################
> # Run the tests
>
> @@ -1862,6 +1957,7 @@ test_list = [ None,
> locking,
> mergeinfo_race,
> recover_old,
> + verify_keep_going,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svnadmin/svnadmin.c
> ===================================================================
> --- subversion/svnadmin/svnadmin.c (.../trunk) (revision 1491841)
> +++ subversion/svnadmin/svnadmin.c (.../branches/verify-keep-going) (revision 1492172)
> @@ -180,6 +180,7 @@ enum svnadmin__cmdline_options_t
> {
> svnadmin__version = SVN_OPT_FIRST_LONGOPT_ID,
> svnadmin__incremental,
> + svnadmin__keep_going,
> svnadmin__deltas,
> svnadmin__ignore_uuid,
> svnadmin__force_uuid,
> @@ -288,6 +289,9 @@ static const apr_getopt_option_t options_table[] =
> {"pre-1.6-compatible", svnadmin__pre_1_6_compatible, 0,
> N_("deprecated; see --compatible-version")},
>
> + {"keep-going", svnadmin__keep_going, 0,
> + N_("continue verifying after detecting a corruption")},
> +
Perhaps say "continue verification" instead of "continue verifying"?
> {"memory-cache-size", 'M', 1,
> N_("size of the extra in-memory cache in MB used to\n"
> " minimize redundant operations. Default: 16.\n"
> @@ -491,7 +495,7 @@ static const svn_opt_subcommand_desc2_t cmd_table[
> {"verify", subcommand_verify, {0}, N_
> ("usage: svnadmin verify REPOS_PATH\n\n"
> "Verify the data stored in the repository.\n"),
> - {'t', 'r', 'q', 'M'} },
> + {'t', 'r', 'q', svnadmin__keep_going, 'M'} },
>
> { NULL, NULL, {0}, NULL, {0} }
> };
> @@ -522,6 +526,7 @@ struct svnadmin_opt_state
> svn_boolean_t clean_logs; /* --clean-logs */
> svn_boolean_t bypass_hooks; /* --bypass-hooks */
> svn_boolean_t wait; /* --wait */
> + svn_boolean_t keep_going; /* --keep-going */
> svn_boolean_t bypass_prop_validation; /* --bypass-prop-validation */
> enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
> --force-uuid */
> @@ -822,6 +827,16 @@ repos_notify_handler(void *baton,
> notify->warning_str);
> return;
>
> + case svn_repos_notify_failure:
> + if (notify->revision != SVN_INVALID_REVNUM)
> + cmdline_stream_printf(feedback_stream, scratch_pool,
> + _("* Error verifying revision %ld.\n"),
> + notify->revision);
Indentation off by one:
cmdline_stream_printf(feedback_stream, scratch_pool,
-> _("* Error verifying revision %ld.\n"),
-> notify->revision);
> + if (notify->err)
> + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> + "svnadmin: ");
> + return;
> +
> case svn_repos_notify_dump_rev_end:
> cmdline_stream_printf(feedback_stream, scratch_pool,
> _("* Dumped revision %ld.\n"),
> @@ -1657,10 +1672,12 @@ subcommand_verify(apr_getopt_t *os, void *baton, a
> if (! opt_state->quiet)
> progress_stream = recode_stream_create(stdout, pool);
>
> - return svn_repos_verify_fs2(repos, lower, upper,
> - !opt_state->quiet
> - ? repos_notify_handler : NULL,
> - progress_stream, check_cancel, NULL, pool);
> + return svn_error_trace(svn_repos_verify_fs3(repos, lower, upper,
> + opt_state->keep_going,
> + !opt_state->quiet
> + ? repos_notify_handler : NULL,
> + progress_stream, check_cancel,
> + NULL, pool));
> }
>
> /* This implements `svn_opt_subcommand_t'. */
> @@ -2281,6 +2298,9 @@ sub_main(int argc, const char *argv[], apr_pool_t
> opt_state.compatible_version = compatible_version;
> }
> break;
> + case svnadmin__keep_going:
> + opt_state.keep_going = TRUE;
> + break;
> case svnadmin__fs_type:
> SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_state.fs_type, opt_arg, pool));
> break;
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (.../trunk) (revision 1491841)
> +++ subversion/include/svn_error_codes.h (.../branches/verify-keep-going) (revision 1492172)
> @@ -850,6 +850,11 @@ SVN_ERROR_START
> SVN_ERR_REPOS_CATEGORY_START + 10,
> "Repository upgrade is not supported")
>
> + /** @since New in 1.8. */
> + SVN_ERRDEF(SVN_ERR_REPOS_CORRUPTED,
> + SVN_ERR_REPOS_CATEGORY_START + 11,
> + "Repository has corruptions")
Perhaps say "Repository is corrupt"?
> +
> /* generic RA errors */
>
> SVN_ERRDEF(SVN_ERR_RA_ILLEGAL_URL,
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (.../trunk) (revision 1491841)
> +++ subversion/include/svn_repos.h (.../branches/verify-keep-going) (revision 1492172)
> @@ -251,8 +251,11 @@ typedef enum svn_repos_notify_action_t
> svn_repos_notify_load_skipped_rev,
>
> /** The structure of a revision is being verified. @since New in 1.8. */
> - svn_repos_notify_verify_rev_structure
> + svn_repos_notify_verify_rev_structure,
>
> + /** A revision is found with corruption/errors. @since New in 1.8. */
> + svn_repos_notify_failure
> +
> } svn_repos_notify_action_t;
>
> /** The type of error occurring.
> @@ -323,6 +326,11 @@ 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.8. */
New in 1.9!
> + svn_error_t *err;
> +
> /* NOTE: Add new fields at the end to preserve binary compatibility.
> Also, if you add fields here, you have to update
> svn_repos_notify_create(). */
> @@ -2592,12 +2600,39 @@ svn_repos_info_format(int *repos_format,
> * the verified revision and @a warning_text @c NULL. For warnings call @a
> * notify_func with @a warning_text set.
> *
> + * For every revision verification failure, if @a notify_func is not @c NULL,
> + * call @a notify_func with @a rev set to the corrupt revision and @err set to
> + * the corresponding error message.
> + *
> * If @a cancel_func is not @c NULL, call it periodically with @a
> * cancel_baton as argument to see if the caller wishes to cancel the
> * verification.
> *
> + * If @a keep_going is @c TRUE, the verify process notifies the error message
> + * and continues. If @a notify_func is @c NULL, the verification failure is
> + * not notified. Finally, return an error if there were any failures during
> + * verification, or SVN_NO_ERROR if there were no failures.
> + *
> + * @since New in 1.8.
New in 1.9.
> + */
> +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_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel,
> + void *cancel_baton,
> + apr_pool_t *scratch_pool);
> +
> +/**
> + * Like svn_repos_verify_fs3(), but with @a keep_going set to @c FALSE.
> + *
> * @since New in 1.7.
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> +SVN_DEPRECATED
> svn_error_t *
> svn_repos_verify_fs2(svn_repos_t *repos,
> svn_revnum_t start_rev,
> Index: subversion/libsvn_repos/deprecated.c
> ===================================================================
> --- subversion/libsvn_repos/deprecated.c (.../trunk) (revision 1491841)
> +++ subversion/libsvn_repos/deprecated.c (.../branches/verify-keep-going) (revision 1492172)
> @@ -727,6 +727,27 @@ svn_repos_dump_fs2(svn_repos_t *repos,
> }
>
> svn_error_t *
> +svn_repos_verify_fs2(svn_repos_t *repos,
> + svn_revnum_t start_rev,
> + svn_revnum_t end_rev,
> + svn_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)
> +{
> + return svn_error_trace(svn_repos_verify_fs3(repos,
> + start_rev,
> + end_rev,
> + FALSE,
> + notify_func,
> + notify_baton,
> + cancel_func,
> + cancel_baton,
> + pool));
> +}
> +
> +svn_error_t *
> svn_repos_verify_fs(svn_repos_t *repos,
> svn_stream_t *feedback_stream,
> svn_revnum_t start_rev,
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (.../trunk) (revision 1491841)
> +++ subversion/libsvn_repos/dump.c (.../branches/verify-keep-going) (revision 1492172)
> @@ -1332,6 +1332,71 @@ verify_close_directory(void *dir_baton,
> 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)
> +{
> + if (notify_func)
> + {
> + svn_repos_notify_t *notify_failure;
> + 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);
> + }
Perhaps write this function like this?
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,
> + svn_revnum_t rev,
> + svn_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_revnum_t start_rev,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *scratchpool)
Please name the scratch pool like this, as elsewhere; scratch_pool
> +{
> + const svn_delta_editor_t *dump_editor;
> + void *dump_edit_baton;
> +
> + svn_fs_root_t *to_root;
> + apr_hash_t *props;
> + const svn_delta_editor_t *cancel_editor;
> + void *cancel_edit_baton;
> +
> + /* Get cancellable dump editor, but with our close_directory handler.*/
> + SVN_ERR(get_dump_editor(&dump_editor, &dump_edit_baton,
> + fs, rev, "",
> + svn_stream_empty(scratchpool),
> + NULL, NULL,
> + verify_close_directory,
> + notify_func, notify_baton,
> + start_rev,
> + FALSE, TRUE, /* use_deltas, verify */
> + scratchpool));
> + SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
> + dump_editor, dump_edit_baton,
> + &cancel_editor,
> + &cancel_edit_baton,
> + scratchpool));
> + SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, scratchpool));
> + SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
> + cancel_editor, cancel_edit_baton,
> + NULL, NULL, scratchpool));
> +
> + /* While our editor close_edit implementation is a no-op, we still
> + do this for completeness. */
> + SVN_ERR(cancel_editor->close_edit(cancel_edit_baton, scratchpool));
> +
> + SVN_ERR(svn_fs_revision_proplist(&props, fs, rev, scratchpool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* Baton type used for forwarding notifications from FS API to REPOS API. */
> struct verify_fs2_notify_func_baton_t
> {
> @@ -1359,9 +1424,10 @@ verify_fs2_notify_func(svn_revnum_t revision,
> }
>
> svn_error_t *
> -svn_repos_verify_fs2(svn_repos_t *repos,
> +svn_repos_verify_fs3(svn_repos_t *repos,
> svn_revnum_t start_rev,
> svn_revnum_t end_rev,
> + svn_boolean_t keep_going,
> svn_repos_notify_func_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> @@ -1376,6 +1442,9 @@ svn_error_t *
> svn_fs_progress_notify_func_t verify_notify = NULL;
> struct verify_fs2_notify_func_baton_t *verify_notify_baton = NULL;
>
> + svn_error_t *err;
> + svn_boolean_t found_corruption = FALSE;
> +
> /* Determine the current youngest revision of the filesystem. */
> SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
>
> @@ -1413,11 +1482,30 @@ svn_error_t *
> }
>
> /* Verify global metadata and backend-specific data first. */
> - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), svn_fs_config(fs, pool),
> - start_rev, end_rev,
> - verify_notify, verify_notify_baton,
> - cancel_func, cancel_baton, pool));
> + err = svn_fs_verify(svn_fs_path(fs, pool), svn_fs_config(fs, pool),
> + start_rev, end_rev,
> + verify_notify, verify_notify_baton,
> + cancel_func, cancel_baton, pool);
>
> + if (err && !keep_going)
> + {
> + found_corruption = TRUE;
> + notify_verification_error(SVN_INVALID_REVNUM, err, notify_func,
> + notify_baton, iterpool);
> + svn_error_clear(err);
> + return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
> + _("Repository '%s' failed to verify"),
> + svn_dirent_local_style(svn_repos_path(repos,
> + pool),
> + pool));
> + }
> + else
> + {
> + if (err)
> + found_corruption = TRUE;
> + svn_error_clear(err);
> + }
> +
> for (rev = start_rev; rev <= end_rev; rev++)
> {
> const svn_delta_editor_t *dump_editor;
> @@ -1427,36 +1515,26 @@ svn_error_t *
> svn_fs_root_t *to_root;
> apr_hash_t *props;
>
> + svn_error_t *err;
> svn_pool_clear(iterpool);
Please swap the blank linke with the line declaring 'err':
svn_fs_root_t *to_root;
apr_hash_t *props;
svn_error_t *err;
svn_pool_clear(iterpool);
>
> - /* Get cancellable dump editor, but with our close_directory handler. */
> - SVN_ERR(get_dump_editor(&dump_editor, &dump_edit_baton,
> - fs, rev, "",
> - svn_stream_empty(iterpool),
> - NULL, NULL,
> - verify_close_directory,
> - notify_func, notify_baton,
> - start_rev,
> - FALSE, TRUE, /* use_deltas, verify */
> - iterpool));
> - SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
> - dump_editor, dump_edit_baton,
> - &cancel_editor,
> - &cancel_edit_baton,
> - iterpool));
> + /* Wrapper function to catch the possible errors. */
> + err = verify_one_revision(fs, rev, notify_func, notify_baton, start_rev,
> + cancel_func, cancel_baton, iterpool);
>
> - SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, iterpool));
> - SVN_ERR(svn_fs_verify_root(to_root, iterpool));
> + if (err)
> + {
> + found_corruption = TRUE;
> + notify_verification_error(rev, err, notify_func, notify_baton,
> + iterpool);
> + svn_error_clear(err);
>
> - SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
> - cancel_editor, cancel_edit_baton,
> - NULL, NULL, iterpool));
> - /* While our editor close_edit implementation is a no-op, we still
> - do this for completeness. */
> - SVN_ERR(cancel_editor->close_edit(cancel_edit_baton, iterpool));
> + if (keep_going)
> + continue;
> + else
> + break;
> + }
>
> - SVN_ERR(svn_fs_revision_proplist(&props, fs, rev, iterpool));
> -
> if (notify_func)
> {
> notify->revision = rev;
> @@ -1474,5 +1552,12 @@ svn_error_t *
> /* Per-backend verification. */
> svn_pool_destroy(iterpool);
>
> + if (found_corruption)
> + return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
> + _("Repository '%s' failed to verify"),
> + svn_dirent_local_style(svn_repos_path(repos,
> + pool),
> + pool));
> +
> return SVN_NO_ERROR;
> }
> Index: tools/dist/make-deps-tarball.sh
> ===================================================================
> --- tools/dist/make-deps-tarball.sh (.../trunk) (revision 1491841)
> +++ tools/dist/make-deps-tarball.sh (.../branches/verify-keep-going) (revision 1492172)
>
> Property changes on: tools/dist/make-deps-tarball.sh
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /subversion/trunk/tools/dist/make-deps-tarball.sh:r1439280-1488969
> Index: .
> ===================================================================
> --- . (.../trunk) (revision 1491841)
> +++ . (.../branches/verify-keep-going) (revision 1492172)
>
> Property changes on: .
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /subversion/trunk:r1439280-1491841
Received on 2013-06-12 15:52:21 CEST