{"thread":{"id":"33417","subject":"[PATCH 2/2] t5704: Test prerequisites with empty commit messages","startedAt":"2013-04-07T11:53:15Z","lastAt":"2013-04-08T09:53:03Z","messageCount":7,"participants":["Lukas Fleischer","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"213405","messageId":"1365335596-19917-1-git-send-email-git@cryptocrack.de","threadId":"33417","inReplyTo":null,"subject":"[PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2013-04-07T11:53:15Z","receivedAt":"2013-04-07T11:53:15Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"While explicitly stating that the commit message in a prerequisite line\nis optional, we required all lines with 40 or more characters to contain\na space after the object name, bailing out if a line consisted of an\nobject name only. Fix this off-by-one error and only require lines with\nmore than 40 characters to contain the separating space.\n\nSigned-off-by: Lukas Fleischer <git@cryptocrack.de>\n---\n bundle.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 505e07e..4b0e5cd 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t * followed by SP and subject line.\n \t\t */\n \t\tif (get_sha1_hex(buf.buf, sha1) ||\n-\t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n+\t\t    (buf.len > 40 && !isspace(buf.buf[40])) ||\n \t\t    (!is_prereq && buf.len <= 40)) {\n \t\t\tif (report_path)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n-- \n1.8.2.675.gda3bb24.dirty\n"},{"id":"213404","messageId":"1365335596-19917-2-git-send-email-git@cryptocrack.de","threadId":"33417","inReplyTo":"1365335596-19917-1-git-send-email-git@cryptocrack.de","subject":"[PATCH 2/2] t5704: Test prerequisites with empty commit messages","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2013-04-07T11:53:16Z","receivedAt":"2013-04-07T11:53:16Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"While stating that commit messages of prerequisites are optional in\nbundles, we used to disallow prerequisite lines consisting of an object\nname only. This behavior made `git bundle verify` and `git bundle\nunbundle` reject bundles that contain prerequisites without any commit\nmessage.\n\nAdd a test to reproduce this behavior.\n\nSigned-off-by: Lukas Fleischer <git@cryptocrack.de>\n---\n t/t5704-bundle.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 9e43731..a45c316 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -58,4 +58,14 @@ test_expect_success 'ridiculously long subject in boundary' '\n \tgrep \"^-[0-9a-f]\\\\{40\\\\} \" boundary\n '\n \n+test_expect_success 'prerequisites with an empty commit message' '\n+\t: >file1 &&\n+\tgit add file1 &&\n+\ttest_tick &&\n+\tgit commit --allow-empty-message -m \"\" &&\n+\ttest_commit file2 &&\n+\tgit bundle create bundle HEAD^.. &&\n+\tgit bundle verify bundle\n+'\n+\n test_done\n-- \n1.8.2.675.gda3bb24.dirty\n"},{"id":"213412","messageId":"20130407152437.GA334@blizzard","threadId":"33417","inReplyTo":"1365335596-19917-1-git-send-email-git@cryptocrack.de","subject":"Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2013-04-07T15:24:37Z","receivedAt":"2013-04-07T15:24:37Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"On Sun, Apr 07, 2013 at 01:53:15PM +0200, Lukas Fleischer wrote:\n> While explicitly stating that the commit message in a prerequisite line\n> is optional, we required all lines with 40 or more characters to contain\n> a space after the object name, bailing out if a line consisted of an\n> object name only. Fix this off-by-one error and only require lines with\n> more than 40 characters to contain the separating space.\n> \n> Signed-off-by: Lukas Fleischer <git@cryptocrack.de>\n> ---\n>  bundle.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/bundle.c b/bundle.c\n> index 505e07e..4b0e5cd 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n>  \t\t * followed by SP and subject line.\n>  \t\t */\n>  \t\tif (get_sha1_hex(buf.buf, sha1) ||\n> -\t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n> +\t\t    (buf.len > 40 && !isspace(buf.buf[40])) ||\n\nBy the way, I changed this to \"buf.len > 40\" instead of \"40 < buf.len\"\nbecause I personally think that the former is much easier to read here.\nIs there any general guideline when to use which order? grep(1) says we\nuse both forms:\n\n    $ grep '0 <' *.c | wc -l\n    119\n    $ grep '> 0' *.c | wc -l\n    164\n\n>  \t\t    (!is_prereq && buf.len <= 40)) {\n>  \t\t\tif (report_path)\n>  \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n> -- \n> 1.8.2.675.gda3bb24.dirty\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"213424","messageId":"7va9pa5k9u.fsf@alter.siamese.dyndns.org","threadId":"33417","inReplyTo":"1365335596-19917-1-git-send-email-git@cryptocrack.de","subject":"Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T17:21:33Z","receivedAt":"2013-04-07T17:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lukas Fleischer <git@cryptocrack.de> writes:\n\n> While explicitly stating that the commit message in a prerequisite\n> line is optional, \n\nGood spotting.  What e9ee84cf28b6 (bundle: allowing to read from an\nunseekable fd, 2011-10-13) meant to say was the SP was optional but\nwe want to allow a tip bundled to have a commit without any commit\nlog message (hence making it \"optional\"), and the check you are\nlooking at does try to enforce.  What was buggy was that the\ncomparison did not take into account that the codepath earlier\ncalled rtrim() on it, stripping \"-<object name>SP<eol>\" of the\ntrailing SP it wants to look for.\n\nAs to the order of comparison to match the order on the number line,\ne.g. write \"0 < something\" or \"negative < 0\" to let readers more\neasily visualize in what relation on the number line the quantity of\neach side of the comparison stands, here is a reference to a long\nand amusing thread:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912\n\nWe never had any official guideline on which to use.  A patch that\nchanges an existing \"b > a\" to \"a < b\" (this directoin only) because\nof the above thread is _not_ \"a patch to fix coding style\", and is\nvery much unwelcome.\n\nSimilarly, a patch that changes an existing \"a < b\" to \"b > a\" (or\nvice versa) only because the author thinks it is easier to read is\nnot welcomed.\n\nA switch done as a part of other meaningful rewrite is not a big\nenough deal to make a fuss over, though.  Your patch falls into this\ncategory, I think.\n"},{"id":"213431","messageId":"7v61zy45cw.fsf@alter.siamese.dyndns.org","threadId":"33417","inReplyTo":"7va9pa5k9u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T17:29:03Z","receivedAt":"2013-04-07T17:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Lukas Fleischer <git@cryptocrack.de> writes:\n>\n>> While explicitly stating that the commit message in a prerequisite\n>> line is optional, \n>\n> Good spotting.  What e9ee84cf28b6 (bundle: allowing to read from an\n> unseekable fd, 2011-10-13) meant to say was the SP was optional but\n\ns/say was the SP was optional/say was not the SP was optional/\n\n> we want to allow a tip bundled to have a commit without any commit\n> log message (hence making it \"optional\"), and the check you are\n> looking at does try to enforce.  What was buggy was that the\n> comparison did not take into account that the codepath earlier\n> called rtrim() on it, stripping \"-<object name>SP<eol>\" of the\n> trailing SP it wants to look for.\n"},{"id":"213491","messageId":"20130408010610.GB24030@sigill.intra.peff.net","threadId":"33417","inReplyTo":"7va9pa5k9u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T01:06:10Z","receivedAt":"2013-04-08T01:06:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:\n\n> As to the order of comparison to match the order on the number line,\n> e.g. write \"0 < something\" or \"negative < 0\" to let readers more\n> easily visualize in what relation on the number line the quantity of\n> each side of the comparison stands, here is a reference to a long\n> and amusing thread:\n> \n>   http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912\n\nI do not necessarily agree with the \"always use less-than\" style, but as\na reviewer of this series, it took me an extra minute to figure out what\nwas going on because two things changed. If the diff instead looked\nlike:\n\ndiff --git a/bundle.c b/bundle.c\nindex 505e07e..a9c1335 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t * followed by SP and subject line.\n \t\t */\n \t\tif (get_sha1_hex(buf.buf, sha1) ||\n-\t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n+\t\t    (40 < buf.len && !isspace(buf.buf[40])) ||\n \t\t    (!is_prereq && buf.len <= 40)) {\n \t\t\tif (report_path)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n\nthen it is immediately obvious that we are only impacting the case where\nbuf.len is exactly 40 (and it is even more obvious if you happen to use\nthe diff-highlight script, which highlights the single changed\ncharacter).\n\nJust my two cents as a reader of the patch. Other than that, it looks\ncorrect to me. :)\n\n-Peff\n"},{"id":"213504","messageId":"20130408095303.GA10392@blizzard","threadId":"33417","inReplyTo":"20130408010610.GB24030@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] bundle: Accept prerequisites without commit messages","fromName":"Lukas Fleischer","fromEmail":"git@cryptocrack.de","sentAt":"2013-04-08T09:53:03Z","receivedAt":"2013-04-08T09:53:03Z","isPatch":true,"sender":{"key":"git@cryptocrack.de","avatar":null},"body":"On Sun, Apr 07, 2013 at 09:06:10PM -0400, Jeff King wrote:\n> On Sun, Apr 07, 2013 at 10:21:33AM -0700, Junio C Hamano wrote:\n> \n> > As to the order of comparison to match the order on the number line,\n> > e.g. write \"0 < something\" or \"negative < 0\" to let readers more\n> > easily visualize in what relation on the number line the quantity of\n> > each side of the comparison stands, here is a reference to a long\n> > and amusing thread:\n> > \n> >   http://thread.gmane.org/gmane.comp.version-control.git/3903/focus=3912\n> \n> I do not necessarily agree with the \"always use less-than\" style, but as\n> a reviewer of this series, it took me an extra minute to figure out what\n> was going on because two things changed. If the diff instead looked\n> like:\n> \n> diff --git a/bundle.c b/bundle.c\n> index 505e07e..a9c1335 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -57,7 +57,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n>  \t\t * followed by SP and subject line.\n>  \t\t */\n>  \t\tif (get_sha1_hex(buf.buf, sha1) ||\n> -\t\t    (40 <= buf.len && !isspace(buf.buf[40])) ||\n> +\t\t    (40 < buf.len && !isspace(buf.buf[40])) ||\n>  \t\t    (!is_prereq && buf.len <= 40)) {\n>  \t\t\tif (report_path)\n>  \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n> \n> then it is immediately obvious that we are only impacting the case where\n> buf.len is exactly 40 (and it is even more obvious if you happen to use\n> the diff-highlight script, which highlights the single changed\n> character).\n\nI changed it for the very same reason -- it took me an extra minute to\nfigure out what is going on when trying to pinpoint the bug (it was\nespecially weird since we use \"40 <= buf.len\" here and \"buf.len <= 40\"\none line below -- which kind of makes sense now, though). Thanks for the\nreview (and merging), I won't change operand order in future patches :)\n\n> \n> Just my two cents as a reader of the patch. Other than that, it looks\n> correct to me. :)\n> \n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"}]}