* [PATCH 1/2] format-patch: Fix antipatterns in tests @ 2022-01-31 23:23 Jerry Zhang 2022-01-31 23:23 ` [PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after Jerry Zhang 2022-01-31 23:25 ` [PATCH V2 1/2] patch-id: Fix antipatterns in tests Jerry Zhang 0 siblings, 2 replies; 11+ messages in thread From: Jerry Zhang @ 2022-01-31 23:23 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Clean up the tests for format-patch by moving file preparation tasks inside the test body and redirecting files directly into stdin instead of using 'cat'. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- t/t4204-patch-id.sh | 64 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 80f4a65b28..da60f5b472 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' ' cd subdir && test_patch_id irrelevant patchid.stable=true ) ' -cat >nonl <<\EOF -diff --git i/a w/a -index e69de29..2e65efe 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -\ No newline at end of file -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - -cat >withnl <<\EOF -diff --git i/a w/a -index e69de29..7898192 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - test_expect_success 'patch-id handles no-nl-at-eof markers' ' - cat nonl | calc_patch_id nonl && - cat withnl | calc_patch_id withnl && + cat >nonl <<-EOF && + diff --git i/a w/a + index e69de29..2e65efe 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + \ No newline at end of file + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + cat >withnl <<-EOF && + diff --git i/a w/a + index e69de29..7898192 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + calc_patch_id nonl <nonl && + calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after 2022-01-31 23:23 [PATCH 1/2] format-patch: Fix antipatterns in tests Jerry Zhang @ 2022-01-31 23:23 ` Jerry Zhang 2022-01-31 23:52 ` [PATCH V4 " Jerry Zhang 2022-01-31 23:25 ` [PATCH V2 1/2] patch-id: Fix antipatterns in tests Jerry Zhang 1 sibling, 1 reply; 11+ messages in thread From: Jerry Zhang @ 2022-01-31 23:23 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Normally diffs will contain a hunk header of the format "@@ -2,2 +2,15 @@ code". However when there is only 1 line of change, the unified diff format allows for the second comma separated value to be omitted in either before or after line counts. This can produce hunk headers that look like "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code". As a result, scan_hunk_header mistakenly returns the line number as line count, which then results in unpredictable parsing errors with the rest of the patch, including giving multiple lines of output for a single commit. Fix by explicitly setting line count to 1 when there is no comma, and add a test. apply.c contains this same logic except it is correct. A worthwhile future project might be to unify these two diff parsers so they both benefit from fixes. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V2->V3: - Made it clearer that the 1 line case is the only one where unified diff would use this particular format. - Cleaned up test and made separate patch to clean up old test. builtin/patch-id.c | 9 +++++++-- t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 822ffff51f..881fcf3273 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after) q = p + 4; n = strspn(q, digits); if (q[n] == ',') { q += n + 1; + *p_before = atoi(q); n = strspn(q, digits); + } else { + *p_before = 1; } + if (n == 0 || q[n] != ' ' || q[n+1] != '+') return 0; r = q + n + 2; n = strspn(r, digits); if (r[n] == ',') { r += n + 1; + *p_after = atoi(r); n = strspn(r, digits); + } else { + *p_after = 1; } if (n == 0) return 0; - *p_before = atoi(q); - *p_after = atoi(r); return 1; } static int get_one_patchid(struct object_id *next_oid, struct object_id *result, struct strbuf *line_buf, int stable) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index da60f5b472..686ecc3c18 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' ' calc_patch_id () { patch_name="$1" shift git patch-id "$@" >patch-id.output && sed "s/ .*//" patch-id.output >patch-id_"$patch_name" && - test_line_count -gt 0 patch-id_"$patch_name" + test_line_count -eq 1 patch-id_"$patch_name" } get_top_diff () { git log -p -1 "$@" -O bar-then-foo -- } @@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' ' EOF calc_patch_id nonl <nonl && calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' + +test_expect_success 'patch-id handles diffs with one line of before/after' ' + cat >diffu1 <<-EOF && + diff --git a/bar b/bar + index bdaf90f..31051f6 100644 + --- a/bar + +++ b/bar + @@ -2 +2,2 @@ + b + +c + diff --git a/car b/car + index 00750ed..2ae5e34 100644 + --- a/car + +++ b/car + @@ -1 +1,2 @@ + 3 + +d + diff --git a/foo b/foo + index e439850..7146eb8 100644 + --- a/foo + +++ b/foo + @@ -2 +2,2 @@ + a + +e + EOF + calc_patch_id diffu1 <diffu1 && + test_config patchid.stable true && + calc_patch_id diffu1stable <diffu1 +' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V4 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after 2022-01-31 23:23 ` [PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after Jerry Zhang @ 2022-01-31 23:52 ` Jerry Zhang 2022-02-02 4:19 ` [PATCH V5 " Jerry Zhang 0 siblings, 1 reply; 11+ messages in thread From: Jerry Zhang @ 2022-01-31 23:52 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Normally diffs will contain a hunk header of the format "@@ -2,2 +2,15 @@ code". However when there is only 1 line of change, the unified diff format allows for the second comma separated value to be omitted in either before or after line counts. This can produce hunk headers that look like "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code". As a result, scan_hunk_header mistakenly returns the line number as line count, which then results in unpredictable parsing errors with the rest of the patch, including giving multiple lines of output for a single commit. Fix by explicitly setting line count to 1 when there is no comma, and add a test. apply.c contains this same logic except it is correct. A worthwhile future project might be to unify these two diff parsers so they both benefit from fixes. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V3->V4: - Quote the EOF marker builtin/patch-id.c | 9 +++++++-- t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 822ffff51f..881fcf3273 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after) q = p + 4; n = strspn(q, digits); if (q[n] == ',') { q += n + 1; + *p_before = atoi(q); n = strspn(q, digits); + } else { + *p_before = 1; } + if (n == 0 || q[n] != ' ' || q[n+1] != '+') return 0; r = q + n + 2; n = strspn(r, digits); if (r[n] == ',') { r += n + 1; + *p_after = atoi(r); n = strspn(r, digits); + } else { + *p_after = 1; } if (n == 0) return 0; - *p_before = atoi(q); - *p_after = atoi(r); return 1; } static int get_one_patchid(struct object_id *next_oid, struct object_id *result, struct strbuf *line_buf, int stable) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index a4b8f2b9ca..0e73af747f 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' ' calc_patch_id () { patch_name="$1" shift git patch-id "$@" >patch-id.output && sed "s/ .*//" patch-id.output >patch-id_"$patch_name" && - test_line_count -gt 0 patch-id_"$patch_name" + test_line_count -eq 1 patch-id_"$patch_name" } get_top_diff () { git log -p -1 "$@" -O bar-then-foo -- } @@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' ' 'EOF' calc_patch_id nonl <nonl && calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' + +test_expect_success 'patch-id handles diffs with one line of before/after' ' + cat >diffu1 <<-'EOF' && + diff --git a/bar b/bar + index bdaf90f..31051f6 100644 + --- a/bar + +++ b/bar + @@ -2 +2,2 @@ + b + +c + diff --git a/car b/car + index 00750ed..2ae5e34 100644 + --- a/car + +++ b/car + @@ -1 +1,2 @@ + 3 + +d + diff --git a/foo b/foo + index e439850..7146eb8 100644 + --- a/foo + +++ b/foo + @@ -2 +2,2 @@ + a + +e + 'EOF' + calc_patch_id diffu1 <diffu1 && + test_config patchid.stable true && + calc_patch_id diffu1stable <diffu1 +' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V5 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after 2022-01-31 23:52 ` [PATCH V4 " Jerry Zhang @ 2022-02-02 4:19 ` Jerry Zhang 0 siblings, 0 replies; 11+ messages in thread From: Jerry Zhang @ 2022-02-02 4:19 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Normally diffs will contain a hunk header of the format "@@ -2,2 +2,15 @@ code". However when there is only 1 line of change, the unified diff format allows for the second comma separated value to be omitted in either before or after line counts. This can produce hunk headers that look like "@@ -2 +2,18 @@ code" or "@@ -2,2 +2 @@ code". As a result, scan_hunk_header mistakenly returns the line number as line count, which then results in unpredictable parsing errors with the rest of the patch, including giving multiple lines of output for a single commit. Fix by explicitly setting line count to 1 when there is no comma, and add a test. apply.c contains this same logic except it is correct. A worthwhile future project might be to unify these two diff parsers so they both benefit from fixes. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V4->V5: - Quote the EOF marker correctly builtin/patch-id.c | 9 +++++++-- t/t4204-patch-id.sh | 31 ++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 822ffff51f..881fcf3273 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -30,26 +30,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after) q = p + 4; n = strspn(q, digits); if (q[n] == ',') { q += n + 1; + *p_before = atoi(q); n = strspn(q, digits); + } else { + *p_before = 1; } + if (n == 0 || q[n] != ' ' || q[n+1] != '+') return 0; r = q + n + 2; n = strspn(r, digits); if (r[n] == ',') { r += n + 1; + *p_after = atoi(r); n = strspn(r, digits); + } else { + *p_after = 1; } if (n == 0) return 0; - *p_before = atoi(q); - *p_after = atoi(r); return 1; } static int get_one_patchid(struct object_id *next_oid, struct object_id *result, struct strbuf *line_buf, int stable) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 2bc940a07e..a730c0db98 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -36,11 +36,11 @@ test_expect_success 'patch-id output is well-formed' ' calc_patch_id () { patch_name="$1" shift git patch-id "$@" >patch-id.output && sed "s/ .*//" patch-id.output >patch-id_"$patch_name" && - test_line_count -gt 0 patch-id_"$patch_name" + test_line_count -eq 1 patch-id_"$patch_name" } get_top_diff () { git log -p -1 "$@" -O bar-then-foo -- } @@ -198,6 +198,35 @@ test_expect_success 'patch-id handles no-nl-at-eof markers' ' EOF calc_patch_id nonl <nonl && calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' + +test_expect_success 'patch-id handles diffs with one line of before/after' ' + cat >diffu1 <<-\EOF && + diff --git a/bar b/bar + index bdaf90f..31051f6 100644 + --- a/bar + +++ b/bar + @@ -2 +2,2 @@ + b + +c + diff --git a/car b/car + index 00750ed..2ae5e34 100644 + --- a/car + +++ b/car + @@ -1 +1,2 @@ + 3 + +d + diff --git a/foo b/foo + index e439850..7146eb8 100644 + --- a/foo + +++ b/foo + @@ -2 +2,2 @@ + a + +e + EOF + calc_patch_id diffu1 <diffu1 && + test_config patchid.stable true && + calc_patch_id diffu1stable <diffu1 +' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V2 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:23 [PATCH 1/2] format-patch: Fix antipatterns in tests Jerry Zhang 2022-01-31 23:23 ` [PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after Jerry Zhang @ 2022-01-31 23:25 ` Jerry Zhang 2022-01-31 23:36 ` Junio C Hamano 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang 1 sibling, 2 replies; 11+ messages in thread From: Jerry Zhang @ 2022-01-31 23:25 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Clean up the tests for patch-id by moving file preparation tasks inside the test body and redirecting files directly into stdin instead of using 'cat'. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V1->V2: - For some reason I put format-patch in the commit text when this change is actually to patch-id. t/t4204-patch-id.sh | 64 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 80f4a65b28..da60f5b472 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' ' cd subdir && test_patch_id irrelevant patchid.stable=true ) ' -cat >nonl <<\EOF -diff --git i/a w/a -index e69de29..2e65efe 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -\ No newline at end of file -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - -cat >withnl <<\EOF -diff --git i/a w/a -index e69de29..7898192 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - test_expect_success 'patch-id handles no-nl-at-eof markers' ' - cat nonl | calc_patch_id nonl && - cat withnl | calc_patch_id withnl && + cat >nonl <<-EOF && + diff --git i/a w/a + index e69de29..2e65efe 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + \ No newline at end of file + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + cat >withnl <<-EOF && + diff --git i/a w/a + index e69de29..7898192 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + calc_patch_id nonl <nonl && + calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:25 ` [PATCH V2 1/2] patch-id: Fix antipatterns in tests Jerry Zhang @ 2022-01-31 23:36 ` Junio C Hamano 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-01-31 23:36 UTC (permalink / raw) To: Jerry Zhang; +Cc: git Jerry Zhang <jerry@skydio.com> writes: > test_expect_success 'patch-id handles no-nl-at-eof markers' ' > - cat nonl | calc_patch_id nonl && > - cat withnl | calc_patch_id withnl && > + cat >nonl <<-EOF && Unless you use $variable_expanded_to_its_value in the here-doc, always make it a habit to quote the EOF marker. That helps the readers by assuring that there is no funny interpolation going on. > + diff --git i/a w/a > + index e69de29..2e65efe 100644 > + --- i/a > + +++ w/a > + @@ -0,0 +1 @@ > + +a > + \ No newline at end of file > + diff --git i/b w/b > + index e69de29..6178079 100644 > + --- i/b > + +++ w/b > + @@ -0,0 +1 @@ > + +b > + EOF > + cat >withnl <<-EOF && Likewise. > + diff --git i/a w/a > + index e69de29..7898192 100644 > + --- i/a > + +++ w/a > + @@ -0,0 +1 @@ > + +a > + diff --git i/b w/b > + index e69de29..6178079 100644 > + --- i/b > + +++ w/b > + @@ -0,0 +1 @@ > + +b > + EOF > + calc_patch_id nonl <nonl && > + calc_patch_id withnl <withnl && > test_cmp patch-id_nonl patch-id_withnl > ' > test_done ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V3 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:25 ` [PATCH V2 1/2] patch-id: Fix antipatterns in tests Jerry Zhang 2022-01-31 23:36 ` Junio C Hamano @ 2022-01-31 23:52 ` Jerry Zhang 2022-02-01 22:07 ` Johannes Sixt ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Jerry Zhang @ 2022-01-31 23:52 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Clean up the tests for patch-id by moving file preparation tasks inside the test body and redirecting files directly into stdin instead of using 'cat'. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V2->V3: - Quote the EOF marker t/t4204-patch-id.sh | 64 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 80f4a65b28..a4b8f2b9ca 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' ' cd subdir && test_patch_id irrelevant patchid.stable=true ) ' -cat >nonl <<\EOF -diff --git i/a w/a -index e69de29..2e65efe 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -\ No newline at end of file -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - -cat >withnl <<\EOF -diff --git i/a w/a -index e69de29..7898192 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - test_expect_success 'patch-id handles no-nl-at-eof markers' ' - cat nonl | calc_patch_id nonl && - cat withnl | calc_patch_id withnl && + cat >nonl <<-'EOF' && + diff --git i/a w/a + index e69de29..2e65efe 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + \ No newline at end of file + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + 'EOF' + cat >withnl <<-'EOF' && + diff --git i/a w/a + index e69de29..7898192 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + 'EOF' + calc_patch_id nonl <nonl && + calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang @ 2022-02-01 22:07 ` Johannes Sixt 2022-02-01 23:18 ` Junio C Hamano 2022-02-01 23:16 ` Junio C Hamano 2022-02-02 4:20 ` [PATCH V4 " Jerry Zhang 2 siblings, 1 reply; 11+ messages in thread From: Johannes Sixt @ 2022-02-01 22:07 UTC (permalink / raw) To: Jerry Zhang; +Cc: git, gitster Am 01.02.22 um 00:52 schrieb Jerry Zhang: > Clean up the tests for patch-id by moving file preparation > tasks inside the test body and redirecting files directly into > stdin instead of using 'cat'. You announce that `cat` is about to be removed... > test_expect_success 'patch-id handles no-nl-at-eof markers' ' > - cat nonl | calc_patch_id nonl && > - cat withnl | calc_patch_id withnl && > + cat >nonl <<-'EOF' && ... but it is still here... > + diff --git i/a w/a > + index e69de29..2e65efe 100644 > + --- i/a > + +++ w/a > + @@ -0,0 +1 @@ > + +a > + \ No newline at end of file > + diff --git i/b w/b > + index e69de29..6178079 100644 > + --- i/b > + +++ w/b > + @@ -0,0 +1 @@ > + +b > + 'EOF' > + cat >withnl <<-'EOF' && ... and here, although... > + diff --git i/a w/a > + index e69de29..7898192 100644 > + --- i/a > + +++ w/a > + @@ -0,0 +1 @@ > + +a > + diff --git i/b w/b > + index e69de29..6178079 100644 > + --- i/b > + +++ w/b > + @@ -0,0 +1 @@ > + +b > + 'EOF' > + calc_patch_id nonl <nonl && > + calc_patch_id withnl <withnl && ... you could in fact just redirect the here-documents into these commands. > test_cmp patch-id_nonl patch-id_withnl > ' > test_done -- Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests 2022-02-01 22:07 ` Johannes Sixt @ 2022-02-01 23:18 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-02-01 23:18 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jerry Zhang, git Johannes Sixt <j6t@kdbg.org> writes: >> + calc_patch_id nonl <nonl && >> + calc_patch_id withnl <withnl && > > ... you could in fact just redirect the here-documents into these commands. That was part of my original suggestion, and then I realized that nonl and withnl may later be reused and rewrote it before sending my review comment. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V3 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang 2022-02-01 22:07 ` Johannes Sixt @ 2022-02-01 23:16 ` Junio C Hamano 2022-02-02 4:20 ` [PATCH V4 " Jerry Zhang 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2022-02-01 23:16 UTC (permalink / raw) To: Jerry Zhang; +Cc: git Jerry Zhang <jerry@skydio.com> writes: > Clean up the tests for patch-id by moving file preparation > tasks inside the test body and redirecting files directly into > stdin instead of using 'cat'. > > Signed-off-by: Jerry Zhang <jerry@skydio.com> > --- > V2->V3: > - Quote the EOF marker Yes but no. > test_expect_success 'patch-id handles no-nl-at-eof markers' ' > - cat nonl | calc_patch_id nonl && > - cat withnl | calc_patch_id withnl && > + cat >nonl <<-'EOF' && We started the "executable" part of the test_expect_success as a single-quoted string, and then after writing <<-, we stepped out of that single-quote pair. Then we are writing E O F unquoted, and stepped back into another single-quote pair here. So, to the shell that runs this executable part, it is exactly the same as cat >nonl <<-EOF && side note: if it were not in a plain shell script (not the executable part that is passed as a single string to the test_expect_success function as an argument), what we see above, quoting EOF within a pair of single-quotes, is perfectly acceptable thing to do. But not here, for the reasons explained above. > + diff --git i/a w/a > + index e69de29..2e65efe 100644 > + --- i/a > + +++ w/a > + @@ -0,0 +1 @@ > + +a > + \ No newline at end of file > + diff --git i/b w/b > + index e69de29..6178079 100644 > + --- i/b > + +++ w/b > + @@ -0,0 +1 @@ > + +b > + 'EOF' Same here. It is exactly the same as writing EOF without any quotes around it, just like the opening one we saw earlier. In other words, the above is not quoting at all. I think I demonstrated the way we should write this in my earlier review when I pointed out this exiting issue this step is fixing (https://lore.kernel.org/git/xmqqmtjbh5fu.fsf@gitster.g/): test_expect_success "title string" ' ... command <<-\EOF && here document indented by tab more document EOF ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V4 1/2] patch-id: Fix antipatterns in tests 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang 2022-02-01 22:07 ` Johannes Sixt 2022-02-01 23:16 ` Junio C Hamano @ 2022-02-02 4:20 ` Jerry Zhang 2 siblings, 0 replies; 11+ messages in thread From: Jerry Zhang @ 2022-02-02 4:20 UTC (permalink / raw) To: git, gitster; +Cc: Jerry Zhang Clean up the tests for patch-id by moving file preparation tasks inside the test body and redirecting files directly into stdin instead of using 'cat'. Signed-off-by: Jerry Zhang <jerry@skydio.com> --- V3->V4: - Quote the EOF marker correctly t/t4204-patch-id.sh | 64 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index 80f4a65b28..2bc940a07e 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -164,42 +164,40 @@ test_expect_success 'patch-id respects config from subdir' ' cd subdir && test_patch_id irrelevant patchid.stable=true ) ' -cat >nonl <<\EOF -diff --git i/a w/a -index e69de29..2e65efe 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -\ No newline at end of file -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - -cat >withnl <<\EOF -diff --git i/a w/a -index e69de29..7898192 100644 ---- i/a -+++ w/a -@@ -0,0 +1 @@ -+a -diff --git i/b w/b -index e69de29..6178079 100644 ---- i/b -+++ w/b -@@ -0,0 +1 @@ -+b -EOF - test_expect_success 'patch-id handles no-nl-at-eof markers' ' - cat nonl | calc_patch_id nonl && - cat withnl | calc_patch_id withnl && + cat >nonl <<-\EOF && + diff --git i/a w/a + index e69de29..2e65efe 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + \ No newline at end of file + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + cat >withnl <<-\EOF && + diff --git i/a w/a + index e69de29..7898192 100644 + --- i/a + +++ w/a + @@ -0,0 +1 @@ + +a + diff --git i/b w/b + index e69de29..6178079 100644 + --- i/b + +++ w/b + @@ -0,0 +1 @@ + +b + EOF + calc_patch_id nonl <nonl && + calc_patch_id withnl <withnl && test_cmp patch-id_nonl patch-id_withnl ' test_done -- 2.32.0.1314.g6ed4fcc4cc ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-02-02 4:20 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-31 23:23 [PATCH 1/2] format-patch: Fix antipatterns in tests Jerry Zhang 2022-01-31 23:23 ` [PATCH V3 2/2] patch-id: fix scan_hunk_header on diffs with 1 line of before/after Jerry Zhang 2022-01-31 23:52 ` [PATCH V4 " Jerry Zhang 2022-02-02 4:19 ` [PATCH V5 " Jerry Zhang 2022-01-31 23:25 ` [PATCH V2 1/2] patch-id: Fix antipatterns in tests Jerry Zhang 2022-01-31 23:36 ` Junio C Hamano 2022-01-31 23:52 ` [PATCH V3 " Jerry Zhang 2022-02-01 22:07 ` Johannes Sixt 2022-02-01 23:18 ` Junio C Hamano 2022-02-01 23:16 ` Junio C Hamano 2022-02-02 4:20 ` [PATCH V4 " Jerry Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.