Re: git apply: git diff header lacks filename information for git diff --no-index patch

From: Jeff King <peff@peff.net>
Date: Oct 5, 2008, 19:17 UTC
Message-ID: <20081005191728.GA6173@coredump.intra.peff.net>
In-Reply-To: <alpine.LFD.2.00.0810040918290.3208@nehalem.linux-foundation.org>

On Sat, Oct 04, 2008 at 09:54:36AM -0700, Linus Torvalds wrote:

Show 7 quoted lines

> Exactly. In order to avoid all the ambiguities, we want the filename to 
> match on the 'diff -' line to even be able to guess, and if it doesn't, we 
> should pick it up from the "rename from" lines (for a git diff), or from 
> the '--- a/filename'/'+++ b/filename' otherwise (if it's not a rename, or 
> not a git diff).
> 
> And being a binary diff, and a creation event, all of this fails.

I wonder if it might have been better for binary diffs, like text diffs, to contain the "from" and "to" filenames in a similar format. But at this point I don't think a format change is really worthwhile.

> To make things worse, git has also screwed up the "/dev/null" and 
> prepended the prefix to it, making it even harder to see any patterns to 
> the names. Gaah.

Yes, I noticed that, as well. And obviously it looks bogus, but I thought I managed to get "git diff" to produce "a/dev/null" on some otherwise valid input, and so assumed that was something we were able to work around in applying the patch. But testing again today, I can't seem to get anything except this broken diff to say "a/dev/null". So probably I was just mistaken yesterday.

> So I think "git apply" is correct, and "git diff --no-index" is broken.

OK, your reasoning is sound.

> That said, I think git-apply being "correct" is not a great excuse, and we 
> should do the "be liberal in what you accept, conservative in what you 
> generate", and think about how to make git-apply be more willing to handle 
> this case too.

I think for now we might as well just fix the broken "diff" output. The only thing generating these broken diffs will be older versions of git, and such diffs are presumably rare (given that this is the first report we've seen).

So the only advantage would be to accept rare patches from people with older git, versus asking them to upgrade to a non-broken git.

Show 11 quoted lines

> Quite frankly, I should have doen the explicit headers as
> 
> 	"new file " <mode> SP <name>
> 
> instead of
> 
> 	"new file mode " <mode>
> 
> (and same for "deleted file", obviously) and the patch would have been 
> more readable _and_ we'd have avoided this issue. But when I did all that, 
> I didn't even think of binary diffs (they weren't an issue originally).

Agreed (and I think this is just another form of what I mentioned above; in my suggestion, we would include the filename on _all_ binary diffs. In practice, it wouldn't matter in non-creation cases, since we actually get the "diff --git" line _right_ in those cases :) ).

But again, I don't think it is worth trying to change the format now.

-Peff