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