> 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.
Received on 2012-12-10 15:55:41 CET