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.
--Prabhu
Received on 2012-11-01 10:00:11 CET