From: Jeff King <peff@peff.net>
Date: Mon, 08 Apr 2013 01:06:10 GMT
Subject: Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages
Message-ID: <20130408010610.GB24030@sigill.intra.peff.net>
In-Reply-To: <7va9pa5k9u.fsf@alter.siamese.dyndns.org>

On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:

> As to the order of comparison to match the order on the number line,
> e.g. write "0 < something" or "negative < 0" to let readers more
> easily visualize in what relation on the number line the quantity of
> each side of the comparison stands, here is a reference to a long
> and amusing thread:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912

I do not necessarily agree with the "always use less-than" style, but as
a reviewer of this series, it took me an extra minute to figure out what
was going on because two things changed. If the diff instead looked
like:

diff --git a/bundle.c b/bundle.c
index 505e07e..a9c1335 100644
--- a/bundle.c
+++ b/bundle.c
@@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 		 * followed by SP and subject line.
 		 */
 		if (get_sha1_hex(buf.buf, sha1) ||
-		    (40 <= buf.len && !isspace(buf.buf[40])) ||
+		    (40 < buf.len && !isspace(buf.buf[40])) ||
 		    (!is_prereq && buf.len <= 40)) {
 			if (report_path)
 				error(_("unrecognized header: %s%s (%d)"),

then it is immediately obvious that we are only impacting the case where
buf.len is exactly 40 (and it is even more obvious if you happen to use
the diff-highlight script, which highlights the single changed
character).

Just my two cents as a reader of the patch. Other than that, it looks
correct to me. :)

-Peff