bundle: Accept prerequisites without commit messages
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.
--- bundle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
bundle.c +1 -1
@@ -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)"),
Signature · 1 line
1.8.2.675.gda3bb24.dirty
PATCH 2/2 t5704: Test prerequisites with empty commit messages
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.
--- t/t5704-bundle.sh | 10 ++++++++++ 1 file changed, 10 insertions(+)
t/t5704-bundle.sh +10 -0
@@ -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
Signature · 1 line
1.8.2.675.gda3bb24.dirty
Quoted 16 lines · 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 <[email protected]>--- bundle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)bundle.c +0
-0
bundle.c static int parse_bundle_header(int fd, struct bundle_header *header, · line 60
Quoted 5 lines
bundle.c +1
-1@@ -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
164Quoted 10 lines
bundle.c +0
-0(!is_prereq && buf.len <= 40)) {if (report_path) error(_("unrecognized header: %s%s (%d)"),Signature · 6 lines
1.8.2.675.gda3bb24.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · Lukas Fleischer
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.
Quoted 7 lines · Junio C Hamano
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:
bundle.c +1 -1
@@ -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
Quoted 29 lines · Jeff King
Quoted 7 lines · Junio C Hamano
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:
bundle.c +1
-1@@ -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)"),
Quoted 4 lines
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 :)
Quoted 9 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Junio C Hamano
Quoted 2 lines · Lukas Fleischer
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/
Quoted 6 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.