apply: fix new-style empty context line triggering incomplete-line check
PATCH 1/1 apply: fix new-style empty context line triggering incomplete-line check
A new-style unified context diff represents an empty context line with an empty line (instead of a line with a single SP on it). The code to check whitespace errors in an incoming patch is designed to omit the first byte of a line (typically SP, "-", or "+") and pass the remainder of the line to the whitespace checker.
Usually we do not pass a context line to the whitespace error checker, but when we are correcting errors, we do. This "remove the first byte and send the remainder" strategy of checking a line ended up sending a zero-length string to the whitespace checker when seeing a new-style empty context line, which caused the whitespace checker to say "ah, you do not even have a newline at the end!", leading to an "incomplete line" in the middle of the patch!
Fix this by pretending that we got a traditional empty context line when we drive the whitespace checker.
--- apply.c | 12 ++++++++++-- t/t4124-apply-ws-rule.sh | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)
apply.c +10 -2
@@ -1796,8 +1796,16 @@ static int parse_fragment(struct apply_state *state, trailing++;check_old_for_crlf (patch, line, len);if (!state->apply_in_reverse && - state->ws_error_action == correct_ws_error) -check_whitespace (state, line, len, patch->ws_rule); + state->ws_error_action == correct_ws_error) { +const char *test_line = line; +int test_len = len; +if (*line =='\n' ) { + test_line =" \n" ; + test_len =2 ; + } +check_whitespace (state, test_line, test_len, + patch->ws_rule); + }break ;case '-' :if (!state->apply_in_reverse)
t/t4124-apply-ws-rule.sh +16 -0
@@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' ' git config core.whitespace incomplete-line ' +test_expect_success 'no incomplete context line (not an error)' ' + test_when_finished "rm -f sample*-i patch patch-new target" && + (test_write_lines 1 2 3 "" 4 5 ) >sample-i && + (test_write_lines 1 2 3 "" 0 5 ) >sample2-i && + cat sample-i >target && + git add target && + cat sample2-i >target && + git diff-files -p target >patch && + sed -e "s/^ $//" <patch >patch-new && + + cat sample-i >target && + git apply --whitespace=fix <patch-new 2>error && + test_cmp sample2-i target && + test_must_be_empty error +' + test_expect_success 'incomplete context line (not an error)' ' (test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&
Signature · 1 line
2.53.0-769-g8689fa97fd
Quoted 24 lines · Junio C Hamano
A new-style unified context diff represents an empty context line with an empty line (instead of a line with a single SP on it). The code to check whitespace errors in an incoming patch is designed to omit the first byte of a line (typically SP, "-", or "+") and pass the remainder of the line to the whitespace checker.
Usually we do not pass a context line to the whitespace error checker, but when we are correcting errors, we do. This "remove the first byte and send the remainder" strategy of checking a line ended up sending a zero-length string to the whitespace checker when seeing a new-style empty context line, which caused the whitespace checker to say "ah, you do not even have a newline at the end!", leading to an "incomplete line" in the middle of the patch!
Fix this by pretending that we got a traditional empty context line when we drive the whitespace checker.
Signed-off-by Junio C Hamano <[email protected]>---
t/t4124-apply-ws-rule.sh +0
-0
t/t4124-apply-ws-rule.sh test_expect_success 'check incomplete lines (setup)' ' · line 564
Quoted 4 lines
t/t4124-apply-ws-rule.sh +4
-0@@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' ' +test_expect_success 'no incomplete context line (not an error)' ' + test_when_finished "rm -f sample*-i patch patch-new target" && + (test_write_lines 1 2 3 "" 4 5 ) >sample-i && + (test_write_lines 1 2 3 "" 0 5 ) >sample2-i &&
Curious. Why are the test_write_line invocations wrapped in parentheses?
Also, is the whitespace before the closing parenthesis intentional?
t/t4124-apply-ws-rule.sh test_expect_success 'check incomplete lines (setup)' ' · line 582
Quoted 3 lines
t/t4124-apply-ws-rule.sh +0
-0test_expect_success 'incomplete context line (not an error)' ' (test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&
Perhaps the parentheses in the new test were copied from some existing test, such as this, which already used them for a legitimate reason?
Quoted 6 lines · Eric Sunshine
t/t4124-apply-ws-rule.sh test_expect_success 'check incomplete lines (setup)' ' · line 567
Quoted 3 lines
t/t4124-apply-ws-rule.sh +3
-0+ test_when_finished "rm -f sample*-i patch patch-new target" && + (test_write_lines 1 2 3 "" 4 5 ) >sample-i && + (test_write_lines 1 2 3 "" 0 5 ) >sample2-i &&Curious. Why are the
test_write_lineinvocations wrapped in parentheses?
Quoted 8 lines
Also, is the whitespace before the closing parenthesis intentional?
Quoted 3 lines
test_expect_success'incomplete context line (not an error)' '(test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&Perhaps the parentheses in the new test were copied from some existing test, such as this, which already used them for a legitimate reason?
Yes, the existing one was concatenating output from two commands run in a row into a single redirection, so (grouping of the commands) in parentheses were justifiable.
The new one does not have such a justification. Thanks for noticing.
Quoted 18 lines · Junio C Hamano
Quoted 6 lines · Eric Sunshine
t/t4124-apply-ws-rule.sh test_expect_success 'check incomplete lines (setup)' ' · line 567
Quoted 3 lines
t/t4124-apply-ws-rule.sh +3
-0+ test_when_finished "rm -f sample*-i patch patch-new target" && + (test_write_lines 1 2 3 "" 4 5 ) >sample-i && + (test_write_lines 1 2 3 "" 0 5 ) >sample2-i &&Curious. Why are the
test_write_lineinvocations wrapped in parentheses?Quoted 8 lines
Also, is the whitespace before the closing parenthesis intentional?
Quoted 3 lines
test_expect_success'incomplete context line (not an error)' '(test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&Perhaps the parentheses in the new test were copied from some existing test, such as this, which already used them for a legitimate reason?
Quoted 6 lines
Yes, the existing one was concatenating output from two commands run in a row into a single redirection, so (grouping of the commands) in parentheses were justifiable.
The new one does not have such a justification. Thanks for noticing.
I think braces { test_writes lines … && printf … ; } would have sufficed for the second example, and might be cheaper (avoiding the extra process for the subshell, which we've been told is especially expensive for Windows).
Signature · 1 line
D. Ben Knoble
Quoted 13 lines · D. Ben Knoble
Quoted 12 lines · Junio C Hamano
Quoted 6 lines · Eric Sunshine
Quoted 3 lines
test_expect_success'incomplete context line (not an error)' '(test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&Perhaps the parentheses in the new test were copied from some existing test, such as this, which already used them for a legitimate reason?
Yes, the existing one was concatenating output from two commands run in a row into a single redirection, so (grouping of the commands) in parentheses were justifiable.
Quoted 7 lines
Quoted 2 lines
The new one does not have such a justification. Thanks for noticing.
I think braces {
test_writeslines … && printf … ; } would have sufficed for the second example, and might be cheaper (avoiding the extra process for the subshell, which we've been told is especially expensive for Windows).
No doubt. I considered making the exact same comment, however, this is existing code which Junio's patch was not touching, so the comment would not have been directly applicable except possibly as a #leftoverbits for someone to tackle as a mini-project or some such, so I opted against saying anything about it.
Quoted 2 lines · D. Ben Knoble
I think braces {
test_writeslines … && printf … ; } would have sufficed for the second example
Nobody complained about them ever since they were written. More importantly, we have plenty of them that nobody complained and bothered to uglify so far [*].
;-)
$ git grep -e '(.*) >' master -- t/t[0-9]\*.sh | grep -v -e '\$(' -e 'cd ' | wc -l
60
$ git grep -e '(.*) >' master -- t/t[0-9]\*.sh | grep -v -e '\$(' -e 'cd ' |
head -n 20
master:t/t3050-subprojects-fetch.sh: (git rev-parse HEAD && git ls-files -s) >expected &&
master:t/t3050-subprojects-fetch.sh: (git rev-parse HEAD && git ls-files -s) >../actual
master:t/t3050-subprojects-fetch.sh: (git rev-parse HEAD && git ls-files -s) >expected &&
master:t/t3050-subprojects-fetch.sh: (git rev-parse HEAD && git ls-files -s) >../actual
master:t/t3402-rebase-merge.sh: (echo "0 $T" && cat original) >renamed &&
master:t/t3900-i18n-commit.sh: (sed "1,/^$/d" raw | iconv -f $new -t utf-8) >actual &&
master:t/t4001-diff-rename.sh: (cat path1 && echo new) >new-path &&
master:t/t4015-diff-whitespace.sh: (echo foo && echo baz | tr -d "\012") >x &&
master:t/t4015-diff-whitespace.sh: (echo bar && echo baz | tr -d "\012") >x &&
master:t/t4019-diff-wserror.sh:if (grep "$blue_grep" <check-grep | grep "$blue_grep") >/dev/null 2>&1
master:t/t4019-diff-wserror.sh:elif (grep -a "$blue_grep" <check-grep | grep -a "$blue_grep") >/dev/null 2>&1
master:t/t4024-diff-optimize-common.sh: ( zs $n && echo a ) >file-a$n &&
master:t/t4024-diff-optimize-common.sh: ( echo b && zs $n && echo ) >file-b$n &&
master:t/t4024-diff-optimize-common.sh: ( printf c && zs $n ) >file-c$n &&
master:t/t4024-diff-optimize-common.sh: ( echo d && zs $n ) >file-d$n &&
master:t/t4024-diff-optimize-common.sh: ( zs $n && echo A ) >file-a$n &&
master:t/t4024-diff-optimize-common.sh: ( echo B && zs $n && echo ) >file-b$n &&
master:t/t4024-diff-optimize-common.sh: ( printf C && zs $n ) >file-c$n &&
master:t/t4024-diff-optimize-common.sh: ( echo D && zs $n ) >file-d$n &&
master:t/t4101-apply-nonl.sh:(echo a; echo b) >frotz.0[Footnote]
* I personally find that we need ';' immediately before '}' intolerably ugly.
Quoted 14 lines · Junio C Hamano
A new-style unified context diff represents an empty context line with an empty line (instead of a line with a single SP on it). The code to check whitespace errors in an incoming patch is designed to omit the first byte of a line (typically SP, "-", or "+") and pass the remainder of the line to the whitespace checker.
Usually we do not pass a context line to the whitespace error checker, but when we are correcting errors, we do. This "remove the first byte and send the remainder" strategy of checking a line ended up sending a zero-length string to the whitespace checker when seeing a new-style empty context line, which caused the whitespace checker to say "ah, you do not even have a newline at the end!", leading to an "incomplete line" in the middle of the patch!
Quoted 8 lines
Fix this by pretending that we got a traditional empty context line when we drive the whitespace checker.
Signed-off-by Junio C Hamano <[email protected]>--- apply.c | 12 ++++++++++-- t/t4124-apply-ws-rule.sh | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)
There were only comments on the unnecessary uses of subshell in test, which were fixed since then, and I've been using them in production without problems, so let me mark this for 'next' now.
Objections and better yet polishing on top are of course welcome.
Quoted 50 lines
apply.c +10
-2@@ -1796,8 +1796,16 @@ static int parse_fragment(struct apply_state *state, trailing++;check_old_for_crlf (patch, line, len);if (!state->apply_in_reverse && - state->ws_error_action == correct_ws_error) -check_whitespace (state, line, len, patch->ws_rule); + state->ws_error_action == correct_ws_error) { +const char *test_line = line; +int test_len = len; +if (*line =='\n' ) { + test_line =" \n" ; + test_len =2 ; + } +check_whitespace (state, test_line, test_len, + patch->ws_rule); + }break ;case '-' :if (!state->apply_in_reverse)t/t4124-apply-ws-rule.sh +16
-0@@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' ' git config core.whitespace incomplete-line ' +test_expect_success 'no incomplete context line (not an error)' ' + test_when_finished "rm -f sample*-i patch patch-new target" && + (test_write_lines 1 2 3 "" 4 5 ) >sample-i && + (test_write_lines 1 2 3 "" 0 5 ) >sample2-i && + cat sample-i >target && + git add target && + cat sample2-i >target && + git diff-files -p target >patch && + sed -e "s/^ $//" <patch >patch-new && + + cat sample-i >target && + git apply --whitespace=fix <patch-new 2>error && + test_cmp sample2-i target && + test_must_be_empty error +' + test_expect_success 'incomplete context line (not an error)' ' (test_write_lines 1 2 3 4 5 && printf 6) >sample-i && (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&