Re: [PATCH] Implement svnadmin verify --keep-going

Thanks Daniel,

I have worked on the suggestions that you gave and am attaching the new
patch and log message with this mail. Please share your thoughts

Thanks and regards
Prabhu

On 12/10/2012 08:24 PM, Daniel Shahaf wrote:
>> 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.