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

[Stefan Sperling]

Prabhu, any news on this? I would like to see this feature committed.
Is there anything I could do to help move it along?

Julian, do you think Prabhu's latest patch is ready for commit,
provided Prabhu or I take care of any unaddressed concerns in
follow-up patches? Or would committing it to trunk in its current
state be too disruptive? If that is the case, perhaps we should
offer Prabhu commit access to a branch where he could work on this
feature more comfortably until it has been completed and is ready
to be reintegrated into trunk?

Thanks!

On Fri, Jan 04, 2013 at 03:03:11PM +, Julian Foad wrote:
 Thanks for this version, Prabhu.  It looks much better.  Still a few more 
 points...
 
 
 Prabhu Gnana Sundar prabh...@collab.net wrote:
  On 12/20/2012 11:25 PM, Julian Foad wrote:
  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.
 
 I can still see this error message being returned in two different places in 
 your patch.  (Maybe there were three before; I haven't checked.)  But it's 
 much better now because they are both in the same function and  at the same 
 level, and that's fine.  I suggest you leave it like that.
 
 In the first of these places:
 
     /* 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));
  +  if (err  !keep_going)
  +    {
  +  rev = SVN_INVALID_REVNUM;
  +  found_corruption = TRUE;
 
 (Redundant assignment as you're about to return -- remove it.)
 
  +  notify_verification_error(rev, 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(..., pool));
  +    }
  +  else
  +    {
  +  if (err)
  +    found_corruption = TRUE;
  +  svn_error_clear(err);
  +    }
 
 Why only notify the error if we're stopping but not if we're keeping going?  
 That seems wrong.
 
 Further on, in the main loop:
 
  +  if (err  keep_going)
  +    {
  +  found_corruption = TRUE;
  +  notify_verification_error(rev, err, notify_func, notify_baton,
  +    iterpool);
  +  svn_error_clear(err);
  +  continue;
  +    }
  +  else
  +    if (err)
  +  {
  +    found_corruption = TRUE;
  +    notify_verification_error(rev, err, notify_func, notify_baton,
  +  iterpool);
  +    svn_error_clear(err);
  +    break;
  +  }
 
 Is there any difference between these the if branch and the else branch 
 apart from the last line?  Simplify to:
 
    if (err)
      ...
      if (keep_going)
        continue;
  else
    break;
 
 
  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.
 
 Great -- thanks.
 
  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.
  [...]
  svnadmin: E160004: Invalid change kind in rev file
  * Verified revision 3.
  ]]]
  
  The 

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

[Julian Foad]

Stefan Sperling wrote:

 Prabhu, any news on this? I would like to see this feature committed.
 Is there anything I could do to help move it along?
 
 Julian, do you think Prabhu's latest patch is ready for commit,
 provided Prabhu or I take care of any unaddressed concerns in
 follow-up patches? Or would committing it to trunk in its current
 state be too disruptive? If that is the case, perhaps we should
 offer Prabhu commit access to a branch where he could work on this
 feature more comfortably until it has been completed and is ready
 to be reintegrated into trunk?

What holds me back from committing it is lack of a test that at least exercises 
the 'keep-going' functionality (that is, reports on another revision after 
reporting a failed revision) and lack of evidence that it has been tested and 
found to work as expected in a few different failure scenarios.  I tried some 
quick tests by hand with a previous version of the patch and found some wrong 
behaviour, and haven't tested the latest version at all.  Mainly I am unwilling 
to commit myself to ensuring that necessary testing and follow-ups do in fact 
happen.

The other issues I raised are not blockers for me and could be addressed in 
follow-up commits.  I wouldn't veto you or anyone else committing it in its 
current state with follow-ups afterwards.

Prabhu is welcome to work on this on a branch from my point of view, if that 
would help.

Prabhu, you might also find it helpful to ask for help if you're having 
difficulty with some part.  For example, if you find it hard to write tests for 
it, it's quite possible that someone here would be willing to help or even do 
that for you.  (In fact it's good engineering practice for someone else to 
write the tests.)  Don't be shy of asking.

- Julian

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

[Prabhu Gnana Sundar]

Julian Foad julianf...@btopenworld.com wrote:

What holds me back from committing it is lack of a test that at least
exercises the 'keep-going' functionality (that is, reports on another
revision after reporting a failed revision) and lack of evidence that
it has been tested and found to work as expected in a few different
failure scenarios.  I tried some quick tests by hand with a previous
version of the patch and found some wrong behaviour, and haven't tested
the latest version at all.  Mainly I am unwilling to commit myself to
ensuring that necessary testing and follow-ups do in fact happen.


Thanks Julian. I have now handled the errors that can occur while 
verifying-one-revision in a single place, which would evade falling into wrong 
behaviour. I am still writing test cases to test a few possible scenarios.


The other issues I raised are not blockers for me and could be
addressed in follow-up commits.  I wouldn't veto you or anyone else
committing it in its current state with follow-ups afterwards.

Prabhu is welcome to work on this on a branch from my point of view, if
that would help.


Sure, that would be more comfortable to work on the feature.

Prabhu, you might also find it helpful to ask for help if you're having
difficulty with some part.  For example, if you find it hard to write
tests for it, it's quite possible that someone here would be willing to
help or even do that for you.  (In fact it's good engineering practice
for someone else to write the tests.)  Don't be shy of asking.



Sure Julian..


Regards
Prabhu

--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

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

[Prabhu Gnana Sundar]

Stefan Sperling s...@elego.de wrote:

Prabhu, any news on this? I would like to see this feature committed.
Is there anything I could do to help move it along?


Thank you Stefan...
 I am working on this closely. I have been getting great help from Julian and 
am working on his last suggestions and improving the current test case.


Thanks and regards
Prabhu



Julian, do you think Prabhu's latest patch is ready for commit,
provided Prabhu or I take care of any unaddressed concerns in
follow-up patches? Or would committing it to trunk in its current
state be too disruptive? If that is the case, perhaps we should
offer Prabhu commit access to a branch where he could work on this
feature more comfortably until it has been completed and is ready
to be reintegrated into trunk?

Thanks!

On Fri, Jan 04, 2013 at 03:03:11PM +, Julian Foad wrote:
 Thanks for this version, Prabhu.  It looks much better.  Still a few
more points...


 Prabhu Gnana Sundar prabh...@collab.net wrote:
  On 12/20/2012 11:25 PM, Julian Foad wrote:
  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.

 I can still see this error message being returned in two different
places in your patch.  (Maybe there were three before; I haven't
checked.)  But it's much better now because they are both in the same
function and  at the same level, and that's fine.  I suggest you leave
it like that.

 In the first of these places:

     /* 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));
  +  if (err  !keep_going)
  +    {
  +  rev = SVN_INVALID_REVNUM;
  +  found_corruption = TRUE;

 (Redundant assignment as you're about to return -- remove it.)

  +  notify_verification_error(rev, 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(..., pool));
  +    }
  +  else
  +    {
  +  if (err)
  +    found_corruption = TRUE;
  +  svn_error_clear(err);
  +    }

 Why only notify the error if we're stopping but not if we're keeping
going?  That seems wrong.

 Further on, in the main loop:

  +  if (err  keep_going)
  +    {
  +  found_corruption = TRUE;
  +  notify_verification_error(rev, err, notify_func,
notify_baton,
  +    iterpool);
  +  svn_error_clear(err);
  +  continue;
  +    }
  +  else
  +    if (err)
  +  {
  +    found_corruption = TRUE;
  +    notify_verification_error(rev, err, notify_func,
notify_baton,
  +  iterpool);
  +    svn_error_clear(err);
  +    break;
  +  }

 Is there any difference between these the if branch and the else
branch apart from the last line?  Simplify to:

    if (err)
      ...
      if (keep_going)
        continue;
  else
    break;


  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.

 Great -- thanks.

  A more serious problem: it doesn't always report the error at the
end.
 
  [[[
  $ bin/svnadmin verify 

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

[Julian Foad]

Thanks for this version, Prabhu.  It looks much better.  Still a few more 
points...


Prabhu Gnana Sundar prabh...@collab.net wrote:
 On 12/20/2012 11:25 PM, Julian Foad wrote:
 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.

I can still see this error message being returned in two different places in 
your patch.  (Maybe there were three before; I haven't checked.)  But it's much 
better now because they are both in the same function and  at the same level, 
and that's fine.  I suggest you leave it like that.

In the first of these places:

    /* 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));
 +  if (err  !keep_going)
 +    {
 +  rev = SVN_INVALID_REVNUM;
 +  found_corruption = TRUE;

(Redundant assignment as you're about to return -- remove it.)

 +  notify_verification_error(rev, 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(..., pool));
 +    }
 +  else
 +    {
 +  if (err)
 +    found_corruption = TRUE;
 +  svn_error_clear(err);
 +    }

Why only notify the error if we're stopping but not if we're keeping going?  
That seems wrong.

Further on, in the main loop:

 +  if (err  keep_going)
 +    {
 +  found_corruption = TRUE;
 +  notify_verification_error(rev, err, notify_func, notify_baton,
 +    iterpool);
 +  svn_error_clear(err);
 +  continue;
 +    }
 +  else
 +    if (err)
 +  {
 +    found_corruption = TRUE;
 +    notify_verification_error(rev, err, notify_func, notify_baton,
 +  iterpool);
 +    svn_error_clear(err);
 +    break;
 +  }

Is there any difference between these the if branch and the else branch 
apart from the last line?  Simplify to:

   if (err)
     ...
     if (keep_going)
       continue;
 else
   break;


 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.

Great -- thanks.

 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.
 [...]
 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.

Good.


 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 

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

[Prabhu Gnana Sundar]

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


  

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

[Prabhu Gnana Sundar]

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: E24: 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 = 

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

[Daniel Shahaf]

 Index: subversion/libsvn_repos/dump.c
 ===
 --- subversion/libsvn_repos/dump.c(revision 1420101)
 +++ subversion/libsvn_repos/dump.c(working copy)
 @@ -1413,19 +1452,31 @@
void *cancel_edit_baton;
svn_fs_root_t *to_root;
apr_hash_t *props;
 +  svn_error_t *err;

Pretty sure you'll get a -Wshadow compiler warning here.  You should
ensure patches don't add compiler warnings.

 @@ -1433,9 +1484,20 @@
  iterpool));
  
SVN_ERR(svn_fs_revision_root(to_root, fs, rev, iterpool));
 -  SVN_ERR(svn_repos_replay2(to_root, , SVN_INVALID_REVNUM, FALSE,
 -cancel_editor, cancel_edit_baton,
 -NULL, NULL, iterpool));
 +  err = svn_repos_replay2(to_root, , SVN_INVALID_REVNUM, FALSE,
 +  cancel_editor, cancel_edit_baton,
 +  NULL, NULL, iterpool);
 +
 +  if (err  keep_going)
 +{
 +  notify_verification_error(rev, err, notify_func, notify_baton,
 +iterpool);
 +  svn_error_clear(err);

Hmm.  What if this is a malfunction error?  (Compare get_shared_rep() in
fs_fs.c)

Maybe we should introduce:

#define svn_error_clear2(err) \
  do { \
svn_error_t *__svn_error = (err); \
if (__svn_error  SVN_ERROR_IN_CATEGORY(__svn_error-apr_err, \
 SVN_ERR_MALFUNC_CATEGORY_START)) \
  return svn_error_trace (__svn_error); \
else \
  svn_error_clear(err); \
  }

(this is orthogonal to your patch)

 +++ subversion/tests/cmdline/svnadmin_tests.py(working copy)
 @@ -1835,6 +1835,107 @@
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()

Wrap to 80 columns

 

Thanks.

Daniel

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

[Julian Foad]

Prabhu Gnana Sundar wrote on 14 December 2012:

 On 12/10/2012 08:15 PM, Julian Foad wrote:
  Prabhu Gnana Sundar
 
  This patch is a follow up of the long discussion we had in thread [1]. 

  Please will you summarize the issues that were raised in the previous 
 discussion and how you have chosen to proceed.  I'm thinking in
 particular of the discussion about how the output is notified -- how
 to display each error, on what output stream, and what to print at the
 end of the whole verification and what exit code to return.  There may
 have been other issues too.
 
 Thanks Julian,
 
 Before this patch:
 ---
 
 When we run svnadmin verify on a repo, the verification process would stop 
 once 
 a failure/corruption is found. The error is notified via the error stream and 
 the exit code would be 1.
 Upon successful verification of each revision of the repo Verified 
 revision r is also notified via the stderr stream and the exit code is 0.
 
 
 Eg:
 
 $ svnadmin verify 
 subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31
 svnadmin: E24: Could not convert '' into a number
 
 
 After this patch:
 -
 
 When we run svnadmin verify on a repo, the verification process would stop 
 once 
 a failure/corruption is found. The error is notified via the error stream and 
 the exit code would be 1. The error stream would end as svnadmin: E165005: 
 Repository 'reponame' failed to verify.
 The behaviour is unchanged for successful verification.
 
 When we run svnadmin verify --keep-going, the verification process continues 
 after detecting any failure/corruption. The failure is reported for whichever 
 revision it is found. The error message is notified via the stderr stream. 
 The 
 successful verification of revisions will also be reported via the stderr 
 stream 
 (no change in behaviour of notifying success). The repo verification failure 
 will be notified at the end as svnadmin: E165005: Repository 
 'reponame' failed to verify. The exit code is 1.
 
 Eg:
 
 $ svnadmin verify 
 subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31
 svnadmin: E24: Could not convert '' into a number
 svnadmin: E165005: Repository 
 'subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31' 
 failed to verify
 
 
 $ svnadmin verify 
 subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31 
 --keep-going
 * Verified revision 0.
 * Verified revision 1.
 * Verified revision 2.
 * Verified revision 3.
 * Verified revision 4.
 * Verified revision 5.
 * Error verifying revision 6.
 svnadmin: E24: Could not convert '' into a number
 svnadmin: E165005: Repository 
 'subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31' 
 failed to verify

Thanks, Prabhu, for explaining this.

That sounds good.  I was surprised to learn that the progress notifications (* 
Verified...) were already going to stderr.

(I was meaning to review the patch before sending this reply, but I have not 
got around to it yet, and now I see you have just sent a new version in 
response to Daniel's comments.)

 Earlier, Mike suggested to notify the errors in both stdout and stderr stream 
 so 
 that they would be independently valuable to the user.
 But I have not notified the errors on both the streams, rather only via the 
 stderr stream, because the error notification would appear twice for each 
 failure when the verification is run from the command line. Thoughts ?

The way I understood C-Mike's suggestion was that all the * Verified revision 
R and * Error verifying revision R messages (and probably the final summary 
line too) would go to stdout, whereas all the svnadmin: Exx: ... messages 
would go to stderr.  Therefore the output would appear just like you describe 
above -- except perhaps within a given revision the error messages might come 
before the Failed to verify revision message instead of after it.

However, that suggestion is something we can consider as a possible future 
change -- it seems to me that your solution is fine.

- Julian

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

[C. Michael Pilato]

On 12/20/2012 08:24 AM, Julian Foad wrote:
 The way I understood C-Mike's suggestion was that all the * Verified
 revision R and * Error verifying revision R messages (and probably the
 final summary line too) would go to stdout, whereas all the svnadmin:
 Exx: ... messages would go to stderr.

Can't remember my precise recommendation here, but I'll happily take credit
for the above as it sounds like something I would probably suggest. :-)

-- 
C. Michael Pilato cmpil...@collab.net
CollabNet  www.collab.net  Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

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

[Daniel Shahaf]

Julian Foad wrote on Thu, Dec 20, 2012 at 13:24:46 +:
 That sounds good.  I was surprised to learn that the progress
 notifications (* Verified...) were already going to stderr.

Probably because * Dumped revision %ld. goes to stderr, and verify
shares code with dump.

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

[Julian Foad]

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?

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?


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'.

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=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.

  * 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?

+ * 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]).

+ * @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
 

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

[Daniel Shahaf]

Julian Foad wrote on Thu, Dec 20, 2012 at 17:55:19 +:
 Please write responses to the points I mentioned in this email.  You
 don't have to agree with all of them, but I want to know whether you
 agree or disagree or don't understand or aren't able to do what
 I suggest, or if I misunderstood your work and made a silly
 suggestion, or whatever.

++1

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

[Prabhu Gnana Sundar]

On 12/10/2012 08:15 PM, Julian Foad wrote:

Prabhu Gnana Sundar


This patch is a follow up of the long discussion we had in thread [1]. This
patch implements a new switch --keep-going to svnadmin verify.

If --keep-going is set(True), svnadmin verify would continue to run
till verifying all the revisions i.e, it would not stop at the very first
occurance of a corruption/error found in the repo and would report corruptions
wherever found.


I have worked on your suggestions and inputs for this patch. Kindly share your
thoughts. Attaching the patch and the log message with this mail.

Thank you for the patch.

Please will you summarize the issues that were raised in the previous 
discussion and how you have chosen to proceed.  I'm thinking in particular of 
the discussion about how the output is notified -- how to display each error, 
on what output stream, and what to print at the end of the whole verification 
and what exit code to return.  There may have been other issues too.


Thanks Julian,

Before this patch:
---

When we run svnadmin verify on a repo, the verification process would 
stop once a failure/corruption is found. The error is notified via the 
error stream and the exit code would be 1.
Upon successful verification of each revision of the repo Verified 
revision r is also notified via the stderr stream and the exit code is 0.



Eg:

$ svnadmin verify 
subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31

svnadmin: E24: Could not convert '' into a number


After this patch:
-

When we run svnadmin verify on a repo, the verification process would 
stop once a failure/corruption is found. The error is notified via the 
error stream and the exit code would be 1. The error stream would end as 
svnadmin: E165005: Repository 'reponame' failed to verify.

The behaviour is unchanged for successful verification.

When we run svnadmin verify --keep-going, the verification process 
continues after detecting any failure/corruption. The failure is 
reported for whichever revision it is found. The error message is 
notified via the stderr stream. The successful verification of revisions 
will also be reported via the stderr stream (no change in behaviour of 
notifying success). The repo verification failure will be notified at 
the end as svnadmin: E165005: Repository 'reponame' failed to verify. 
The exit code is 1.


Eg:

$ svnadmin verify 
subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31

svnadmin: E24: Could not convert '' into a number
svnadmin: E165005: Repository 
'subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31' 
failed to verify



$ svnadmin verify 
subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31 
--keep-going

* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
* Verified revision 3.
* Verified revision 4.
* Verified revision 5.
* Error verifying revision 6.
svnadmin: E24: Could not convert '' into a number
svnadmin: E165005: Repository 
'subversion/tests/cmdline/svn-test-work/repositories/svnadmin_tests-31' 
failed to verify



Earlier, Mike suggested to notify the errors in both stdout and stderr 
stream so that they would be independently valuable to the user.
But I have not notified the errors on both the streams, rather only via 
the stderr stream, because the error notification would appear twice for 
each failure when the verification is run from the command line. Thoughts ?



Thanks and regards
Prabhu

[PATCH] Implement svnadmin verify --keep-going

[Prabhu Gnana Sundar]

Hi,

This patch is a follow up of the long discussion we had in thread [1]. 
This patch implements a new switch --keep-going to svnadmin verify.


If --keep-going is set(True), svnadmin verify would continue to run 
till verifying all the revisions i.e, it would not stop at the very 
first occurance of a corruption/error found in the repo and would report 
corruptions wherever found.



I have worked on your suggestions and inputs for this patch. Kindly 
share your thoughts. Attaching the patch and the log message with this mail.



Thanks and regards
Prabhu

[1] http://svn.haxx.se/dev/archive-2012-10/0271.shtml

Implement svnadmin verify --keep-going, which would continue the verification
even if there is some corruption, after printing the errors to stderr.

* subversion/svnadmin/svnadmin.c
  (svnadmin__cmdline_options_t): Add keep-going option.
  (svnadmin_opt_state): Add keep-going option.
  (subcommand_verify): Switch to the new svn_repos_verify_fs3 function instead
   of svn_repos_verify_fs2, and pass the keep-going option.
  (repos_notify_handler): Handle svn_repos_notify_failure notification by
   printing warnings to stderr with the respective revision number.

* subversion/include/svn_repos.h
  (svn_repos_notify_action_t): Add svn_repos_notify_failure to notify failure.
  (svn_repos_verify_fs3): Newly added to handle keep-going option.
  (svn_repos_notify_t): Add err, the error chain which indicates what went
   wrong during verification.

* subversion/libsvn_repos/dump.c
  (svn_repos_verify_fs3): Handle keep-going. If keep-going is TRUE, the
   error message is notified and verification process continues.
  When a repository fails to verify, return an error SVN_ERR_REPOS_CORRUPTED.
  (notify_verification_error): New function to notify the verification
  failure error message.

* subversion/libsvn_repos/deprecated.c
  (svn_repos_verify_fs2): Deprecate. Call svn_repos_verify_fs3 with
   keep_going as FALSE by default to keep the old default implementation.

* subversion/libsvn_repos/notify.c
  (svn_repos_notify_create): Initialise the error chain err to SVN_NO_ERROR.

* subversion/tests/cmdline/svnadmin_tests.py
  (verify_keep_going): New test case to test svnadmin verify and the new
  switch --keep-going.

Patch by: Prabhu Gnana Sundar prabhugs{_AT_}collab.net
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
+type: file
+count: 0
+text: 2 0 60 48 02d086e41b03058c5f1af6282c1f483f cc67e4dd7cd8ca83095c8b95f65b6698b39cb263 5-5/_5
+props: 2 73 34 0 25e6c2f7558b7484000d4d090dea5b92
+cpath: /Projects/docs/README
+copyroot: 0 /
+
+PLAIN
+K 6
+README
+V 15
+file 3-6.0.r6/0
+END
+ENDREP
+id: 1-6.0.r6/275
+type: dir
+count: 0
+text: 6 226 36 0 2c3f6410944c6ff8667fa1b3e78f45a2
+cpath: /Projects/docs
+copyroot: 0 /
+
+PLAIN
+K 9
+Project-X
+V 14
+dir 1-3.0.r3/0
+K 9
+Project-Y
+V 14
+dir 1-4.0.r4/0
+K 9
+Project-Z
+V 14
+dir 1-5.0.r5/0
+K 4
+docs
+V 16
+dir 1-6.0.r6/275
+END
+ENDREP
+id: 0-1.0.r6/548
+type: dir
+pred: 0-1.0.r5/195
+count: 4
+text: 6 398 137 0 c758f548da93cc57d68af0610766b549
+cpath: /Projects
+copyroot: 0 /
+
+PLAIN
+K 8
+Projects
+V 16
+dir 0-1.0.r6/548
+K 6
+README
+V 17
+file 0-2.0.r2/120
+END
+ENDREP
+id: 0.0.r6/772
+type: dir
+pred: 0.0.r5/418
+count: 6
+text: 6 686 73 0 353c6bbf43b0f2ae474d85e206337bbd
+cpath: /
+copyroot: 0 /
+
+_1.0.t5-5 add-dir false false /Projects/docs
+
+_3.0.t5-5 add-file true true /Projects/docs/README
+
+
+)
+  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 

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

[Julian Foad]

Prabhu Gnana Sundar

 This patch is a follow up of the long discussion we had in thread [1]. This 
 patch implements a new switch --keep-going to svnadmin verify.
 
 If --keep-going is set(True), svnadmin verify would continue to run 
 till verifying all the revisions i.e, it would not stop at the very first 
 occurance of a corruption/error found in the repo and would report 
 corruptions 
 wherever found.
 
 
 I have worked on your suggestions and inputs for this patch. Kindly share 
 your 
 thoughts. Attaching the patch and the log message with this mail.

Thank you for the patch.

Please will you summarize the issues that were raised in the previous 
discussion and how you have chosen to proceed.  I'm thinking in particular of 
the discussion about how the output is notified -- how to display each error, 
on what output stream, and what to print at the end of the whole verification 
and what exit code to return.  There may have been other issues too.

I scanned quickly through your patch and I noticed one place where you declare 
a local function without the 'static' keyword.  I expect this should give a 
warning when you compile it, so please will you compile with and without your 
patch applied and check for any additional compiler warnings that your patch 
adds.

- Julian

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

[Daniel Shahaf]

 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: E24: 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, 

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

[Daniel Shahaf]

Julian Foad wrote on Mon, Dec 10, 2012 at 14:45:57 +:
 I scanned quickly through your patch and I noticed one place where you 
 declare a local function without the 'static' keyword.  I expect this should 
 give a warning when you compile it, so please will you compile with and 
 without your patch applied and check for any additional compiler warnings 
 that your patch adds.

Or you can cheat by using compiler flags that build trunk by default
with no warnings:

'./configure'  '--enable-maintainer-mode' '-C' 'CFLAGS= -DSVN_DEPRECATED= 
-Wformat=0 -Wno-unreachable-code -g' 'CC=x86_64-linux-gnu-gcc' $@