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
