Stefan Sperling wrote on Thu, Nov 01, 2012 at 11:59:43 +0100:
> 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<prabhugs_at_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?
svn_error_quick_wrap maybe?
Received on 2012-11-01 16:24:35 CET