Re: [PATCH v3] apply.c: fix -p argument parsing

From: Jeff King <peff@peff.net>
Date: Mar 13, 2026, 01:12 UTC
Message-ID: <20260313011259.GA3204960@coredump.intra.peff.net>
In-Reply-To: <20260313001629.GA3193660@coredump.intra.peff.net>

On Thu, Mar 12, 2026 at 08:16:29PM -0400, Jeff King wrote:

Show 14 quoted lines

> > +test_expect_success 'git apply -p 1 patch' '
> > +	test_when_finished "rm -rf t" &&
> > +	git apply -p 1 $TEST_DIRECTORY/t4120/patch &&
> > +	test_path_is_dir t
> > +'
> 
> This test seems to fail on Windows. From CI:
> 
>     ++ git apply -p 1 /d/a/git/git/t/t4120/patch
>     error: git diff header lacks filename information when removing 1 leading pathname component (line 14)
>     error: last command exited with $?=128
> 
> but I can't figure out why (and don't have a local Windows machine to
> test on easily).

Ah, I figured it out. The culprit is CRLF line endings. Naturally. :-/

It looks like there is an existing bug in apply.c when reading patches with CRLF endings. Because of complicated historical reasons, parsing the:

  diff --git a/t/test/test b/t/test/test

line insists that we find the same "t/test/test" path at the very end of the line. But instead, we find the extra CR. As a result, we leave patch->def_name NULL instead of filling it in with "t/test/test".

Usually this is not too big a deal, as we can pick up the name from the "---" and "+++" lines. But in your patch:

  diff --git a/t/test/test b/t/test/test
  new file mode 100644
  index 0000000000..e69de29bb2

since there is no content, we omit them entirely. And so Git has no idea where to apply the patch (even without "-p" at all).

I think the fix is probably:

diff --git a/apply.c b/apply.c
index 61df3bdcd0..62d6a1f8d7 100644
--- a/apply.c
+++ b/apply.c
@@ -1295,7 +1295,7 @@ static char *git_header_name(int p_value,
 			 * (that are separated by one HT or SP we just
 			 * found) exactly match?
 			 */
-			if (second[len] == '\n' && !strncmp(name, second, len))
+			if ((second[len] == '\n' || second[len] == '\r') && !strncmp(name, second, len))
 				return xmemdupz(name, len);
 		}
 	}

but I'm not sure if there are other lurking CRLF issues, or if this
might allow malicious input to cause confusion.


Getting back to your patch: why is there a CRLF here in the first place?
Because on Windows, we check out the whole repo with CRLF conversion,
except for a few known file types listed in .gitattributes. And that
includes your t/t4120/patch file.

Coincidentally the style suggestion I made earlier, to just inline it in
the t4120 script itself, makes the problem go away. Because we check out
those scripts with bare line feeds, per .gitattributes, the file we
create will also have regular line feeds.

So I would suggest doing that as a workaround. It might be worth
addressing the CRLF header parsing problem above, too, but I think that
should be a separate topic.

-Peff