You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Prabhu Gnana Sundar <pr...@collab.net> on 2012/12/10 12:42:49 UTC
[PATCH] Implement svnadmin verify --keep-going
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
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Thu, Dec 20, 2012 at 13:24:46 +0000:
> 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
Posted by "C. Michael Pilato" <cm...@collab.net>.
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:
> Exxxxxx: ..." 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 <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Julian Foad <ju...@btopenworld.com>.
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: E200004: 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: E200004: 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: E200004: 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: Exxxxxx: ..." 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
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
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: E200004: 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: E200004: 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: E200004: 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
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Mon, Dec 10, 2012 at 14:45:57 +0000:
> 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' "$@"
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Julian Foad <ju...@btopenworld.com>.
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
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Stefan Sperling <st...@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 +0000, Julian Foad wrote:
>> Thanks for this version, Prabhu. It looks much better. Still a few
>more points...
>>
>>
>> Prabhu Gnana Sundar <pr...@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 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
>>
>> Great.
>>
>> So, have you tested any or all of these scenarios by hand?
>>
>> Do you plan to update the test you wrote to cover any of these
>scenarios? I'm not saying you have to automate these, but it would be
>nice, and the one test that you have included still doesn't actually
>test that the verify keeps going, so that one really does need to be
>improved.
>>
>>
>> >> 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 notify_func is not @c NULL, write feedback to it (lines
>of the
>> > + * form "* Verified revision %ld\n". If an error/corruption is
>found,
>> > + * the feedback is of the form "* Error verifying revision
>%ld\n").
>>
>> That's not accurate, as we don't write lines of text to the callback
>function, we pass it an
>> object that contains an 'action' enumerator and a revision number and
>other values.
>>
>> >> * 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...
>>
>> You haven't changed that paragraph. What are these warnings that it
>refers to? Thatdoesn't seem right. You have added another paragraph:
>>
>> > + * For every revision verification failure call
>notify_verification_error
>> > + * with @a rev set to the corrupt revision.
>>
>> A doc string should be written in terms of the interface to the
>function. The interface to this function doesn't know about the
>existence of the function notify_verification_error().
>>
>> >> + * 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
>>
>> Thanks. You added:
>>
>> > + * [...] Upon successful completion of verification, return
>> > + * SVN_NO_ERROR else return the corresponding failure.
>>
>> Let's change this to: "Finally, return an error if there were any
>failures during verification, or SVN_NO_ERROR if there were no
>failures."
>>
>>
>> >> + * @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.
>>
>> Great. It needs the 'static' keyword and it needs a doc string. In
>a case like this, the doc string could just say something very simple
>like "Verify revision REV in file system FS." as the rest of the
>parameters are pretty standard. "verify_one_revision" might be a
>better name for the function. The pool parameter should be renamed to
>'scratch_pool'.
>>
>> Thanks.
>>
>> - Julian
>>
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Julian Foad <ju...@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
Posted by Julian Foad <ju...@btopenworld.com>.
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
Posted by Stefan Sperling <st...@elego.de>.
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 +0000, Julian Foad wrote:
> Thanks for this version, Prabhu. It looks much better. Still a few more points...
>
>
> Prabhu Gnana Sundar <pr...@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 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
>
> Great.
>
> So, have you tested any or all of these scenarios by hand?
>
> Do you plan to update the test you wrote to cover any of these scenarios? I'm not saying you have to automate these, but it would be nice, and the one test that you have included still doesn't actually test that the verify keeps going, so that one really does need to be improved.
>
>
> >> 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 notify_func is not @c NULL, write feedback to it (lines of the
> > + * form "* Verified revision %ld\n". If an error/corruption is found,
> > + * the feedback is of the form "* Error verifying revision %ld\n").
>
> That's not accurate, as we don't write lines of text to the callback function, we pass it an
> object that contains an 'action' enumerator and a revision number and other values.
>
> >> * 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...
>
> You haven't changed that paragraph. What are these warnings that it refers to? Thatdoesn't seem right. You have added another paragraph:
>
> > + * For every revision verification failure call notify_verification_error
> > + * with @a rev set to the corrupt revision.
>
> A doc string should be written in terms of the interface to the function. The interface to this function doesn't know about the existence of the function notify_verification_error().
>
> >> + * 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
>
> Thanks. You added:
>
> > + * [...] Upon successful completion of verification, return
> > + * SVN_NO_ERROR else return the corresponding failure.
>
> Let's change this to: "Finally, return an error if there were any failures during verification, or SVN_NO_ERROR if there were no failures."
>
>
> >> + * @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.
>
> Great. It needs the 'static' keyword and it needs a doc string. In a case like this, the doc string could just say something very simple like "Verify revision REV in file system FS." as the rest of the parameters are pretty standard. "verify_one_revision" might be a better name for the function. The pool parameter should be renamed to 'scratch_pool'.
>
> Thanks.
>
> - Julian
>
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Julian Foad <ju...@btopenworld.com>.
Thanks for this version, Prabhu. It looks much better. Still a few more points...
Prabhu Gnana Sundar <pr...@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 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
Great.
So, have you tested any or all of these scenarios by hand?
Do you plan to update the test you wrote to cover any of these scenarios? I'm not saying you have to automate these, but it would be nice, and the one test that you have included still doesn't actually test that the verify keeps going, so that one really does need to be improved.
>> 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 notify_func is not @c NULL, write feedback to it (lines of the
> + * form "* Verified revision %ld\n". If an error/corruption is found,
> + * the feedback is of the form "* Error verifying revision %ld\n").
That's not accurate, as we don't write lines of text to the callback function, we pass it an
object that contains an 'action' enumerator and a revision number and other values.
>> * 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...
You haven't changed that paragraph. What are these warnings that it refers to? Thatdoesn't seem right. You have added another paragraph:
> + * For every revision verification failure call notify_verification_error
> + * with @a rev set to the corrupt revision.
A doc string should be written in terms of the interface to the function. The interface to this function doesn't know about the existence of the function notify_verification_error().
>> + * 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
Thanks. You added:
> + * [...] Upon successful completion of verification, return
> + * SVN_NO_ERROR else return the corresponding failure.
Let's change this to: "Finally, return an error if there were any failures during verification, or SVN_NO_ERROR if there were no failures."
>> + * @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.
Great. It needs the 'static' keyword and it needs a doc string. In a case like this, the doc string could just say something very simple like "Verify revision REV in file system FS." as the rest of the parameters are pretty standard. "verify_one_revision" might be a better name for the function. The pool parameter should be renamed to 'scratch_pool'.
Thanks.
- Julian
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
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 :) )
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Thu, Dec 20, 2012 at 17:55:19 +0000:
> 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
Posted by Julian Foad <ju...@btopenworld.com>.
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
verification error that isn't checked -- the call to
svn_fs_revision_root()."
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.
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.
Thank you.
- Julian
[1] <http://svn.haxx.se/dev/archive-2012-11/0118.shtml>
[2] <http://svn.haxx.se/dev/archive-2012-10/0498.shtml>
[3] <http://svn.haxx.se/dev/archive-2012-11/0034.shtml>
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Daniel Shahaf <da...@elego.de>.
> 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
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
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.
Re: [PATCH] Implement svnadmin verify --keep-going
Posted by Daniel Shahaf <da...@elego.de>.
> 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.