{"thread":{"id":"723","subject":"git full diff output issues..","startedAt":"2005-05-26T19:19:21Z","lastAt":"2005-06-05T15:11:02Z","messageCount":13,"participants":["Linus Torvalds","Junio C Hamano","Anton Altaparmakov","Chris Wedgwood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"3989","messageId":"Pine.LNX.4.58.0505261214140.2307@ppc970.osdl.org","threadId":"723","inReplyTo":null,"subject":"git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-26T19:19:21Z","receivedAt":"2005-05-26T19:19:21Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWhile testing my \"git-apply\" thing (coming along quite nicely, thanks for\nasking), I've hit a case that is nasty to parse.\n\nThis is from the 2.6.12-rc4 -> 2.6.12-rc5 patch:\n\n\tdiff --git a/arch/um/kernel/checksum.c b/arch/um/kernel/checksum.c\n\tdeleted file mode 100644\n\tdiff --git a/arch/um/kernel/initrd.c b/arch/um/kernel/initrd.c\n\tnew file mode 100644\n\t--- /dev/null\n\t+++ b/arch/um/kernel/initrd.c\n\t@@ -0,0 +1,78 @@\n\nand the magic here is that deleted file that was empty to begin with, so \nit didn't have a patch, just a note on deletion.\n\nWhy is that nasty? Because we don't have the file _name_ in any good \nformat. The filename only exists int he \"diff --git\" header, and that one \nhas the space-parsing issue, which makes it less than optimal.\n\nI'd suggest we enhance the \"full diff\" output for new and deleted files to \nmatch the rename output, ie we'd give the actual filename on that line \ntoo, to avoid any ambiguities.\n\nSo we'd change it from\n\n\tdeleted file mode 100644\n\nto\n\n\tdeleted file mode 100644 arch/um/kernel/checksum.c\n\nin this case..\n\nComments?\n\n\t\tLinus\n"},{"id":"3990","messageId":"Pine.LNX.4.58.0505261223240.2307@ppc970.osdl.org","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261214140.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-26T19:25:15Z","receivedAt":"2005-05-26T19:25:15Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 26 May 2005, Linus Torvalds wrote:\n> \n> So we'd change it from\n> \n> \tdeleted file mode 100644\n> \n> to\n> \n> \tdeleted file mode 100644 arch/um/kernel/checksum.c\n> \n> in this case..\n\nI just realized that this same thing is equally true of just plain mode \nchanges, where wif we don't have any content we just get\n\n\tdiff --git a/name b/name\n\told mode xxxx\n\tnew mode yyyy\n\nso I might as well parse the diff header here (I don't want to repeat the \nname twice for mode changes). Oh well.\n\n\t\tLinus\n"},{"id":"3991","messageId":"7vwtplbwze.fsf@assigned-by-dhcp.cox.net","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261223240.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-26T19:36:37Z","receivedAt":"2005-05-26T19:36:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds writes:\n\nLT> On Thu, 26 May 2005, Linus Torvalds wrote:\n>> \n>> So we'd change it from\n>> \n>> deleted file mode 100644\n>> \n>> to\n>> \n>> deleted file mode 100644 arch/um/kernel/checksum.c\n>> \n>> in this case..\n\nLT> I just realized that this same thing is equally true of just plain mode \nLT> changes, where wif we don't have any content we just get\n\nLT> \tdiff --git a/name b/name\nLT> \told mode xxxx\nLT> \tnew mode yyyy\n\nLT> so I might as well parse the diff header here (I don't want to repeat the \nLT> name twice for mode changes). Oh well.\n\nSo what do you want? created and deleted would acquire path and\nmode thing doesn't? I think adding path only to \"new mode\" line\nwould be a sensible compromise, since we are interested in what\nthe resulting tree would look like most of the time.\n\n\n"},{"id":"3992","messageId":"7vpsvdbwj5.fsf@assigned-by-dhcp.cox.net","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261223240.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-26T19:46:22Z","receivedAt":"2005-05-26T19:46:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'd appreciate it if you take these two patches I sent last\nnight.\n\n * Add git-external-diff-script\n\n This is a demonstration of GIT_EXTERNAL_DIFF mechanism, and a\n testbed for tweaking and enhancing what the built-in diff should\n be. This script is designed to output exactly the same output\n as the built-in diff driver produces when set as GIT_EXTERNAL_DIFF.\n\n * Diff updates.\n\n With the introduction of 'T', and the \"apply-patch\" Linus has\n been quietly working on without much advertisement, it started\n to make sense to emit usable information in the \"diff --git\"\n patch output format. Earlier built-in diff driver punted and\n did not say anything about a symbolic link changing into a file\n or vice versa, but this version represents that as a pair of\n deletion and creation.\n\nAfter that, you can experiment to flush out issues with the\ncurrent built-in using git-external-diff-script for quick\nturnaround. When you have a concrete \"ok this is good\" format\nwe can port that to C in diff.c:builtin_diff().\n\n"},{"id":"3993","messageId":"Pine.LNX.4.60.0505262036500.16829@hermes-1.csi.cam.ac.uk","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261223240.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Anton Altaparmakov","fromEmail":"aia21@cam.ac.uk","sentAt":"2005-05-26T19:53:31Z","receivedAt":"2005-05-26T19:53:31Z","isPatch":false,"sender":{"key":"aia21@cam.ac.uk","avatar":null},"body":"On Thu, 26 May 2005, Linus Torvalds wrote:\n> On Thu, 26 May 2005, Linus Torvalds wrote:\n> > \n> > So we'd change it from\n> > \n> > \tdeleted file mode 100644\n> > \n> > to\n> > \n> > \tdeleted file mode 100644 arch/um/kernel/checksum.c\n> > \n> > in this case..\n> \n> I just realized that this same thing is equally true of just plain mode \n> changes, where wif we don't have any content we just get\n> \n> \tdiff --git a/name b/name\n> \told mode xxxx\n> \tnew mode yyyy\n> \n> so I might as well parse the diff header here (I don't want to repeat the \n> name twice for mode changes). Oh well.\n\nGiven that git already has the metadata lines in the diff (\"old mode\", \n\"deleted file mode\", etc) why not simply add another metadata line \"name\" \nand what follows that is the name until an end of line character (or a NUL \nif you want file names with embedded new lines). You can then only emit \nthe \"name\" metadata line when no actual diff is present and hence the name \nis uncertain.\n\nBest regards,\n\n\tAnton\n-- \nAnton Altaparmakov (replace at with @)\nUnix Support, Computing Service, University of Cambridge, CB2 3QH, UK\nLinux NTFS maintainer / IRC: #ntfs on irc.freenode.net\nWWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/\n"},{"id":"3997","messageId":"Pine.LNX.4.58.0505261316250.2307@ppc970.osdl.org","threadId":"723","inReplyTo":"Pine.LNX.4.60.0505262036500.16829@hermes-1.csi.cam.ac.uk","subject":"Re: git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-26T20:33:26Z","receivedAt":"2005-05-26T20:33:26Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 26 May 2005, Anton Altaparmakov wrote:\n> \n> Given that git already has the metadata lines in the diff (\"old mode\", \n> \"deleted file mode\", etc) why not simply add another metadata line \"name\" \n> and what follows that is the name until an end of line character (or a NUL \n> if you want file names with embedded new lines). You can then only emit \n> the \"name\" metadata line when no actual diff is present and hence the name \n> is uncertain.\n\nYes, that would work. \n\nHowever, I ended up just validating the name parsing by making sure that \nwhen I parse the \"git --diff\" line, I only take the name if I can see it \nbeing the same for both the old and the new. IOW, if I see\n\n\tdiff --git a/hi b/hello\n\nthen I won't take it, but if I see\n\n\tdiff --git hi there/I am/being difficult oopsie dir/I am/being difficult\n\nthen I get \"I am/being difficult\" by virtue of checking the two names \nagainst each other.\n\nThis means, btw, that the \"git --diff\" format must _not_ do\n\n\tdiff --git a/file /dev/null\n\tdeleted file mode 100644\n\nbecause in that case I don't trust the filename enough. Of course, this\nall only happens when deleting empty files, if the file had any contents,\nthen I will see the unambiguos filename on the '---' line, and again be\nhappy.\n\nIOW, git-apply is being pretty anal about things, but it looks like that\nworks out well.\n\n\t\t\tLinus\n"},{"id":"3999","messageId":"7v64x5bt9n.fsf@assigned-by-dhcp.cox.net","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261316250.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-26T20:56:52Z","receivedAt":"2005-05-26T20:56:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds writes:\n\nLT> This means, btw, that the \"git --diff\" format must _not_ do\n\nLT> \tdiff --git a/file /dev/null\nLT> \tdeleted file mode 100644\n\nI just checked, and both built-in and git-external-diff-script\nshould be safe about this issue.\n\nSo what do you want from me at this point? Nothing?\n \n\n"},{"id":"4001","messageId":"Pine.LNX.4.58.0505261402470.2307@ppc970.osdl.org","threadId":"723","inReplyTo":"7v64x5bt9n.fsf@assigned-by-dhcp.cox.net","subject":"Re: git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-26T21:09:31Z","receivedAt":"2005-05-26T21:09:31Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 26 May 2005, Junio C Hamano wrote:\n> \n> So what do you want from me at this point? Nothing?\n\nYeah, I'm happy. Sorry for the false alarm.\n\nAnyway, at this point\n\n\tgit-apply --stat\n\nis actually already useful: it's a diffstat clone. Which is perhaps not\nvery useful in itself, but it has the advantage of being an easy way to\ncheck that I do the right thing there, and may well be useful also for the\ngit-specific extensions (ie right now it's really purely a diffstat clone,\nbut there's nothign that says that it couldn't show rename information etc \ntoo, which diffstat doesn't understand).\n\n\t\tLinus\n"},{"id":"4004","messageId":"7vis15aczv.fsf@assigned-by-dhcp.cox.net","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261402470.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-05-26T21:33:40Z","receivedAt":"2005-05-26T21:33:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"LT\" == Linus Torvalds writes:\n\nLT> On Thu, 26 May 2005, Junio C Hamano wrote:\n>> \n>> So what do you want from me at this point? Nothing?\n\nLT> Yeah, I'm happy. Sorry for the false alarm.\n\nNo problem. I still kinda like Anton's proposal for conceptual\ncleanness, but if the tool can cope with what we already have\nthen less cluttering in the output is better for human eyes.\n\nLet me again remind you about git-external-diff-script patch.\nWhen you encounter more gotcha in the built-in diff output\nformat in the future, it would be a valuable tool to experiment\nand express what you would like to have git-apply to parse.\n\n"},{"id":"4011","messageId":"faf0d98cb35ad4b51c55d23d851093b5.ANY@taniwha.stupidest.org","threadId":"723","inReplyTo":"Pine.LNX.4.58.0505261214140.2307@ppc970.osdl.org","subject":"Re: git full diff output issues..","fromName":"Chris Wedgwood","fromEmail":"cw@f00f.org","sentAt":"2005-05-26T23:34:43Z","receivedAt":"2005-05-26T23:34:43Z","isPatch":false,"sender":{"key":"cw@f00f.org","avatar":null},"body":"On Thu, May 26, 2005 at 12:19:21PM -0700, Linus Torvalds wrote:\n\n> \tdeleted file mode 100644 arch/um/kernel/checksum.c\n\nwhy do we care about the mode here?\n"},{"id":"4012","messageId":"Pine.LNX.4.58.0505261648360.2307@ppc970.osdl.org","threadId":"723","inReplyTo":"faf0d98cb35ad4b51c55d23d851093b5.ANY@taniwha.stupidest.org","subject":"Re: git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-05-26T23:49:21Z","receivedAt":"2005-05-26T23:49:21Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 26 May 2005, Chris Wedgwood wrote:\n>\n> On Thu, May 26, 2005 at 12:19:21PM -0700, Linus Torvalds wrote:\n> \n> > \tdeleted file mode 100644 arch/um/kernel/checksum.c\n> \n> why do we care about the mode here?\n\nJunio makes the (correct) argument that patches should be reversible.\n\nAnd the reverse of a delete is a create. And thus the file mode of the\nfile that got deleted matters.\n\n\t\tLinus\n"},{"id":"4570","messageId":"7vu0kd42dm.fsf@assigned-by-dhcp.cox.net","threadId":"723","inReplyTo":"7v64x5bt9n.fsf@assigned-by-dhcp.cox.net","subject":"Re: git full diff output issues..","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-06-05T08:46:45Z","receivedAt":"2005-06-05T08:46:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">>>>> \"JCH\" == Junio C Hamano writes:\n>>>>> \"LT\" == Linus Torvalds writes:\n\nLT> This means, btw, that the \"git --diff\" format must _not_ do\n\nLT> diff --git a/file /dev/null\nLT> deleted file mode 100644\n\nJCH> I just checked, and both built-in and git-external-diff-script\nJCH> should be safe about this issue.\n\nSorry, I spoke too soon about a week and half ago X-<, and I am\nbugging you about this because this clearly belongs to \"fix\"\ncategory not \"new stuff\".\n\nThe case you mentioned (i.e. /dev/null) is fine but rename/copy\nis \"broken\" according to the definition by git-apply.\n\nWhat do you want the diff-patch format to say for this one?\n\n :100644 100644 SHA1-OLD SHA1-NEW R frotz.c nitfol.c\n\nCurrently I am saying:\n\n diff --git a/frotz.c b/nitfol.c\n similarity index 89%\n rename old frotz.c\n rename new nitfol.c\n --- a/frotz.c\n +++ b/nitfol.c\n @@ ...\n\nand this makes git-apply barf, because a/ and b/ names are\ndifferent. Is the following what you want? That is, do you\nalways want p->two->path (name in the right hand side tree)?\n\n diff --git a/nitfol.c b/nitfol.c\n similarity index 89%\n rename old frotz.c\n rename new nitfol.c\n --- a/frotz.c\n +++ b/nitfol.c\n @@ ...\n\nAccording to the current apply.c, git_header_name() does not\ncare as long as a/ and b/ names are the same (that is, I could\neven say \"diff --git a/junkio b/junkio\" to make it grok the\nabove example, as long as I have the correct \"rename old\" and\n\"rename new\" in the extended header part). In that sense, it\nall boils down to which name you, as a human consumer of the\npatch, would want to see on the header, if we go the route of\nmaking a/ and b/ name always the same. However I suspect that\nthis slightly breaks patch reversibility.\n\nIf we do care about patch reversibility, having a/ and b/ names\nto show the pre- and post- paths like my current output does\n(which _does_ break the current apply.c name checking) is\nprobably the most sensible thing to keep things symmetric. I am\nnot sure if it is worth it to make the name checking logic in\napply.c more complicated only to support this rename symmetry,\nthough.\n\nAnother possibility; since \"diff --git\" is a git-specific header\nformat anyway, we could quote things to help apply.c parsing it,\nwithout introducing too much clutter for ordinary cases. How\nabout taking advantage of the fact that most pathnames do not\ncontain spaces nor backslashes, and if we see them we simply\nquote, like this?\n\n # no need for quote\n diff --git a/frotz.c b/nitfol.c\n rename old frotz.c\n rename new nitfol.c\n \n # patch for \"frotz and nitfol.c\"\n diff --git a/frotz\\ and\\ nitfol.c b/frotz\\ and\\ nitfol.c\n\n # rename but filename has spaces and a backslash\n diff --git a/old\\ name\\\\with\\ bs b/new\\ name\\\\with\\ bs\n rename old old name\\with bs\n rename new new name\\with bs\n\n"},{"id":"4573","messageId":"Pine.LNX.4.58.0506050806400.1876@ppc970.osdl.org","threadId":"723","inReplyTo":"7vu0kd42dm.fsf@assigned-by-dhcp.cox.net","subject":"Re: git full diff output issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-06-05T15:11:02Z","receivedAt":"2005-06-05T15:11:02Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 5 Jun 2005, Junio C Hamano wrote:\n> \n> The case you mentioned (i.e. /dev/null) is fine but rename/copy\n> is \"broken\" according to the definition by git-apply.\n\nNo problem, the renames always get the names from the \"rename\" line, not \nthe header. Same goes for copies.\n\nIt's only modified files that keep the same name _and_ the same content \nthat don't have the name uniquely on a line somewhere.\n\n> What do you want the diff-patch format to say for this one?\n> \n> :100644 100644 SHA1-OLD SHA1-NEW R frotz.c nitfol.c\n> \n> Currently I am saying:\n> \n> diff --git a/frotz.c b/nitfol.c\n> similarity index 89%\n> rename old frotz.c\n> rename new nitfol.c\n> --- a/frotz.c\n> +++ b/nitfol.c\n\nThis finds the old names unambiguously in _two_ places: in the \"--- \" line \n(no question about where it begins: it's -p1, or where it ends - at the \nnewline) _and_ on the \"rename old xxxx\" line.\n\nThe only case that was special was literally the \"same name, no content \nchanges, new mode\" case, which looked like\n\n\tdiff --git a/oldname.c b/oldname.c\n\tnew mode 100755\n\told mode 100644\n\nand thus _only_ had the name in the (normally ambiguous wrt whitepsace) \nheader line.\n\nBut by having the requirement that the format of the header line for that\ncase is \"-p1\" together with both names being the same, it's not ambigious \nany more.\n\n\t\tLinus\n"}]}