Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages

[Lukas Fleischer <git@cryptocrack.de>]

On Sun, Apr 07, 2013 at 09:06:10PM -0400, Jeff King wrote:
> 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).

I changed it for the very same reason -- it took me an extra minute to
figure out what is going on when trying to pinpoint the bug (it was
especially weird since we use "40 <= buf.len" here and "buf.len <= 40"
one line below -- which kind of makes sense now, though). Thanks for the
review (and merging), I won't change operand order in future patches :)

> 
> Just my two cents as a reader of the patch. Other than that, it looks
> correct to me. :)
> 
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html