[PATCH] apply: fix new-style empty context line triggering incomplete-line check

[PATCH] apply: fix new-style empty context line triggering incomplete-line check

[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 <gitster@pobox.com>
---
 apply.c                  | 12 ++++++++++--
 t/t4124-apply-ws-rule.sh | 16 ++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index f01204d15b..e88e5c77e3 100644
--- a/apply.c
+++ b/apply.c
@@ -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)
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 29ea7d4268..8573e12f46 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -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 &&
-- 
2.53.0-769-g8689fa97fd

Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check

[Eric Sunshine]

On Tue, Mar 17, 2026 at 2:01 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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 <gitster@pobox.com>
> ---
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 29ea7d4268..8573e12f46 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -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?

>  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?

Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       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?
>
>>  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.

Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check

[D. Ben Knoble]

On Tue, Mar 17, 2026 at 2:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >> +       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?
> >
> >>  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.

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).

-- 
D. Ben Knoble

Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check

[Eric Sunshine]

On Wed, Mar 18, 2026 at 12:36 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> On Tue, Mar 17, 2026 at 2:48 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > >>  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.
>
> 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).

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.

Re: [PATCH] apply: fix new-style empty context line triggering incomplete-line check

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

> I think braces { test_writes lines … && 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.