{"thread":{"id":"15759","subject":"git apply: git diff header lacks filename information for git diff --no-index patch","startedAt":"2008-10-02T18:27:36Z","lastAt":"2008-10-05T19:24:39Z","messageCount":10,"participants":["Imre Deak","Jeff King","Jakub Narebski","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"92188","messageId":"500f3d130810021127j570bb540p901f6a73f58a6cb1@mail.gmail.com","threadId":"15759","inReplyTo":null,"subject":"git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Imre Deak","fromEmail":"imre.deak@gmail.com","sentAt":"2008-10-02T18:27:36Z","receivedAt":"2008-10-02T18:27:36Z","isPatch":false,"sender":{"key":"imre.deak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1049948?v=4"},"body":"Hi,\n\nI have the following problem:\n\n$ echo -e '\\x0' > a\n$ git diff --no-index --binary /dev/null a > patch\n$ rm a\n$ git apply patch\nfatal: git diff header lacks filename information (line 4)\n$ cat patch\ndiff --git a/dev/null b/a\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1\nGIT binary patch\nliteral 2\nJcmZSJ0ssIE01E&B\n\nliteral 0\nHcmV?d00001\n\nThe same works for text based patches:\n\n$ echo 1 > a\n$ git diff --no-index /dev/null a > patch\n$ rm a\n$ git apply patch\n$ ls\na\n$ cat patch\ndiff --git a/dev/null b/a\nnew file mode 100644\nindex 0000000..d00491f\n--- /dev/null\n+++ b/a\n@@ -0,0 +1 @@\n+1\n\nThe binary patch lacks ---/+++ lines but still provides the name info\non the diff --git line which I think should suffice for git apply.\n\n--Imre\n"},{"id":"92288","messageId":"20081004041714.GA12413@coredump.intra.peff.net","threadId":"15759","inReplyTo":"500f3d130810021127j570bb540p901f6a73f58a6cb1@mail.gmail.com","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-04T04:17:14Z","receivedAt":"2008-10-04T04:17:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:\n\n> $ git apply patch\n> fatal: git diff header lacks filename information (line 4)\n> $ cat patch\n> diff --git a/dev/null b/a\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1\n> GIT binary patch\n\nHmm. The problem is that \"git apply\" doesn't accept that \"a/dev/null\"\nand \"b/a\" are the same, so it rejects them as a name. I guess on a text\npatch, we would just pull that information from the \"---\" and \"+++\"\nlines, so we don't care that it's not on the diff commandline.\n\nHowever, a _non_ --no-index patch doesn't produce the same output. It\nwill actually produce the line:\n\n diff --git a/a b/a\n\neven if it is a creation patch. So I'm not sure which piece of code is\nat fault. Either:\n\n 1. git apply is right to reject, and \"git diff --no-index\" should be\n putting the actual filename on the commandline of a binary patch\n instead of /dev/null, even if it is a creation patch.\n\n or\n\n 2. git apply should accept this construct. Perhaps we should relax the\n \"both names must be the same\" rule if one of the names is /dev/null\n (and we would take the other)?\n\nLinus, the \"both names must be the same\" code in git_header_name blames\nto you (5041aa70). Thoughts on number 2?\n\n-Peff\n"},{"id":"92292","messageId":"gc79f4$22m$1@ger.gmane.org","threadId":"15759","inReplyTo":"20081004041714.GA12413@coredump.intra.peff.net","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-10-04T08:28:19Z","receivedAt":"2008-10-04T08:28:19Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"[Cc: git@vger.kernel.org, Jeff King , \n Imre Deak ]\n\nJeff King wrote:\n\n> On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:\n> \n>> $ git apply patch\n>> fatal: git diff header lacks filename information (line 4)\n>> $ cat patch\n>> diff --git a/dev/null b/a\n>> new file mode 100644\n>> index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1\n>> GIT binary patch\n> \n> Hmm. The problem is that \"git apply\" doesn't accept that \"a/dev/null\"\n> and \"b/a\" are the same, so it rejects them as a name. I\n\nShouldn't it be \"/dev/null\", not \"a/dev/null\"? \n\nBesides git-diff(1) states:\n\n 1. It is preceded with a \"git diff\" header, that looks like\n this:\n\n diff --git a/file1 b/file2\n\n The `a/` and `b/` filenames are the same unless rename/copy is\n involved. Especially, even for a creation or a deletion,\n `/dev/null` is _not_ used in place of `a/` or `b/` filenames.\n\nLooks like a bug in patch generation code...\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"92297","messageId":"alpine.LFD.2.00.0810040918290.3208@nehalem.linux-foundation.org","threadId":"15759","inReplyTo":"20081004041714.GA12413@coredump.intra.peff.net","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-10-04T16:54:36Z","receivedAt":"2008-10-04T16:54:36Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 4 Oct 2008, Jeff King wrote:\n> On Thu, Oct 02, 2008 at 09:27:36PM +0300, Imre Deak wrote:\n> > $ git apply patch\n> > fatal: git diff header lacks filename information (line 4)\n> > $ cat patch\n> > diff --git a/dev/null b/a\n> > new file mode 100644\n> > index 0000000000000000000000000000000000000000..1f2a4f5ef3df7f7456d91c961da36fc58904f2f1\n> > GIT binary patch\n\nOk, this patch is broken in so many ways..\n\n> Hmm. The problem is that \"git apply\" doesn't accept that \"a/dev/null\"\n> and \"b/a\" are the same, so it rejects them as a name. I guess on a text\n> patch, we would just pull that information from the \"---\" and \"+++\"\n> lines, so we don't care that it's not on the diff commandline.\n\nExactly. In order to avoid all the ambiguities, we want the filename to \nmatch on the 'diff -' line to even be able to guess, and if it doesn't, we \nshould pick it up from the \"rename from\" lines (for a git diff), or from \nthe '--- a/filename'/'+++ b/filename' otherwise (if it's not a rename, or \nnot a git diff).\n\nAnd being a binary diff, and a creation event, all of this fails.\n\nTo make things worse, git has also screwed up the \"/dev/null\" and \nprepended the prefix to it, making it even harder to see any patterns to \nthe names. Gaah.\n\n> However, a _non_ --no-index patch doesn't produce the same output. It\n> will actually produce the line:\n> \n> diff --git a/a b/a\n> \n> even if it is a creation patch. So I'm not sure which piece of code is\n> at fault.\n\nIt's some of both. I think --no-index is broken, that \"a/dev/null\" is \ntotal crap regardless of anything else. There's no way that thing is \ncorrect, and even if you were not to want \"a/a\", it should have been just \nplain \"/dev/null\". \n\nBut for a native git patch, we shouldn't even use the (at least slightly \nmore correct) \"/dev/null\", since native git application doesn't actually \ndo the \"is_dev_null()\" for native patches, and just wants the filename to \nbe the same.\n\nSo I think \"git apply\" is correct, and \"git diff --no-index\" is broken.\n\nThat said, I think git-apply being \"correct\" is not a great excuse, and we \nshould do the \"be liberal in what you accept, conservative in what you \ngenerate\", and think about how to make git-apply be more willing to handle \nthis case too. \n\nQuite frankly, I should have doen the explicit headers as\n\n\t\"new file \" SP \n\ninstead of\n\n\t\"new file mode \" \n\n(and same for \"deleted file\", obviously) and the patch would have been \nmore readable _and_ we'd have avoided this issue. But when I did all that, \nI didn't even think of binary diffs (they weren't an issue originally). \n\nDang.\n\nAnyway, for now I'd suggest a patch like this. Hmm? The \"diff --git\" \nformat assumes that the names are the same, so make it so.\n\n\t\t\tLinus\n\n---\n diff.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9053d4d..0910b4e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1479,7 +1479,7 @@ static void builtin_diff(const char *name_a,\n {\n \tmmfile_t mf1, mf2;\n \tconst char *lbl[2];\n-\tchar *a_one, *b_two;\n+\tchar *a_one, *b_two, *h_name;\n \tconst char *set = diff_get_color_opt(o, DIFF_METAINFO);\n \tconst char *reset = diff_get_color_opt(o, DIFF_RESET);\n \tconst char *a_prefix, *b_prefix;\n@@ -1497,7 +1497,8 @@ static void builtin_diff(const char *name_a,\n \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n \tlbl[1] = DIFF_FILE_VALID(two) ? b_two : \"/dev/null\";\n-\tfprintf(o->file, \"%sdiff --git %s %s%s\\n\", set, a_one, b_two, reset);\n+\th_name = DIFF_FILE_VALID(one) ? a_one : b_two;\n+\tfprintf(o->file, \"%sdiff --git %s %s%s\\n\", set, h_name, h_name, reset);\n \tif (lbl[0][0] == '/') {\n \t\t/* /dev/null */\n \t\tfprintf(o->file, \"%snew file mode %06o%s\\n\", set, two->mode, reset);\n"},{"id":"92298","messageId":"alpine.LFD.2.00.0810041004290.3208@nehalem.linux-foundation.org","threadId":"15759","inReplyTo":"alpine.LFD.2.00.0810040918290.3208@nehalem.linux-foundation.org","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-10-04T17:08:58Z","receivedAt":"2008-10-04T17:08:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 4 Oct 2008, Linus Torvalds wrote:\n> \n> Anyway, for now I'd suggest a patch like this. Hmm? The \"diff --git\" \n> format assumes that the names are the same, so make it so.\n\nAhh, never mind, this one is broken. It's not _horribly_ so, but I \nshouldhave checked the test breakage. It changes non-broken cases from\n\n\tdiff --git a/file b/file\n\ninto\n\n\tdiff --git a/file a/file\n\nso the header thing would need to be done differently. Don't even bother \nwith that broken patch. I'll think about doing it better.\n\n\t\tLinus\n"},{"id":"92299","messageId":"alpine.LFD.2.00.0810041031580.3208@nehalem.linux-foundation.org","threadId":"15759","inReplyTo":"alpine.LFD.2.00.0810041004290.3208@nehalem.linux-foundation.org","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-10-04T17:48:08Z","receivedAt":"2008-10-04T17:48:08Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 4 Oct 2008, Linus Torvalds wrote:\n> \n> Ahh, never mind, this one is broken.\n\nOk, so here's a much better version, I think.\n\nIt just refuses to use an invalid name for anything visible in the diff.\n\nNow, this fixes the \"git diff --no-index --binary a /dev/null\" kind of \ncase (and we'll end up using \"a\" as the basename), but some other insane \ncases are impossible to handle. If you do\n\n\tgit diff --no-index --binary a /bin/echo\n\nyou'll still get a patch like\n\n\tdiff --git a/a b/bin/echo\n\told mode 100644\n\tnew mode 100755\n\tindex ...\n\nand \"git apply\" will refuse to apply it for a couple of reasons, and the \ndiff is simply bogus. \n\nAnd that, btw, is no longer a bug, I think. It's impossible to know whethe \nthe user meant for the patch to be a rename or not. And as such, refusing \nto apply it because you don't know what name you should use is probably \n_exactly_ the right thing to do!\n\n\t\tLinus\n\n----\n diff.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9053d4d..197533e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1493,6 +1493,10 @@ static void builtin_diff(const char *name_a,\n \t\tb_prefix = o->b_prefix;\n \t}\n \n+\t/* Never use a non-valid filename anywhere if at all possible */\n+\tname_a = DIFF_FILE_VALID(one) ? name_a : name_b;\n+\tname_b = DIFF_FILE_VALID(two) ? name_b : name_a;\n+\n \ta_one = quote_two(a_prefix, name_a + (*name_a == '/'));\n \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n"},{"id":"92347","messageId":"20081005191728.GA6173@coredump.intra.peff.net","threadId":"15759","inReplyTo":"alpine.LFD.2.00.0810040918290.3208@nehalem.linux-foundation.org","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T19:17:28Z","receivedAt":"2008-10-05T19:17:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 04, 2008 at 09:54:36AM -0700, Linus Torvalds wrote:\n\n> Exactly. In order to avoid all the ambiguities, we want the filename to \n> match on the 'diff -' line to even be able to guess, and if it doesn't, we \n> should pick it up from the \"rename from\" lines (for a git diff), or from \n> the '--- a/filename'/'+++ b/filename' otherwise (if it's not a rename, or \n> not a git diff).\n> \n> And being a binary diff, and a creation event, all of this fails.\n\nI wonder if it might have been better for binary diffs, like text diffs,\nto contain the \"from\" and \"to\" filenames in a similar format. But at\nthis point I don't think a format change is really worthwhile.\n\n> To make things worse, git has also screwed up the \"/dev/null\" and \n> prepended the prefix to it, making it even harder to see any patterns to \n> the names. Gaah.\n\nYes, I noticed that, as well. And obviously it looks bogus, but I\nthought I managed to get \"git diff\" to produce \"a/dev/null\" on some\notherwise valid input, and so assumed that was something we were able to\nwork around in applying the patch. But testing again today, I can't seem\nto get anything except this broken diff to say \"a/dev/null\". So probably\nI was just mistaken yesterday.\n\n> So I think \"git apply\" is correct, and \"git diff --no-index\" is broken.\n\nOK, your reasoning is sound.\n\n> That said, I think git-apply being \"correct\" is not a great excuse, and we \n> should do the \"be liberal in what you accept, conservative in what you \n> generate\", and think about how to make git-apply be more willing to handle \n> this case too. \n\nI think for now we might as well just fix the broken \"diff\" output. The\nonly thing generating these broken diffs will be older versions of git,\nand such diffs are presumably rare (given that this is the first report\nwe've seen).\n\nSo the only advantage would be to accept rare patches from people with\nolder git, versus asking them to upgrade to a non-broken git.\n\n> Quite frankly, I should have doen the explicit headers as\n> \n> \t\"new file \" SP \n> \n> instead of\n> \n> \t\"new file mode \" \n> \n> (and same for \"deleted file\", obviously) and the patch would have been \n> more readable _and_ we'd have avoided this issue. But when I did all that, \n> I didn't even think of binary diffs (they weren't an issue originally). \n\nAgreed (and I think this is just another form of what I mentioned\nabove; in my suggestion, we would include the filename on _all_ binary\ndiffs. In practice, it wouldn't matter in non-creation cases, since we\nactually get the \"diff --git\" line _right_ in those cases :) ).\n\nBut again, I don't think it is worth trying to change the format now.\n\n-Peff\n"},{"id":"92348","messageId":"20081005191903.GA13980@coredump.intra.peff.net","threadId":"15759","inReplyTo":"gc79f4$22m$1@ger.gmane.org","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T19:19:03Z","receivedAt":"2008-10-05T19:19:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[argh, resending with the list cc'd]\n\nOn Sat, Oct 04, 2008 at 10:28:19AM +0200, Jakub Narebski wrote:\n\n> > Hmm. The problem is that \"git apply\" doesn't accept that \"a/dev/null\"\n> > and \"b/a\" are the same, so it rejects them as a name. I\n> \n> Shouldn't it be \"/dev/null\", not \"a/dev/null\"? \n\nYes, see my recent reply to Linus elsewhere in the thread for why I\ndidn't think it was an issue at the time (but it clearly is).\n\n> Besides git-diff(1) states:\n> \n> 1. It is preceded with a \"git diff\" header, that looks like\n> this:\n> \n> diff --git a/file1 b/file2\n> \n> The `a/` and `b/` filenames are the same unless rename/copy is\n> involved. Especially, even for a creation or a deletion,\n> `/dev/null` is _not_ used in place of `a/` or `b/` filenames.\n\nAnd I hadn't seen this, which makes it utterly clear that the diff is\nbroken. Thanks for pointing it out.\n\n-Peff\n"},{"id":"92349","messageId":"20081005192128.GC6173@coredump.intra.peff.net","threadId":"15759","inReplyTo":"alpine.LFD.2.00.0810041031580.3208@nehalem.linux-foundation.org","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T19:21:28Z","receivedAt":"2008-10-05T19:21:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 04, 2008 at 10:48:08AM -0700, Linus Torvalds wrote:\n\n> Ok, so here's a much better version, I think.\n\nYes, this approach makes sense to me after reading your other\nexplanation as well as Jakub's pointer to git-diff(1).\n\nAnd it passes my test. ;)\n\nI'll squash in my test case, add a little context to the commit message,\nand send it to Shawn.\n\n-Peff\n"},{"id":"92350","messageId":"20081005192439.GD6173@coredump.intra.peff.net","threadId":"15759","inReplyTo":"20081005191728.GA6173@coredump.intra.peff.net","subject":"Re: git apply: git diff header lacks filename information for git diff --no-index patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-05T19:24:39Z","receivedAt":"2008-10-05T19:24:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 05, 2008 at 03:17:28PM -0400, Jeff King wrote:\n\n> Yes, I noticed that, as well. And obviously it looks bogus, but I\n> thought I managed to get \"git diff\" to produce \"a/dev/null\" on some\n> otherwise valid input, and so assumed that was something we were able to\n> work around in applying the patch. But testing again today, I can't seem\n> to get anything except this broken diff to say \"a/dev/null\". So probably\n> I was just mistaken yesterday.\n\nAh, I found it. It was the fact that _text_ diffs using --no-index also\ngenerate that bogosity. But of course they work fine, since we just\nreject the \"default\" name from the \"diff --git\" line, but end up getting\nthe names from the \"---\" and \"+++\" lines.\n\n-Peff\n"}]}