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

Jeff King wrote:

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

quoted

Jeff King wrote:

quoted

quoted

--- a/remote-curl.c

[...]

quoted

quoted

+		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