Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:
> On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:
>> Jeff King wrote:

>>> --- a/remote-curl.c
[...]
>>> +		if (read_packets_until_flush(&last->buf, &last->len) < 0)
>> 
>> Style nit: this made me wonder "What would it mean if
>> read_packets_until_flush() > 0?"
[...]
> My intent was that it followed the error convention of "negative is
> error, 0 is success, and positive is not used, but reserved for
> future use".

>From a maintainability perspective, that kind of contract would be
dangerous, since some *other* caller could arrive and use the function
without a "< 0" without knowing it is doing anything wrong.  When new
return values appear, the function should be renamed to help the patch
author and reviewers remember to check all callers.

That is, from the point of view of maintainability, there is no
distinction between "if (read_packets_until_... < 0)" and
"if (read_packets_until_...)" and either form is fine.

My comment was just to say the "< 0" forced me to pause a moment and
check out the implementation.  This is basically a stylistic thing and
if you prefer to keep the "< 0", that's fine with me.

>                                                                      If
> an implementation is producing bogus packet lines and expecting us not
> to complain, it really needs to be fixed.

Agreed completely.  Thanks again for the patch.

Jonathan