Re: [PATCH] Implement svnadmin verify --force

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
Received on 2012-10-31 19:02:29 CET