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

Hi,

I have the following problem:

$ echo -e '\x0' > a
$ git diff --no-index --binary /dev/null a > patch
$ rm a
$ git apply patch
fatal: git diff header lacks filename information (line 4)
$ cat patch

diff --git a/dev/null b/a
new file mode 100644
index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1

GIT binary patch
literal 2
JcmZSJ0ssIE01E&B

literal 0
HcmV?d00001

The same works for text based patches:

$ echo 1 > a
$ git diff --no-index /dev/null a > patch
$ rm a
$ git apply patch
$ ls
a
$ cat patch

diff --git a/dev/null b/a
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/a

@@ -0,0 +1 @@
+1

The binary patch lacks ---/+++ lines but still provides the name info
on the diff --git line which I think should suffice for git apply.

--Imre

On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:

Hmm. The problem is that "git apply" doesn't accept that "a/dev/null"
and "b/a" are the same, so it rejects them as a name. I guess on a text
patch, we would just pull that information from the "---" and "+++"
lines, so we don't care that it's not on the diff commandline.

However, a _non_ --no-index patch doesn't produce the same output. It
will actually produce the line:

  diff --git a/a b/a

even if it is a creation patch. So I'm not sure which piece of code is
at fault. Either:

  1. git apply is right to reject, and "git diff --no-index" should be
     putting the actual filename on the commandline of a binary patch
     instead of /dev/null, even if it is a creation patch.

     or

  2. git apply should accept this construct. Perhaps we should relax the
     "both names must be the same" rule if one of the names is /dev/null
     (and we would take the other)?

Linus, the "both names must be the same" code in git_header_name blames
to you (5041aa70). Thoughts on number 2?

-Peff

[Cc: git@vger.kernel.org, Jeff King [off-list ref], 
 Imre Deak [off-list ref]]

Jeff King wrote:

On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:

quoted

$ git apply patch
fatal: git diff header lacks filename information (line 4)
$ cat patch

diff --git a/dev/null b/a
new file mode 100644
index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1

GIT binary patch

Hmm. The problem is that "git apply" doesn't accept that "a/dev/null"
and "b/a" are the same, so it rejects them as a name. I

Shouldn't it be "/dev/null", not "a/dev/null"?  

Besides git-diff(1) states:

  1. It is preceded with a "git diff" header, that looks like
     this:

       diff --git a/file1 b/file2

     The `a/` and `b/` filenames are the same unless rename/copy is
     involved.  Especially, even for a creation or a deletion,
     `/dev/null` is _not_ used in place of `a/` or `b/` filenames.

Looks like a bug in patch generation code...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

On Sat, 4 Oct 2008, Jeff King wrote:

On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:

quoted

$ git apply patch
fatal: git diff header lacks filename information (line 4)
$ cat patch

diff --git a/dev/null b/a
new file mode 100644
index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1

GIT binary patch

Ok, this patch is broken in so many ways..

Hmm. The problem is that "git apply" doesn't accept that "a/dev/null"
and "b/a" are the same, so it rejects them as a name. I guess on a text
patch, we would just pull that information from the "---" and "+++"
lines, so we don't care that it's not on the diff commandline.

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.

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.

However, a _non_ --no-index patch doesn't produce the same output. It
will actually produce the line:

  diff --git a/a b/a

even if it is a creation patch. So I'm not sure which piece of code is
at fault.

It's some of both. I think --no-index is broken, that "a/dev/null" is 
total crap regardless of anything else. There's no way that thing is 
correct, and even if you were not to want "a/a", it should have been just 
plain "/dev/null". 

But for a native git patch, we shouldn't even use the (at least slightly 
more correct) "/dev/null", since native git application doesn't actually 
do the "is_dev_null()" for native patches, and just wants the filename to 
be the same.

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

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. 

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

Dang.

Anyway, for now I'd suggest a patch like this. Hmm? The "diff --git" 
format assumes that the names are the same, so make it so.

			Linus

---
 diff.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 9053d4d..0910b4e 100644
--- a/diff.c
+++ b/diff.c

