From: Jeff King <peff@peff.net>
Date: Mon, 18 Feb 2013 08:59:36 GMT
Subject: Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Message-ID: <20130218085936.GA17003@sigill.intra.peff.net>
In-Reply-To: <20130218005443.GB3221@elie.Belkin>

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

> > 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