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

On Sun, Feb 17, 2013 at 04:54:43PM -0800, Jonathan Nieder wrote:

quoted

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.

True. That's why I always write "< 0". :)

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.

Interesting. To me, "foo() < 0" just reads idiomatically as "error-check
the foo call".

Anyway, I've redone the patch series to just re-use get_remote_heads,
which is more robust. So this function has gone away in the new version.

-Peff