threads / patch / 33417

patch, 2 partst5704: Test prerequisites with empty commit messages

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

## tl;dr

7 messages between Apr 7, 2013 and Apr 8, 2013. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Lukas Fleischer· Apr 7, 2013, 11:53 UTC · lore

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

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(-)
Show changes to bundle.c +1 −1
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
Lukas Fleischer· Apr 7, 2013, 11:53 UTC · re: Lukas Fleischer · lore

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(+)
Show changes to t/t5704-bundle.sh +10 −0
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
Lukas Fleischer· Apr 7, 2013, 15:24 UTC · re: Lukas Fleischer · lore

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

On Sun, Apr 07, 2013 at 01:53:15PM +0200, Lukas Fleischer wrote:
Show 21 quoted lines
> 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
Show 10 quoted lines
>  		    (!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
Junio C Hamano· Apr 7, 2013, 17:21 UTC · re: Lukas Fleischer · lore

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

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.

Junio C Hamano· Apr 7, 2013, 17:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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/
Show 6 quoted lines
> 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.
Jeff King· Apr 8, 2013, 01:06 UTC · re: Junio C Hamano · lore

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

On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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:

Show changes to bundle.c +1 −2
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
Lukas Fleischer· Apr 8, 2013, 09:53 UTC · re: Jeff King · lore

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

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

Show 9 quoted lines
> 
> 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

← back to recent threads