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

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

[Lukas Fleischer]

While explicitly stating that the commit message in a prerequisite line
is optional, we required all lines with 40 or more characters to contain
a space after the object name, bailing out if a line consisted of an
object name only. Fix this off-by-one error and only require lines with
more than 40 characters to contain the separating space.

Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
---
 bundle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bundle.c b/bundle.c
index 505e07e..4b0e5cd 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])) ||
+		    (buf.len > 40 && !isspace(buf.buf[40])) ||
 		    (!is_prereq && buf.len <= 40)) {
 			if (report_path)
 				error(_("unrecognized header: %s%s (%d)"),
-- 
1.8.2.675.gda3bb24.dirty

[PATCH 2/2] t5704: Test prerequisites with empty commit messages

[Lukas Fleischer]

While stating that commit messages of prerequisites are optional in
bundles, we used to disallow prerequisite lines consisting of an object
name only. This behavior made `git bundle verify` and `git bundle
unbundle` reject bundles that contain prerequisites without any commit
message.

Add a test to reproduce this behavior.

Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
---
 t/t5704-bundle.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 9e43731..a45c316 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -58,4 +58,14 @@ test_expect_success 'ridiculously long subject in boundary' '
 	grep "^-[0-9a-f]\\{40\\} " boundary
 '
 
+test_expect_success 'prerequisites with an empty commit message' '
+	: >file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit --allow-empty-message -m "" &&
+	test_commit file2 &&
+	git bundle create bundle HEAD^.. &&
+	git bundle verify bundle
+'
+
 test_done
-- 
1.8.2.675.gda3bb24.dirty

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

[Lukas Fleischer]

On Sun, Apr 07, 2013 at 01:53:15PM +0200, Lukas Fleischer wrote:
> While explicitly stating that the commit message in a prerequisite line
> is optional, we required all lines with 40 or more characters to contain
> a space after the object name, bailing out if a line consisted of an
> object name only. Fix this off-by-one error and only require lines with
> more than 40 characters to contain the separating space.
> 
> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> ---
>  bundle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 505e07e..4b0e5cd 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])) ||
> +		    (buf.len > 40 && !isspace(buf.buf[40])) ||

By the way, I changed this to "buf.len > 40" instead of "40 < buf.len"
because I personally think that the former is much easier to read here.
Is there any general guideline when to use which order? grep(1) says we
use both forms:

    $ grep '0 <' *.c | wc -l
    119
    $ grep '> 0' *.c | wc -l
    164

>  		    (!is_prereq && buf.len <= 40)) {
>  			if (report_path)
>  				error(_("unrecognized header: %s%s (%d)"),
> -- 
> 1.8.2.675.gda3bb24.dirty
> 
> --
> 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

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

[Junio C Hamano]

Lukas Fleischer <git@cryptocrack.de> writes:

> While explicitly stating that the commit message in a prerequisite
> line is optional, 

Good spotting.  What e9ee84cf28b6 (bundle: allowing to read from an
unseekable fd, 2011-10-13) meant to say was the SP was optional but
we want to allow a tip bundled to have a commit without any commit
log message (hence making it "optional"), and the check you are
looking at does try to enforce.  What was buggy was that the
comparison did not take into account that the codepath earlier
called rtrim() on it, stripping "-<object name>SP<eol>" of the
trailing SP it wants to look for.

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

We never had any official guideline on which to use.  A patch that
changes an existing "b > a" to "a < b" (this directoin only) because
of the above thread is _not_ "a patch to fix coding style", and is
very much unwelcome.

Similarly, a patch that changes an existing "a < b" to "b > a" (or
vice versa) only because the author thinks it is easier to read is
not welcomed.

A switch done as a part of other meaningful rewrite is not a big
enough deal to make a fuss over, though.  Your patch falls into this
category, I think.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lukas Fleischer <git@cryptocrack.de> writes:
>
>> While explicitly stating that the commit message in a prerequisite
>> line is optional, 
>
> Good spotting.  What e9ee84cf28b6 (bundle: allowing to read from an
> unseekable fd, 2011-10-13) meant to say was the SP was optional but

s/say was the SP was optional/say was not the SP was optional/

> we want to allow a tip bundled to have a commit without any commit
> log message (hence making it "optional"), and the check you are
> looking at does try to enforce.  What was buggy was that the
> comparison did not take into account that the codepath earlier
> called rtrim() on it, stripping "-<object name>SP<eol>" of the
> trailing SP it wants to look for.

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

[Jeff King]

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

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

[Lukas Fleischer]

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