@@ -1479,7 +1479,7 @@ static void builtin_diff(const char *name_a,
 {
 	mmfile_t mf1, mf2;
 	const char *lbl[2];
-	char *a_one, *b_two;
+	char *a_one, *b_two, *h_name;
 	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
 	const char *reset = diff_get_color_opt(o, DIFF_RESET);
 	const char *a_prefix, *b_prefix;

@@ -1497,7 +1497,8 @@ static void builtin_diff(const char *name_a,
 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-	fprintf(o->file, "%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+	h_name = DIFF_FILE_VALID(one) ? a_one : b_two;
+	fprintf(o->file, "%sdiff --git %s %s%s\n", set, h_name, h_name, reset);
 	if (lbl[0][0] == '/') {
 		/* /dev/null */
 		fprintf(o->file, "%snew file mode %06o%s\n", set, two->mode, reset);

On Sat, 4 Oct 2008, Linus Torvalds wrote:

Anyway, for now I'd suggest a patch like this. Hmm? The "diff --git" 
format assumes that the names are the same, so make it so.

Ahh, never mind, this one is broken. It's not _horribly_ so, but I 
shouldhave checked the test breakage. It changes non-broken cases from

	diff --git a/file b/file

into

	diff --git a/file a/file

so the header thing would need to be done differently. Don't even bother 
with that broken patch. I'll think about doing it better.

		Linus

On Sat, 4 Oct 2008, Linus Torvalds wrote:

Ahh, never mind, this one is broken.

Ok, so here's a much better version, I think.

It just refuses to use an invalid name for anything visible in the diff.

Now, this fixes the "git diff --no-index --binary a /dev/null" kind of 
case (and we'll end up using "a" as the basename), but some other insane 
cases are impossible to handle. If you do

	git diff --no-index --binary a /bin/echo

you'll still get a patch like

	diff --git a/a b/bin/echo
	old mode 100644
	new mode 100755
	index ...

and "git apply" will refuse to apply it for a couple of reasons, and the 
diff is simply bogus. 

And that, btw, is no longer a bug, I think. It's impossible to know whethe 
the user meant for the patch to be a rename or not. And as such, refusing 
to apply it because you don't know what name you should use is probably 
_exactly_ the right thing to do!

		Linus

----
 diff.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 9053d4d..197533e 100644
--- a/diff.c
+++ b/diff.c

@@ -1493,6 +1493,10 @@ static void builtin_diff(const char *name_a,
 		b_prefix = o->b_prefix;
 	}
 
+	/* Never use a non-valid filename anywhere if at all possible */
+	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
+	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
+
 	a_one = quote_two(a_prefix, name_a + (*name_a == '/'));
 	b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";

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

[argh, resending with the list cc'd]

On Sat, Oct 04, 2008 at 10:28:19AM +0200, Jakub Narebski wrote:

quoted

Hmm. The problem is that "git apply" doesn't accept that "a/dev/null"
and "b/a" are the same, so it rejects them as a name. I

Shouldn't it be "/dev/null", not "a/dev/null"?  

Yes, see my recent reply to Linus elsewhere in the thread for why I
didn't think it was an issue at the time (but it clearly is).

Besides git-diff(1) states:

  1. It is preceded with a "git diff" header, that looks like
     this:

       diff --git a/file1 b/file2

     The `a/` and `b/` filenames are the same unless rename/copy is
     involved.  Especially, even for a creation or a deletion,
     `/dev/null` is _not_ used in place of `a/` or `b/` filenames.

And I hadn't seen this, which makes it utterly clear that the diff is
broken. Thanks for pointing it out.

-Peff

On Sat, Oct 04, 2008 at 10:48:08AM -0700, Linus Torvalds wrote:

Ok, so here's a much better version, I think.

Yes, this approach makes sense to me after reading your other
explanation as well as Jakub's pointer to git-diff(1).

And it passes my test. ;)

I'll squash in my test case, add a little context to the commit message,
and send it to Shawn.

-Peff

On Sun, Oct 05, 2008 at 03:17:28PM -0400, Jeff King wrote:

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.

Ah, I found it. It was the fact that _text_ diffs using --no-index also
generate that bogosity. But of course they work fine, since we just
reject the "default" name from the "diff --git" line, but end up getting
the names from the "---" and "+++" lines.

-Peff