{"thread":{"id":"57346","subject":"[PATCH 1/2] format-patch: Fix antipatterns in tests","startedAt":"2022-01-31T23:23:24Z","lastAt":"2022-02-02T04:20:25Z","messageCount":11,"participants":["Jerry Zhang","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"447381","messageId":"20220131232318.8248-1-jerry@skydio.com","threadId":"57346","inReplyTo":null,"subject":"[PATCH 1/2] format-patch: Fix antipatterns in tests","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-31T23:23:17Z","receivedAt":"2022-01-31T23:23:24Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Clean up the tests for format-patch by moving file preparation\ntasks inside the test body and redirecting files directly into\nstdin instead of using 'cat'.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\n t/t4204-patch-id.sh | 64 ++++++++++++++++++++++-----------------------\n 1 file changed, 31 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 80f4a65b28..da60f5b472 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' '\n \t\tcd subdir &&\n \t\ttest_patch_id irrelevant patchid.stable=true\n \t)\n '\n \n-cat >nonl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..2e65efe 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-\\ No newline at end of file\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n-cat >withnl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..7898192 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n test_expect_success 'patch-id handles no-nl-at-eof markers' '\n-\tcat nonl | calc_patch_id nonl &&\n-\tcat withnl | calc_patch_id withnl &&\n+\tcat >nonl <<-EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..2e65efe 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\t\\ No newline at end of file\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcat >withnl <<-EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..7898192 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcalc_patch_id nonl <nonl &&\n+\tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447382","messageId":"20220131232318.8248-2-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131232318.8248-1-jerry@skydio.com","subject":"[PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-31T23:23:18Z","receivedAt":"2022-01-31T23:23:27Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Normally diffs will contain a hunk header of the format\n\"@@ -2,2 +2,15 @@ code\". However when there is only 1 line of\nchange, the unified diff format allows for the second comma\nseparated value to be omitted in either before or after\nline counts.\n\nThis can produce hunk headers that look like\n\"@@ -2 +2,18 @@ code\" or \"@@ -2,2 +2 @@ code\".\nAs a result, scan_hunk_header mistakenly returns the line\nnumber as line count, which then results in unpredictable\nparsing errors with the rest of the patch, including giving\nmultiple lines of output for a single commit.\n\nFix by explicitly setting line count to 1 when there is\nno comma, and add a test.\n\napply.c contains this same logic except it is correct. A\nworthwhile future project might be to unify these two diff\nparsers so they both benefit from fixes.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV2->V3:\n- Made it clearer that the 1 line case is the only one where\nunified diff would use this particular format.\n- Cleaned up test and made separate patch to clean up old test.\n\n builtin/patch-id.c  |  9 +++++++--\n t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 822ffff51f..881fcf3273 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n \tq = p + 4;\n \tn = strspn(q, digits);\n \tif (q[n] == ',') {\n \t\tq += n + 1;\n+\t\t*p_before = atoi(q);\n \t\tn = strspn(q, digits);\n+\t} else {\n+\t\t*p_before = 1;\n \t}\n+\n \tif (n == 0 || q[n] != ' ' || q[n+1] != '+')\n \t\treturn 0;\n \n \tr = q + n + 2;\n \tn = strspn(r, digits);\n \tif (r[n] == ',') {\n \t\tr += n + 1;\n+\t\t*p_after = atoi(r);\n \t\tn = strspn(r, digits);\n+\t} else {\n+\t\t*p_after = 1;\n \t}\n \tif (n == 0)\n \t\treturn 0;\n \n-\t*p_before = atoi(q);\n-\t*p_after = atoi(r);\n \treturn 1;\n }\n \n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t   struct strbuf *line_buf, int stable)\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex da60f5b472..686ecc3c18 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '\n calc_patch_id () {\n \tpatch_name=\"$1\"\n \tshift\n \tgit patch-id \"$@\" >patch-id.output &&\n \tsed \"s/ .*//\" patch-id.output >patch-id_\"$patch_name\" &&\n-\ttest_line_count -gt 0 patch-id_\"$patch_name\"\n+\ttest_line_count -eq 1 patch-id_\"$patch_name\"\n }\n \n get_top_diff () {\n \tgit log -p -1 \"$@\" -O bar-then-foo --\n }\n@@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' '\n \tEOF\n \tcalc_patch_id nonl <nonl &&\n \tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n+\n+test_expect_success 'patch-id handles diffs with one line of before/after' '\n+\tcat >diffu1 <<-EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tdiff --git a/car b/car\n+\tindex 00750ed..2ae5e34 100644\n+\t--- a/car\n+\t+++ b/car\n+\t@@ -1 +1,2 @@\n+\t 3\n+\t+d\n+\tdiff --git a/foo b/foo\n+\tindex e439850..7146eb8 100644\n+\t--- a/foo\n+\t+++ b/foo\n+\t@@ -2 +2,2 @@\n+\t a\n+\t+e\n+\tEOF\n+\tcalc_patch_id diffu1 <diffu1 &&\n+\ttest_config patchid.stable true &&\n+\tcalc_patch_id diffu1stable <diffu1\n+'\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447383","messageId":"20220131232529.8484-1-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131232318.8248-1-jerry@skydio.com","subject":"[PATCH V2 1/2] patch-id: Fix antipatterns in tests","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-31T23:25:29Z","receivedAt":"2022-01-31T23:25:36Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Clean up the tests for patch-id by moving file preparation\ntasks inside the test body and redirecting files directly into\nstdin instead of using 'cat'.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV1->V2:\n- For some reason I put format-patch in the commit text when this\nchange is actually to patch-id.\n\n t/t4204-patch-id.sh | 64 ++++++++++++++++++++++-----------------------\n 1 file changed, 31 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 80f4a65b28..da60f5b472 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' '\n \t\tcd subdir &&\n \t\ttest_patch_id irrelevant patchid.stable=true\n \t)\n '\n \n-cat >nonl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..2e65efe 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-\\ No newline at end of file\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n-cat >withnl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..7898192 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n test_expect_success 'patch-id handles no-nl-at-eof markers' '\n-\tcat nonl | calc_patch_id nonl &&\n-\tcat withnl | calc_patch_id withnl &&\n+\tcat >nonl <<-EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..2e65efe 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\t\\ No newline at end of file\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcat >withnl <<-EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..7898192 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcalc_patch_id nonl <nonl &&\n+\tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447385","messageId":"xmqqfsp3h3zo.fsf@gitster.g","threadId":"57346","inReplyTo":"20220131232529.8484-1-jerry@skydio.com","subject":"Re: [PATCH V2 1/2] patch-id: Fix antipatterns in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T23:36:43Z","receivedAt":"2022-01-31T23:36:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n>  test_expect_success 'patch-id handles no-nl-at-eof markers' '\n> -\tcat nonl | calc_patch_id nonl &&\n> -\tcat withnl | calc_patch_id withnl &&\n> +\tcat >nonl <<-EOF &&\n\nUnless you use $variable_expanded_to_its_value in the here-doc,\nalways make it a habit to quote the EOF marker.  That helps the\nreaders by assuring that there is no funny interpolation going on.\n\n> +\tdiff --git i/a w/a\n> +\tindex e69de29..2e65efe 100644\n> +\t--- i/a\n> +\t+++ w/a\n> +\t@@ -0,0 +1 @@\n> +\t+a\n> +\t\\ No newline at end of file\n> +\tdiff --git i/b w/b\n> +\tindex e69de29..6178079 100644\n> +\t--- i/b\n> +\t+++ w/b\n> +\t@@ -0,0 +1 @@\n> +\t+b\n> +\tEOF\n> +\tcat >withnl <<-EOF &&\n\nLikewise.\n\n> +\tdiff --git i/a w/a\n> +\tindex e69de29..7898192 100644\n> +\t--- i/a\n> +\t+++ w/a\n> +\t@@ -0,0 +1 @@\n> +\t+a\n> +\tdiff --git i/b w/b\n> +\tindex e69de29..6178079 100644\n> +\t--- i/b\n> +\t+++ w/b\n> +\t@@ -0,0 +1 @@\n> +\t+b\n> +\tEOF\n> +\tcalc_patch_id nonl <nonl &&\n> +\tcalc_patch_id withnl <withnl &&\n>  \ttest_cmp patch-id_nonl patch-id_withnl\n>  '\n>  test_done\n"},{"id":"447386","messageId":"20220131235218.27392-1-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131232529.8484-1-jerry@skydio.com","subject":"[PATCH V3 1/2] patch-id: Fix antipatterns in tests","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-31T23:52:18Z","receivedAt":"2022-01-31T23:52:26Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Clean up the tests for patch-id by moving file preparation\ntasks inside the test body and redirecting files directly into\nstdin instead of using 'cat'.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV2->V3:\n- Quote the EOF marker\n\n t/t4204-patch-id.sh | 64 ++++++++++++++++++++++-----------------------\n 1 file changed, 31 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 80f4a65b28..a4b8f2b9ca 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' '\n \t\tcd subdir &&\n \t\ttest_patch_id irrelevant patchid.stable=true\n \t)\n '\n \n-cat >nonl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..2e65efe 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-\\ No newline at end of file\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n-cat >withnl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..7898192 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n test_expect_success 'patch-id handles no-nl-at-eof markers' '\n-\tcat nonl | calc_patch_id nonl &&\n-\tcat withnl | calc_patch_id withnl &&\n+\tcat >nonl <<-'EOF' &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..2e65efe 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\t\\ No newline at end of file\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\t'EOF'\n+\tcat >withnl <<-'EOF' &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..7898192 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\t'EOF'\n+\tcalc_patch_id nonl <nonl &&\n+\tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447387","messageId":"20220131235244.27429-1-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131232318.8248-2-jerry@skydio.com","subject":"[PATCH V4 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-01-31T23:52:44Z","receivedAt":"2022-01-31T23:52:49Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Normally diffs will contain a hunk header of the format\n\"@@ -2,2 +2,15 @@ code\". However when there is only 1 line of\nchange, the unified diff format allows for the second comma\nseparated value to be omitted in either before or after\nline counts.\n\nThis can produce hunk headers that look like\n\"@@ -2 +2,18 @@ code\" or \"@@ -2,2 +2 @@ code\".\nAs a result, scan_hunk_header mistakenly returns the line\nnumber as line count, which then results in unpredictable\nparsing errors with the rest of the patch, including giving\nmultiple lines of output for a single commit.\n\nFix by explicitly setting line count to 1 when there is\nno comma, and add a test.\n\napply.c contains this same logic except it is correct. A\nworthwhile future project might be to unify these two diff\nparsers so they both benefit from fixes.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV3->V4:\n- Quote the EOF marker\n\n builtin/patch-id.c  |  9 +++++++--\n t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 822ffff51f..881fcf3273 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n \tq = p + 4;\n \tn = strspn(q, digits);\n \tif (q[n] == ',') {\n \t\tq += n + 1;\n+\t\t*p_before = atoi(q);\n \t\tn = strspn(q, digits);\n+\t} else {\n+\t\t*p_before = 1;\n \t}\n+\n \tif (n == 0 || q[n] != ' ' || q[n+1] != '+')\n \t\treturn 0;\n \n \tr = q + n + 2;\n \tn = strspn(r, digits);\n \tif (r[n] == ',') {\n \t\tr += n + 1;\n+\t\t*p_after = atoi(r);\n \t\tn = strspn(r, digits);\n+\t} else {\n+\t\t*p_after = 1;\n \t}\n \tif (n == 0)\n \t\treturn 0;\n \n-\t*p_before = atoi(q);\n-\t*p_after = atoi(r);\n \treturn 1;\n }\n \n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t   struct strbuf *line_buf, int stable)\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a4b8f2b9ca..0e73af747f 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '\n calc_patch_id () {\n \tpatch_name=\"$1\"\n \tshift\n \tgit patch-id \"$@\" >patch-id.output &&\n \tsed \"s/ .*//\" patch-id.output >patch-id_\"$patch_name\" &&\n-\ttest_line_count -gt 0 patch-id_\"$patch_name\"\n+\ttest_line_count -eq 1 patch-id_\"$patch_name\"\n }\n \n get_top_diff () {\n \tgit log -p -1 \"$@\" -O bar-then-foo --\n }\n@@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' '\n \t'EOF'\n \tcalc_patch_id nonl <nonl &&\n \tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n+\n+test_expect_success 'patch-id handles diffs with one line of before/after' '\n+\tcat >diffu1 <<-'EOF' &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tdiff --git a/car b/car\n+\tindex 00750ed..2ae5e34 100644\n+\t--- a/car\n+\t+++ b/car\n+\t@@ -1 +1,2 @@\n+\t 3\n+\t+d\n+\tdiff --git a/foo b/foo\n+\tindex e439850..7146eb8 100644\n+\t--- a/foo\n+\t+++ b/foo\n+\t@@ -2 +2,2 @@\n+\t a\n+\t+e\n+\t'EOF'\n+\tcalc_patch_id diffu1 <diffu1 &&\n+\ttest_config patchid.stable true &&\n+\tcalc_patch_id diffu1stable <diffu1\n+'\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447471","messageId":"d9275c31-7558-fc9c-9eb3-2a0cb81a8259@kdbg.org","threadId":"57346","inReplyTo":"20220131235218.27392-1-jerry@skydio.com","subject":"Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-02-01T22:07:31Z","receivedAt":"2022-02-01T22:07:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.02.22 um 00:52 schrieb Jerry Zhang:\n> Clean up the tests for patch-id by moving file preparation\n> tasks inside the test body and redirecting files directly into\n> stdin instead of using 'cat'.\n\nYou announce that `cat` is about to be removed...\n\n>  test_expect_success 'patch-id handles no-nl-at-eof markers' '\n> -\tcat nonl | calc_patch_id nonl &&\n> -\tcat withnl | calc_patch_id withnl &&\n> +\tcat >nonl <<-'EOF' &&\n\n... but it is still here...\n\n> +\tdiff --git i/a w/a\n> +\tindex e69de29..2e65efe 100644\n> +\t--- i/a\n> +\t+++ w/a\n> +\t@@ -0,0 +1 @@\n> +\t+a\n> +\t\\ No newline at end of file\n> +\tdiff --git i/b w/b\n> +\tindex e69de29..6178079 100644\n> +\t--- i/b\n> +\t+++ w/b\n> +\t@@ -0,0 +1 @@\n> +\t+b\n> +\t'EOF'\n> +\tcat >withnl <<-'EOF' &&\n\n... and here, although...\n\n> +\tdiff --git i/a w/a\n> +\tindex e69de29..7898192 100644\n> +\t--- i/a\n> +\t+++ w/a\n> +\t@@ -0,0 +1 @@\n> +\t+a\n> +\tdiff --git i/b w/b\n> +\tindex e69de29..6178079 100644\n> +\t--- i/b\n> +\t+++ w/b\n> +\t@@ -0,0 +1 @@\n> +\t+b\n> +\t'EOF'\n> +\tcalc_patch_id nonl <nonl &&\n> +\tcalc_patch_id withnl <withnl &&\n\n... you could in fact just redirect the here-documents into these commands.\n\n>  \ttest_cmp patch-id_nonl patch-id_withnl\n>  '\n>  test_done\n\n-- Hannes\n"},{"id":"447476","messageId":"xmqqy22u9nzr.fsf@gitster.g","threadId":"57346","inReplyTo":"20220131235218.27392-1-jerry@skydio.com","subject":"Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T23:16:24Z","receivedAt":"2022-02-01T23:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang <jerry@skydio.com> writes:\n\n> Clean up the tests for patch-id by moving file preparation\n> tasks inside the test body and redirecting files directly into\n> stdin instead of using 'cat'.\n>\n> Signed-off-by: Jerry Zhang <jerry@skydio.com>\n> ---\n> V2->V3:\n> - Quote the EOF marker\n\n\nYes but no.\n\n>  test_expect_success 'patch-id handles no-nl-at-eof markers' '\n> -\tcat nonl | calc_patch_id nonl &&\n> -\tcat withnl | calc_patch_id withnl &&\n> +\tcat >nonl <<-'EOF' &&\n\nWe started the \"executable\" part of the test_expect_success as a\nsingle-quoted string, and then after writing <<-, we stepped out of\nthat single-quote pair.  Then we are writing E O F unquoted, and\nstepped back into another single-quote pair here.  So, to the shell\nthat runs this executable part, it is exactly the same as\n\n\tcat >nonl <<-EOF &&\n\nside note: if it were not in a plain shell script (not the\nexecutable part that is passed as a single string to the\ntest_expect_success function as an argument), what we see above,\nquoting EOF within a pair of single-quotes, is perfectly acceptable\nthing to do.  But not here, for the reasons explained above.\n\n> +\tdiff --git i/a w/a\n> +\tindex e69de29..2e65efe 100644\n> +\t--- i/a\n> +\t+++ w/a\n> +\t@@ -0,0 +1 @@\n> +\t+a\n> +\t\\ No newline at end of file\n> +\tdiff --git i/b w/b\n> +\tindex e69de29..6178079 100644\n> +\t--- i/b\n> +\t+++ w/b\n> +\t@@ -0,0 +1 @@\n> +\t+b\n> +\t'EOF'\n\nSame here.  It is exactly the same as writing EOF without any quotes\naround it, just like the opening one we saw earlier.\n\nIn other words, the above is not quoting at all.\n\nI think I demonstrated the way we should write this in my earlier\nreview when I pointed out this exiting issue this step is fixing\n(https://lore.kernel.org/git/xmqqmtjbh5fu.fsf@gitster.g/):\n\n\ttest_expect_success \"title string\" '\n\t\t...\n\t\tcommand <<-\\EOF &&\n\t\there document indented by tab\n\t\tmore document\n\t\tEOF\n"},{"id":"447477","messageId":"xmqqtudi9nwq.fsf@gitster.g","threadId":"57346","inReplyTo":"d9275c31-7558-fc9c-9eb3-2a0cb81a8259@kdbg.org","subject":"Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T23:18:13Z","receivedAt":"2022-02-01T23:18:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> +\tcalc_patch_id nonl <nonl &&\n>> +\tcalc_patch_id withnl <withnl &&\n>\n> ... you could in fact just redirect the here-documents into these commands.\n\nThat was part of my original suggestion, and then I realized that\nnonl and withnl may later be reused and rewrote it before sending my\nreview comment.\n\n\n"},{"id":"447500","messageId":"20220202041945.10077-1-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131235244.27429-1-jerry@skydio.com","subject":"[PATCH V5 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-02-02T04:19:45Z","receivedAt":"2022-02-02T04:20:01Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Normally diffs will contain a hunk header of the format\n\"@@ -2,2 +2,15 @@ code\". However when there is only 1 line of\nchange, the unified diff format allows for the second comma\nseparated value to be omitted in either before or after\nline counts.\n\nThis can produce hunk headers that look like\n\"@@ -2 +2,18 @@ code\" or \"@@ -2,2 +2 @@ code\".\nAs a result, scan_hunk_header mistakenly returns the line\nnumber as line count, which then results in unpredictable\nparsing errors with the rest of the patch, including giving\nmultiple lines of output for a single commit.\n\nFix by explicitly setting line count to 1 when there is\nno comma, and add a test.\n\napply.c contains this same logic except it is correct. A\nworthwhile future project might be to unify these two diff\nparsers so they both benefit from fixes.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV4->V5:\n- Quote the EOF marker correctly\n\n builtin/patch-id.c  |  9 +++++++--\n t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 822ffff51f..881fcf3273 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \n \tq = p + 4;\n \tn = strspn(q, digits);\n \tif (q[n] == ',') {\n \t\tq += n + 1;\n+\t\t*p_before = atoi(q);\n \t\tn = strspn(q, digits);\n+\t} else {\n+\t\t*p_before = 1;\n \t}\n+\n \tif (n == 0 || q[n] != ' ' || q[n+1] != '+')\n \t\treturn 0;\n \n \tr = q + n + 2;\n \tn = strspn(r, digits);\n \tif (r[n] == ',') {\n \t\tr += n + 1;\n+\t\t*p_after = atoi(r);\n \t\tn = strspn(r, digits);\n+\t} else {\n+\t\t*p_after = 1;\n \t}\n \tif (n == 0)\n \t\treturn 0;\n \n-\t*p_before = atoi(q);\n-\t*p_after = atoi(r);\n \treturn 1;\n }\n \n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t   struct strbuf *line_buf, int stable)\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 2bc940a07e..a730c0db98 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' '\n calc_patch_id () {\n \tpatch_name=\"$1\"\n \tshift\n \tgit patch-id \"$@\" >patch-id.output &&\n \tsed \"s/ .*//\" patch-id.output >patch-id_\"$patch_name\" &&\n-\ttest_line_count -gt 0 patch-id_\"$patch_name\"\n+\ttest_line_count -eq 1 patch-id_\"$patch_name\"\n }\n \n get_top_diff () {\n \tgit log -p -1 \"$@\" -O bar-then-foo --\n }\n@@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' '\n \tEOF\n \tcalc_patch_id nonl <nonl &&\n \tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n+\n+test_expect_success 'patch-id handles diffs with one line of before/after' '\n+\tcat >diffu1 <<-\\EOF &&\n+\tdiff --git a/bar b/bar\n+\tindex bdaf90f..31051f6 100644\n+\t--- a/bar\n+\t+++ b/bar\n+\t@@ -2 +2,2 @@\n+\t b\n+\t+c\n+\tdiff --git a/car b/car\n+\tindex 00750ed..2ae5e34 100644\n+\t--- a/car\n+\t+++ b/car\n+\t@@ -1 +1,2 @@\n+\t 3\n+\t+d\n+\tdiff --git a/foo b/foo\n+\tindex e439850..7146eb8 100644\n+\t--- a/foo\n+\t+++ b/foo\n+\t@@ -2 +2,2 @@\n+\t a\n+\t+e\n+\tEOF\n+\tcalc_patch_id diffu1 <diffu1 &&\n+\ttest_config patchid.stable true &&\n+\tcalc_patch_id diffu1stable <diffu1\n+'\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"},{"id":"447501","messageId":"20220202042015.10115-1-jerry@skydio.com","threadId":"57346","inReplyTo":"20220131235218.27392-1-jerry@skydio.com","subject":"[PATCH V4 1/2] patch-id: Fix antipatterns in tests","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-02-02T04:20:15Z","receivedAt":"2022-02-02T04:20:25Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Clean up the tests for patch-id by moving file preparation\ntasks inside the test body and redirecting files directly into\nstdin instead of using 'cat'.\n\nSigned-off-by: Jerry Zhang <jerry@skydio.com>\n---\nV3->V4:\n- Quote the EOF marker correctly\n\n t/t4204-patch-id.sh | 64 ++++++++++++++++++++++-----------------------\n 1 file changed, 31 insertions(+), 33 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex 80f4a65b28..2bc940a07e 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' '\n \t\tcd subdir &&\n \t\ttest_patch_id irrelevant patchid.stable=true\n \t)\n '\n \n-cat >nonl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..2e65efe 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-\\ No newline at end of file\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n-cat >withnl <<\\EOF\n-diff --git i/a w/a\n-index e69de29..7898192 100644\n---- i/a\n-+++ w/a\n-@@ -0,0 +1 @@\n-+a\n-diff --git i/b w/b\n-index e69de29..6178079 100644\n---- i/b\n-+++ w/b\n-@@ -0,0 +1 @@\n-+b\n-EOF\n-\n test_expect_success 'patch-id handles no-nl-at-eof markers' '\n-\tcat nonl | calc_patch_id nonl &&\n-\tcat withnl | calc_patch_id withnl &&\n+\tcat >nonl <<-\\EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..2e65efe 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\t\\ No newline at end of file\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcat >withnl <<-\\EOF &&\n+\tdiff --git i/a w/a\n+\tindex e69de29..7898192 100644\n+\t--- i/a\n+\t+++ w/a\n+\t@@ -0,0 +1 @@\n+\t+a\n+\tdiff --git i/b w/b\n+\tindex e69de29..6178079 100644\n+\t--- i/b\n+\t+++ w/b\n+\t@@ -0,0 +1 @@\n+\t+b\n+\tEOF\n+\tcalc_patch_id nonl <nonl &&\n+\tcalc_patch_id withnl <withnl &&\n \ttest_cmp patch-id_nonl patch-id_withnl\n '\n test_done\n-- \n2.32.0.1314.g6ed4fcc4cc\n\n"}]}