{"thread":{"id":"65280","subject":"[PATCH] apply: fix new-style empty context line triggering incomplete-line check","startedAt":"2026-03-17T18:01:40Z","lastAt":"2026-03-31T21:54:21Z","messageCount":7,"participants":["Junio C Hamano","Eric Sunshine","D. Ben Knoble"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539245","messageId":"xmqqldfql4hp.fsf@gitster.g","threadId":"65280","inReplyTo":null,"subject":"[PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-17T18:01:38Z","receivedAt":"2026-03-17T18:01:40Z","isPatch":true,"body":"A new-style unified context diff represents an empty context line\nwith an empty line (instead of a line with a single SP on it).  The\ncode to check whitespace errors in an incoming patch is designed to\nomit the first byte of a line (typically SP, \"-\", or \"+\") and pass the\nremainder of the line to the whitespace checker.\n\nUsually we do not pass a context line to the whitespace error checker,\nbut when we are correcting errors, we do.  This \"remove the first\nbyte and send the remainder\" strategy of checking a line ended up\nsending a zero-length string to the whitespace checker when seeing a\nnew-style empty context line, which caused the whitespace checker to\nsay \"ah, you do not even have a newline at the end!\", leading to an\n\"incomplete line\" in the middle of the patch!\n\nFix this by pretending that we got a traditional empty context line\nwhen we drive the whitespace checker.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n apply.c                  | 12 ++++++++++--\n t/t4124-apply-ws-rule.sh | 16 ++++++++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex f01204d15b..e88e5c77e3 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1796,8 +1796,16 @@ static int parse_fragment(struct apply_state *state,\n \t\t\ttrailing++;\n \t\t\tcheck_old_for_crlf(patch, line, len);\n \t\t\tif (!state->apply_in_reverse &&\n-\t\t\t    state->ws_error_action == correct_ws_error)\n-\t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n+\t\t\t    state->ws_error_action == correct_ws_error) {\n+\t\t\t\tconst char *test_line = line;\n+\t\t\t\tint test_len = len;\n+\t\t\t\tif (*line == '\\n') {\n+\t\t\t\t\ttest_line = \" \\n\";\n+\t\t\t\t\ttest_len = 2;\n+\t\t\t\t}\n+\t\t\t\tcheck_whitespace(state, test_line, test_len,\n+\t\t\t\t\t\t patch->ws_rule);\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase '-':\n \t\t\tif (!state->apply_in_reverse)\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 29ea7d4268..8573e12f46 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' '\n \tgit config core.whitespace incomplete-line\n '\n \n+test_expect_success 'no incomplete context line (not an error)' '\n+\ttest_when_finished \"rm -f sample*-i patch patch-new target\" &&\n+\t(test_write_lines 1 2 3 \"\" 4 5 ) >sample-i &&\n+\t(test_write_lines 1 2 3 \"\" 0 5 ) >sample2-i &&\n+\tcat sample-i >target &&\n+\tgit add target &&\n+\tcat sample2-i >target &&\n+\tgit diff-files -p target >patch &&\n+\tsed -e \"s/^ $//\" <patch >patch-new &&\n+\n+\tcat sample-i >target &&\n+\tgit apply --whitespace=fix <patch-new 2>error &&\n+\ttest_cmp sample2-i target &&\n+\ttest_must_be_empty error\n+'\n+\n test_expect_success 'incomplete context line (not an error)' '\n \t(test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n \t(test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n-- \n2.53.0-769-g8689fa97fd\n\n"},{"id":"539247","messageId":"CAPig+cTTgLVGPG99gsb19BeJVWS=VZCU4F-rjb25yHTAORWwzg@mail.gmail.com","threadId":"65280","inReplyTo":"xmqqldfql4hp.fsf@gitster.g","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-17T18:12:12Z","receivedAt":"2026-03-17T18:12:25Z","isPatch":true,"body":"On Tue, Mar 17, 2026 at 2:01 PM Junio C Hamano <gitster@pobox.com> wrote:\n> A new-style unified context diff represents an empty context line\n> with an empty line (instead of a line with a single SP on it).  The\n> code to check whitespace errors in an incoming patch is designed to\n> omit the first byte of a line (typically SP, \"-\", or \"+\") and pass the\n> remainder of the line to the whitespace checker.\n>\n> Usually we do not pass a context line to the whitespace error checker,\n> but when we are correcting errors, we do.  This \"remove the first\n> byte and send the remainder\" strategy of checking a line ended up\n> sending a zero-length string to the whitespace checker when seeing a\n> new-style empty context line, which caused the whitespace checker to\n> say \"ah, you do not even have a newline at the end!\", leading to an\n> \"incomplete line\" in the middle of the patch!\n>\n> Fix this by pretending that we got a traditional empty context line\n> when we drive the whitespace checker.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 29ea7d4268..8573e12f46 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' '\n> +test_expect_success 'no incomplete context line (not an error)' '\n> +       test_when_finished \"rm -f sample*-i patch patch-new target\" &&\n> +       (test_write_lines 1 2 3 \"\" 4 5 ) >sample-i &&\n> +       (test_write_lines 1 2 3 \"\" 0 5 ) >sample2-i &&\n\nCurious. Why are the `test_write_line` invocations wrapped in parentheses?\n\nAlso, is the whitespace before the closing parenthesis intentional?\n\n>  test_expect_success 'incomplete context line (not an error)' '\n>         (test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n>         (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n\nPerhaps the parentheses in the new test were copied from some existing\ntest, such as this, which already used them for a legitimate reason?\n"},{"id":"539249","messageId":"xmqqcy12l2ft.fsf@gitster.g","threadId":"65280","inReplyTo":"CAPig+cTTgLVGPG99gsb19BeJVWS=VZCU4F-rjb25yHTAORWwzg@mail.gmail.com","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-17T18:45:58Z","receivedAt":"2026-03-17T18:46:01Z","isPatch":true,"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> +       test_when_finished \"rm -f sample*-i patch patch-new target\" &&\n>> +       (test_write_lines 1 2 3 \"\" 4 5 ) >sample-i &&\n>> +       (test_write_lines 1 2 3 \"\" 0 5 ) >sample2-i &&\n>\n> Curious. Why are the `test_write_line` invocations wrapped in parentheses?\n>\n> Also, is the whitespace before the closing parenthesis intentional?\n>\n>>  test_expect_success 'incomplete context line (not an error)' '\n>>         (test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n>>         (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n>\n> Perhaps the parentheses in the new test were copied from some existing\n> test, such as this, which already used them for a legitimate reason?\n\nYes, the existing one was concatenating output from two commands run\nin a row into a single redirection, so (grouping of the commands) in\nparentheses were justifiable.\n\nThe new one does not have such a justification.  Thanks for\nnoticing.\n"},{"id":"539297","messageId":"CALnO6CDNwa8Ez4Ug0f8zNyxF1n3C_j8mLRbH7wChVioNoC5QVw@mail.gmail.com","threadId":"65280","inReplyTo":"xmqqcy12l2ft.fsf@gitster.g","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-03-18T16:36:04Z","receivedAt":"2026-03-18T16:36:17Z","isPatch":true,"body":"On Tue, Mar 17, 2026 at 2:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n> >> +       test_when_finished \"rm -f sample*-i patch patch-new target\" &&\n> >> +       (test_write_lines 1 2 3 \"\" 4 5 ) >sample-i &&\n> >> +       (test_write_lines 1 2 3 \"\" 0 5 ) >sample2-i &&\n> >\n> > Curious. Why are the `test_write_line` invocations wrapped in parentheses?\n> >\n> > Also, is the whitespace before the closing parenthesis intentional?\n> >\n> >>  test_expect_success 'incomplete context line (not an error)' '\n> >>         (test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n> >>         (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n> >\n> > Perhaps the parentheses in the new test were copied from some existing\n> > test, such as this, which already used them for a legitimate reason?\n>\n> Yes, the existing one was concatenating output from two commands run\n> in a row into a single redirection, so (grouping of the commands) in\n> parentheses were justifiable.\n>\n> The new one does not have such a justification.  Thanks for\n> noticing.\n\nI think braces { test_writes lines … && printf … ; } would have\nsufficed for the second example, and might be cheaper (avoiding the\nextra process for the subshell, which we've been told is especially\nexpensive for Windows).\n\n-- \nD. Ben Knoble\n"},{"id":"539300","messageId":"CAPig+cTx3Gho+uYd9+0SjE+x9GA6VMNu78riUZ=h5_QW2vUHNQ@mail.gmail.com","threadId":"65280","inReplyTo":"CALnO6CDNwa8Ez4Ug0f8zNyxF1n3C_j8mLRbH7wChVioNoC5QVw@mail.gmail.com","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-18T16:43:07Z","receivedAt":"2026-03-18T16:43:20Z","isPatch":true,"body":"On Wed, Mar 18, 2026 at 12:36 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n> On Tue, Mar 17, 2026 at 2:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> > >>  test_expect_success 'incomplete context line (not an error)' '\n> > >>         (test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n> > >>         (test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n> > >\n> > > Perhaps the parentheses in the new test were copied from some existing\n> > > test, such as this, which already used them for a legitimate reason?\n> >\n> > Yes, the existing one was concatenating output from two commands run\n> > in a row into a single redirection, so (grouping of the commands) in\n> > parentheses were justifiable.\n> >\n> > The new one does not have such a justification.  Thanks for\n> > noticing.\n>\n> I think braces { test_writes lines … && printf … ; } would have\n> sufficed for the second example, and might be cheaper (avoiding the\n> extra process for the subshell, which we've been told is especially\n> expensive for Windows).\n\nNo doubt. I considered making the exact same comment, however, this is\nexisting code which Junio's patch was not touching, so the comment\nwould not have been directly applicable except possibly as a\n#leftoverbits for someone to tackle as a mini-project or some such, so\nI opted against saying anything about it.\n"},{"id":"539301","messageId":"xmqq341xgiyw.fsf@gitster.g","threadId":"65280","inReplyTo":"CALnO6CDNwa8Ez4Ug0f8zNyxF1n3C_j8mLRbH7wChVioNoC5QVw@mail.gmail.com","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-18T17:12:23Z","receivedAt":"2026-03-18T17:12:26Z","isPatch":true,"body":"\"D. Ben Knoble\" <ben.knoble@gmail.com> writes:\n\n> I think braces { test_writes lines … && printf … ; } would have\n> sufficed for the second example\n\nNobody complained about them ever since they were written.  More\nimportantly, we have plenty of them that nobody complained and\nbothered to uglify so far [*].\n\n;-)\n\n$ git grep -e '(.*) >' master -- t/t[0-9]\\*.sh | grep -v -e '\\$(' -e 'cd ' | wc -l\n60\n$ git grep -e '(.*) >' master -- t/t[0-9]\\*.sh | grep -v -e '\\$(' -e 'cd ' |\n  head -n 20\nmaster:t/t3050-subprojects-fetch.sh:\t(git rev-parse HEAD && git ls-files -s) >expected &&\nmaster:t/t3050-subprojects-fetch.sh:\t\t(git rev-parse HEAD && git ls-files -s) >../actual\nmaster:t/t3050-subprojects-fetch.sh:\t(git rev-parse HEAD && git ls-files -s) >expected &&\nmaster:t/t3050-subprojects-fetch.sh:\t\t(git rev-parse HEAD && git ls-files -s) >../actual\nmaster:t/t3402-rebase-merge.sh:\t(echo \"0 $T\" && cat original) >renamed &&\nmaster:t/t3900-i18n-commit.sh:\t\t(sed \"1,/^$/d\" raw | iconv -f $new -t utf-8) >actual &&\nmaster:t/t4001-diff-rename.sh:\t(cat path1 && echo new) >new-path &&\nmaster:t/t4015-diff-whitespace.sh:\t(echo foo && echo baz | tr -d \"\\012\") >x &&\nmaster:t/t4015-diff-whitespace.sh:\t(echo bar && echo baz | tr -d \"\\012\") >x &&\nmaster:t/t4019-diff-wserror.sh:if (grep \"$blue_grep\" <check-grep | grep \"$blue_grep\") >/dev/null 2>&1\nmaster:t/t4019-diff-wserror.sh:elif (grep -a \"$blue_grep\" <check-grep | grep -a \"$blue_grep\") >/dev/null 2>&1\nmaster:t/t4024-diff-optimize-common.sh:\t\t( zs $n && echo a ) >file-a$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( echo b && zs $n && echo ) >file-b$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( printf c && zs $n ) >file-c$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( echo d && zs $n ) >file-d$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( zs $n && echo A ) >file-a$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( echo B && zs $n && echo ) >file-b$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( printf C && zs $n ) >file-c$n &&\nmaster:t/t4024-diff-optimize-common.sh:\t\t( echo D && zs $n ) >file-d$n &&\nmaster:t/t4101-apply-nonl.sh:(echo a; echo b) >frotz.0\n\n\n\n[Footnote]\n\n * I personally find that we need ';' immediately before '}'\n   intolerably ugly.\n"},{"id":"540568","messageId":"xmqq7bqry8ac.fsf@gitster.g","threadId":"65280","inReplyTo":"xmqqldfql4hp.fsf@gitster.g","subject":"Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-31T21:54:19Z","receivedAt":"2026-03-31T21:54:21Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A new-style unified context diff represents an empty context line\n> with an empty line (instead of a line with a single SP on it).  The\n> code to check whitespace errors in an incoming patch is designed to\n> omit the first byte of a line (typically SP, \"-\", or \"+\") and pass the\n> remainder of the line to the whitespace checker.\n>\n> Usually we do not pass a context line to the whitespace error checker,\n> but when we are correcting errors, we do.  This \"remove the first\n> byte and send the remainder\" strategy of checking a line ended up\n> sending a zero-length string to the whitespace checker when seeing a\n> new-style empty context line, which caused the whitespace checker to\n> say \"ah, you do not even have a newline at the end!\", leading to an\n> \"incomplete line\" in the middle of the patch!\n>\n> Fix this by pretending that we got a traditional empty context line\n> when we drive the whitespace checker.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  apply.c                  | 12 ++++++++++--\n>  t/t4124-apply-ws-rule.sh | 16 ++++++++++++++++\n>  2 files changed, 26 insertions(+), 2 deletions(-)\n\nThere were only comments on the unnecessary uses of subshell in\ntest, which were fixed since then, and I've been using them in\nproduction without problems, so let me mark this for 'next' now.\n\nObjections and better yet polishing on top are of course welcome.\n\n> diff --git a/apply.c b/apply.c\n> index f01204d15b..e88e5c77e3 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1796,8 +1796,16 @@ static int parse_fragment(struct apply_state *state,\n>  \t\t\ttrailing++;\n>  \t\t\tcheck_old_for_crlf(patch, line, len);\n>  \t\t\tif (!state->apply_in_reverse &&\n> -\t\t\t    state->ws_error_action == correct_ws_error)\n> -\t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n> +\t\t\t    state->ws_error_action == correct_ws_error) {\n> +\t\t\t\tconst char *test_line = line;\n> +\t\t\t\tint test_len = len;\n> +\t\t\t\tif (*line == '\\n') {\n> +\t\t\t\t\ttest_line = \" \\n\";\n> +\t\t\t\t\ttest_len = 2;\n> +\t\t\t\t}\n> +\t\t\t\tcheck_whitespace(state, test_line, test_len,\n> +\t\t\t\t\t\t patch->ws_rule);\n> +\t\t\t}\n>  \t\t\tbreak;\n>  \t\tcase '-':\n>  \t\t\tif (!state->apply_in_reverse)\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 29ea7d4268..8573e12f46 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -561,6 +561,22 @@ test_expect_success 'check incomplete lines (setup)' '\n>  \tgit config core.whitespace incomplete-line\n>  '\n>  \n> +test_expect_success 'no incomplete context line (not an error)' '\n> +\ttest_when_finished \"rm -f sample*-i patch patch-new target\" &&\n> +\t(test_write_lines 1 2 3 \"\" 4 5 ) >sample-i &&\n> +\t(test_write_lines 1 2 3 \"\" 0 5 ) >sample2-i &&\n> +\tcat sample-i >target &&\n> +\tgit add target &&\n> +\tcat sample2-i >target &&\n> +\tgit diff-files -p target >patch &&\n> +\tsed -e \"s/^ $//\" <patch >patch-new &&\n> +\n> +\tcat sample-i >target &&\n> +\tgit apply --whitespace=fix <patch-new 2>error &&\n> +\ttest_cmp sample2-i target &&\n> +\ttest_must_be_empty error\n> +'\n> +\n>  test_expect_success 'incomplete context line (not an error)' '\n>  \t(test_write_lines 1 2 3 4 5 && printf 6) >sample-i &&\n>  \t(test_write_lines 1 2 3 0 5 && printf 6) >sample2-i &&\n"}]}