> Index: subversion/tests/cmdline/svnadmin_tests.py > =================================================================== > --- subversion/tests/cmdline/svnadmin_tests.py (revision 1411074) > +++ subversion/tests/cmdline/svnadmin_tests.py (working copy) > @@ -1835,6 +1835,114 @@ > svntest.main.run_svnadmin("recover", sbox.repo_dir) > > > +def verify_keep_going(sbox): > + "svnadmin verify --keep-going test" > + test_create(sbox) > + dumpfile_skeleton = open(os.path.join(os.path.dirname(sys.argv[0]), > + > 'svnadmin_tests_data', > + > 'skeleton_repos.dump')).read() > + load_dumpstream(sbox, dumpfile_skeleton, '--ignore-uuid') > + > + r2 = fsfs_file(sbox.repo_dir, 'revs', '6') > + fp = open(r2, 'wb') > + fp.write("""id: 3-6.0.r6/0
This test will fail when building with -DSVN_FS_FS_DEFAULT_MAX_FILES_PER_DIR=3 -DPACK_AFTER_EVERY_COMMIT . Can it recognise the situation and Skip? (It's fine if it can't: svnadmin_tests 17 has always FAILed with those flags.) > +""") > + fp.close() > + exit_code, output, errput = svntest.main.run_svnadmin("verify", > + sbox.repo_dir) > + exit_code, output, errput2 = svntest.main.run_svnadmin("verify", > + "--keep-going", > + sbox.repo_dir) > + > + if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin > verify'.", > + [], errput, None, ".*svnadmin: E165005: > .*"): > + raise svntest.Failure > + > + if svntest.verify.verify_outputs("Unexpected error while running 'svnadmin > verify'.", > + [], errput2, None, > + ["* Verified revision 0.\n", > + "* Verified revision 1.\n", > + "* Verified revision 2.\n", > + "* Verified revision 3.\n", Please avoid testing the literal output. It means every single change to the progress reporting or error reporting will need the test to be updated. > + "* Verified revision 4.\n", > + "* Verified revision 5.\n", > + "* Error verifying revision 6.\n", > + "svnadmin: E200004: Could not convert '' > into a number\n", It might be useful to add a comment here explaining the error --- it's because the last line in the revision file is blank. Alternatively, you could make that line contain a sentinel string and then check that the sentinel appears in the error message; that would be self-documenting. > + "svnadmin: E165005: Repository > 'svn-test-work/repositories/svnadmin_tests-31' failed to verify\n"]): > + raise svntest.Failure > + > + {"keep-going", svnadmin__keep_going, 0, > + N_("continue verifying even if there is a corruption")}, s/even if there is/after detecting/ ? > @@ -744,6 +749,21 @@ > 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)); > +/* svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool, > + _("svnadmin: E%d: Error verifying > revision %ld\n"), > + notify->err->apr_err, > + notify->revision)); > +*/ This debris doesn't belong in the patch. :) > /** 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. */ Add @since please to the docstring. > + svn_error_t *err; > + > Index: subversion/libsvn_repos/dump.c > =================================================================== > --- subversion/libsvn_repos/dump.c (revision 1411074) > +++ subversion/libsvn_repos/dump.c (working copy) > @@ -1359,10 +1359,28 @@ > return close_directory(dir_baton, pool); > } > > +void 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); > + } > +} > + > 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, > @@ -1374,6 +1392,8 @@ > svn_revnum_t rev; > apr_pool_t *iterpool = svn_pool_create(pool); > svn_repos_notify_t *notify; > + 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)); > @@ -1397,8 +1417,25 @@ > end_rev, youngest); > > /* Verify global/auxiliary data and backend-specific data first. */ > - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton, > - start_rev, end_rev, pool)); > + err = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton, > + start_rev, end_rev, pool); > + if (err) > + { > + rev = SVN_INVALID_REVNUM; > + if (!keep_going) Maybe I'm missing something, but isn't the condition inverted or redundant? > + notify_verification_error(rev, err, notify_func, notify_baton, > + iterpool); > + found_corruption = TRUE; > + svn_error_clear(err); > + if (!keep_going) > + 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 > + SVN_ERR(err); This else block is redundant. > > /* Create a notify object that we can reuse within the loop. */ > if (notify_func) > @@ -1433,9 +1482,20 @@ Can you generate diffs with function name headers (via 'svn diff -x-p')? > +++ subversion/libsvn_repos/notify.c (working copy) > @@ -39,6 +39,7 @@ > svn_repos_notify_t *notify = apr_pcalloc(result_pool, sizeof(*notify)); > > notify->action = action; > + notify->err = SVN_NO_ERROR; That's redundant; apr_pcalloc() does that for you. > > return notify; > } Looks good otherwise, though I did more of a line-by-line review than a codepath-oriented review --- i.e., further eyes (on this or next revision of the patch) welcome.