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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 18, 2013, 00:54 UTC
Message-ID: <20130218005443.GB3221@elie.Belkin>
In-Reply-To: <20130217191422.GA25096@sigill.intra.peff.net>

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