Re: git apply: git diff header lacks filename information for git diff --no-index patch
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Oct 4, 2008, 16:54 UTC
- Message-ID
- <alpine.LFD.2.00.0810040918290.3208@nehalem.linux-foundation.org>
- In-Reply-To
- <20081004041714.GA12413@coredump.intra.peff.net>
On Sat, 4 Oct 2008, Jeff King wrote:
Show 8 quoted lines
> On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote: > > $ 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.
Show 7 quoted lines
> 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);