Well, the first filename part could end with a space and be under a subpath starting with b, such as a/some /bla.txt in this case a patch is not parsed correctly with this patch.

Yeah, correct, that's the actual issue. The issue becomes even more obvious when considering that it is possible to create custom prefixes. So instead of "a/" and "b/", the user can have "foobar/" and "whateverhelikes/" as prefixes, making it totally impossible for us to parse in all cases. One solution pointed out by @ethomson was to instead parse the "--- a/file\n+++ b/file" lines, but unfortunately they are not mandated by the patch format, either.

This PR now depends on #4308.

I've changed the code to follow @ethomson's proposal of just bailing on the "diff --git" line if it seems to be unparseable and instead just use the "---" and "+++" lines. What I'm still undecided about is whether we want to fill in patch->header_old_part and patch->header_new_part, which is derived from the "diff --git" line, with the contents of patch->old_part and patch->new_part, derived from the "---" and "+++" lines. Opinions?

Opinions?

Yeah, I don't love my proposal of moving to the --- and +++ lines. It was sort of a half baked idea in the first place... I wonder if we should just get aggressive about validating the diff --git line and - if it's bogus - not fall back at all...?

Would your opinion about this behavior be different if I sent a patch to git to always quote filenames with spaces in them, ending this ambiguity once and for all?

(I should do that either way, of course.)

I definitly think this should be fixed upstream, it's simply awful right now. But that won't help users who are stuck with older versions of Git in the next 10+ years (if you count things like RHEL), so in my opinion this fix here is a nice thing to have. Bailing out whenever we cannot parse diff --git simply feels wrong when we can in fact just extract the correct information from the +++ and --- lines.

Sure. Out of curiosity, how does Git itself choose the filenames? Does it use the +++ and --- lines? I was thinking that it did not, but I've actually forgotten by now, tbqh. If it does, then it's a no brainer to emulate them. If they do not, then it would be nice to know what they are doing and how they're working around this issue.

Git uses the diff --git line as defname, which is a fallback in case oldname or newname could not be derived from the +++ and --- lines. Here's the comment from git_header_name, which parses the diff --git thing:

/*
 * This is to extract the same name that appears on "diff --git"
 * line.  We do not find and return anything if it is a rename
 * patch, and it is OK because we will find the name elsewhere.
 * We need to reliably find name only when it is mode-change only,
 * creation or deletion of an empty file.  In any of these cases,
 * both sides are the same name under a/ and b/ respectively.
 */
static char *git_header_name(struct apply_state *state,
			     const char *line,
			     int llen)

Inside of that function:

		/* strip the a/b prefix including trailing slash */
		cp = skip_tree_prefix(state, first.buf, first.len);
		if (!cp)
			goto free_and_fail1;

So if they're not able to strip the prefixes, they simply bail out and fail. Same for other stuff if they're not able to parse the names. They only abort parsing the patch later on if neither oldname, newname nor defname are set.

So this PR here goes into the same direction, I'd argue.

Thanks for fixing this! 😀