Re: git apply: git diff header lacks filename information for git diff --no-index patch
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);
Imre Deak
