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: > > > >>Julian Foad wrote on Wed, Oct 31, 2012 at 17:36:09 +0000: > >>> Prabhu Gnana Sundar<prabh...@collab.net> wrote: > >>> > + revstr = ""; > >>> > + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */, > >>> > + apr_psprintf(scratch_pool, "svnadmin: %s", > >>> revstr)); > >>> > + return; > >>> > >>> In what cases will the revision number be invalid? This prints a > >>>half-empty message in those cases; what did you intend? > >>The code will print > >> svnadmin: E160004: Corrupt filesystem > >>or > >> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem > >>respectively as the revision number is SVN_INVALID_REVNUM or 42. So > >>there is no "half-empty" message here. > >Oh, I see: that psprintf is the 'prefix' argument of handle_error2. I > >misunderstood. > > > >>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. > > > >- Julian > > 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.
The lines should start with "svnamdin: ". 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?
