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

On 12/20/2012 11:25 PM, Julian Foad wrote:
> Hi Prabhu.
>
> I have now looked in detail at your patch and tried using it. I think I have found an inconsistency and a serious problem.
>
> The output for a failed revision depends on whether --keep-going was passed. With --keep-going you print a "* Error verifying revision 2." line; without it you don't.
>
> Consistency of the output is important, but even more important is simplicity of the code. I would expect the code to look something like (pseudo-code):
>
> for R in range:
> err = verify_rev(R)
> if (err)
> show the error and a 'Failed' notification
> had_error = TRUE
> if not keep_going:
> break
>
> if had_error:
> return a 'Failed to verify the repo' error
>
> but
> you seem to have several different code paths. Why do you have two
> different places that generate the "Repository '%s' failed to verify"
> message; can you do it in just one place?

Yeah Julian, now the code generates the "Repository '%s' failed to
verify" message only in one place, thanks for the idea.

>
> Another justification for always printing the "* Error verifying revision 2." line is that with the old output:
>
> [[[
> $ svnadmin verify repo
> ...
> * Verified revision 15921.
> * Verified revision 15922.
> * Verified revision 15923.
> svnadmin: E160004: Invalid change kind in rev file
> ]]]
>
> it
> can be a little confusing at first glance to realize that the error
> relates to a revision number that was not mentioned -- revision 15924 in this example. I have often in the past wished that we would print a
> notification line like "* Error verifying revision 15924." here.
>
> Also, in email [1] I suggested
> that the final summary error should be printed even when keep-going is
> false. Did you consider that suggestion? What do you think?

Yeah I have taken that into consideration and implemented it as well.

<snip>

$ svnadmin verify repo2
* Verified revision 0.
* Verified revision 1.
* Error verifying revision 2.
svnadmin: E160004: Invalid change kind in rev file
svnadmin: E165005: Repository 'repo2' failed to verify

$ echo $?
1

</snip>

>
> A more serious problem: it doesn't always report the error at the end.
>
> [[[
> $ bin/svnadmin verify --keep-going repo2
> * Verified revision 0.
> * Verified revision 1.
> * Error verifying revision 2.
> subversion/libsvn_repos/replay.c:859: (apr_err=160004)
> subversion/libsvn_fs_fs/fs_fs.c:6335: (apr_err=160004)
> subversion/libsvn_fs_fs/fs_fs.c:6311: (apr_err=160004)
> subversion/libsvn_fs_fs/fs_fs.c:6242: (apr_err=160004)
> subversion/libsvn_fs_fs/fs_fs.c:6065: (apr_err=160004)
> svnadmin: E160004: Invalid change kind in rev file
> * Verified revision 3.
> ]]]
>
> The exit code is 0 here. Clearly a bug. The repository I used for this test is attached as 'jaf-corrupt-repo-2.tgz'.
Ahh, I missed out a code when moving my code from main.c to svnadmin.c
(sorry about that). This works fine now.

<snip>

$ svnadmin verify repo2 --keep-going
* Verified revision 0.
* Verified revision 1.
* Error verifying revision 2.
svnadmin: E160004: Invalid change kind in rev file
* Verified revision 3.
svnadmin: E165005: Repository 'repo2' failed to verify

$ echo $?
1

</snip>

> I am now sure that you need more tests. You added a test, but can you tell us what it tests in functional terms? It looks like it tests verifying a repo where there is an error in r6, and the youngest rev is r6. That doesn't cover the main purpose of the 'keep going' feature, which is to keep going, does it?
>
> Can you think of other things that should be tested? Please write a short list of tests that we should ideally have -- even if we might not write them all. It might start with something like:
>
> Scenario 1:
> Repo: youngest=r3, an error in r3.
> Command: svnadmin verify --keep-going repo
> Expected results:
> stdout: Nothing
> stderr: Success messages for r0, r1, r2.
> A 'revision failed' message for r3.
> At least one error message relating to r3.
> exit code: non-zero.
Scenario 2:
   Repo: youngest=r3, an error in r2
   Command: svnadmin verify repo
Expected results:
   stdout: nothing
   stderr: Success for r1
               Revision failed message for r2
               Error message relating to r2
               Repository %s failed to verify message
   exit code: non-zero

Scenario 3:
   Repo: youngest=r3, an error in r2
   Command: svnadmin verify repo --keep-going
Expected results:
   stdout: nothing
   stderr: Success for r1
               Revision failed message for r2
               Error message relating to r2
               Success for r3
               Repository %s failed to verify message
   exit code: non-zero

Scenario 4:
   Repo: youngest=r3, an error in r3
   Command: svnadmin verify repo -r1:2
Expected results:
   stdout: nothing
   stderr: Success for r1, r2
   exit code: zero

Scenario 5:
   Repo: youngest=r3, an error in r3
   Command: svnadmin verify repo -r1:2 --keep-going
Expected results:
   stdout: nothing
   stderr: Success for r1, r2
   exit code: zero

> Scenario 2:
> Repo: youngest=r5, error in r3 only (not affecting r4 or r5)
> Command: svnadmin verify --keep-going repo
> Expected results:
> ...
>
>
> In svn_repos.h: svn_repos_verify_fs3():
>
> /**
> * Verify the contents of the file system in @a repos.
> *
> * If @a feedback_stream is not @c NULL, write feedback to it (lines of
> * the form "* Verified revision %ld\n").
>
> This mentions what feedback the function gives. You are adding new feedback, so please update the doc string. It must be out of date already because the function doesn't have a 'feedback_stream' parameter, so please fix that at the same time.
Updated the doc string...

>
> * If @a start_rev is #SVN_INVALID_REVNUM, then start verifying at
> * revision 0. If @a end_rev is #SVN_INVALID_REVNUM, then verify
> * through the @c HEAD revision.
> *
> * For every verified revision call @a notify_func with @a rev set to
> * the verified revision and @a warning_text @c NULL. For warnings call @a
> * notify_func with @a warning_text set.
>
> Is that paragraph still correct or does it need updating?

Updated the paragraph...

>
> + * 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.
>
> Please document what this function returns (as I asked in [2]).
Yeah sure, documented the changes

>
> + * @since New in 1.8.
> + */
> +svn_error_t *
> +svn_repos_verify_fs3(svn_repos_t *repos, ...);
>
>
> In subversion/libsvn_repos/dump.c: svn_repos_verify_fs3():
>
> You are doing error detection and handling in two places in this function out of more than two possible places where an error could be thrown. As I wrote in [3], "Instead of checking for errors in several places within the main loop,
> put the verification code in a wrapper function and check for errors
> exactly once where you call that function. That way you won't miss
> any. In your last patch, there is at least one call that could return a
> verification error that isn't checked -- the call to
> svn_fs_revision_root()."
>

Yeah I get that, sounds a good idea :)
Now used a new wrapper function verification_checks() to perform all the
error checks, thus catching any possible error in one shot.

> In svnadmin.c:
> + 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));
>
> The message needs _() not just () around it for localization.

oops, I get it... Updated the code...

Attaching the updated patch and the log message with this mail. Please
share your views..

Thanks and regards
Prabhu

(The feature has been rightly name "keep-going", so does it :) )