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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html