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

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

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.

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