Re: [PATCH] Implement svnadmin verify --force

Prabhu Gnana Sundar <prabhugs_at_collab.net> wrote:
> Julian Foad wrote:

>> [...]  We should choose either "notification" style
>> (that is, messages that are not error messages), such as
[...]
>> or "error messages" style, in which case the messages should be
>> formatted like all svn err msgs, for example:
[...]
>> But the output-style design here is linked to the question of what
>> we're going to print at the end of the verification.  Prabhu, what's
>> the plan for when the command exits?  Print a summary or an error
>> message?  Exit with non-zero return code (I hope)?
>
> I made the output to be exactly like the one you have suggested above.

Oh... Why did you choose that and not C-Mike Pilato's follow-up suggestion?

> But, I am also adding one more error message as follows
>
> svnadmin: E165005: Repository has corruptions.
>
> and then exit with exit code 1.

Great, that sounds good, apart from what somebody suggested about changing the wording to something like 'failed to verify'.

> The default behaviour is not at all affected.
> This new error message is effected only when --keep-going is true. Is that fine
> ? Now all the test cases pass.

I think when --keep-going is false we should still emit this new error -- in addition to whatever it already emitted.  That would change the existing behaviour a bit, but I think in a good way, because it would be more consistent.

> Attaching the patch for you to have a look at it.
>
> Not attaching the log message since I am yet to work on sending the
> "Verified revision x" to the stdout.

The log message helps people to review the patch, by explaining the reason for the changes.  Sorry, but I am not willing to review your patch without a log message.

Also, it would
really help me (and other reviewers) if you would write responses to the review
 comments.  There have been lots of comments, probably many more than you were expecting.  I can't tell whether you have read them all and addressed them all in your latest patch, or if you have not yet had enough time to work through them.

> I am facing a few issues in it.
> Like, a few tests fail because the expected output don't match with the
> actual output. So need suggestions on that too. Is it fine to tweak the already
> available test case's expected output based on the output of what we send to
> stdout and stderr ?

 Yes, it is OK to tweak the expectations of existing tests when we change some details of the existing behaviour.

- Julian
Received on 2012-11-05 15:27:42 CET