Stefan Sperling wrote:
> On Thu, Nov 01, 2012 at 02:21:48PM +0530, Prabhu Gnana Sundar wrote:
>> On 10/31/2012 11:31 PM, Julian Foad wrote:
>>> Daniel Shahaf wrote:
>>>> The code will print
>>>> svnadmin: E160004: Corrupt filesystem
>>>> or
>>>> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem
[...]
>>>> But the code moves the E160004 away from its current location
>>>> immediately after the "svnadmin:" prefix. I am not sure I like that;
>>>> is there a good reason not to have the message be of the form
>>>> svnadmin: E160004: %s
>>>> in the interest of parseability?
>>>
>>> I agree that would be better: the prefix should remain just
>>> "svnadmin: " and the error message should be adjusted instead.
>>
>> Does this look fine ? I personally feel this is easily readable.
>>
>> * Verified revision 0.
>> * Verified revision 1.
>> * Verified revision 2.
>> Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev
> at r3 (offset 787)
>> Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt
> data: Decompression of svndiff data failed
>> * Verified revision 5.
That's easily readable, but I don't like it: it's a funny mixture of styles. We should choose either "notification" style (that is, messages that are not error messages), such as
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
* Error verifying revision 3: Missing node-id in node-rev at r3 (offset 787)
* Error verifying revision 4: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
or "error messages" style, in which case the messages should be formatted like all svn err msgs, for example:
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
svnadmin: E199999: Error verifying revision 3
svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
svnadmin: E199999: Error verifying revision 4
svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
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)?
> The lines should start with "svnamdin: ".
The more I look at it, the more I think it makes sense to use notification-style output, and then exit with an error (a standard svn-style error message and non-zero exit code) at the end.
> I believe what Julian was suggesting is to stop doing the apr_psprintf()
> dance I suggested, and make libsvn_repos return a better error message
> instead which includes the revision number. Is that right, Julian?
(Well, that's one way of implementing the behaviour that I was suggesting.)
- Julian
Received on 2012-11-01 15:17:39 CET