{"thread":{"id":"9282","subject":"[PATCH] Documentation/git-diff: remove -r from --name-status example","startedAt":"2007-07-29T00:24:27Z","lastAt":"2007-07-29T16:51:17Z","messageCount":18,"participants":["Jeff King","Linus Torvalds","Junio C Hamano","David Kastrup","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"48941","messageId":"20070729002427.GA1566@coredump.intra.peff.net","threadId":"9282","inReplyTo":null,"subject":"[PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T00:24:27Z","receivedAt":"2007-07-29T00:24:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Calling 'git-diff --name-status' will recursively show any\nchanges already, and it has for quite some time (at least as\nfar back as v1.4.1).\n\nSigned-off-by: Jeff King \n---\nOn Sat, Jul 28, 2007 at 05:26:27PM +0200, Jakub Narebski wrote:\n\n> > How about --name-status?\n> \n> Or -r --name-status?\n\nThe '-r' now seems to be superfluous. I checked using the following\nscript:\n\n mkdir repo && cd repo && git-init &&\n touch root && git-add root && git-commit -m root &&\n mkdir sub && touch sub/file && git-add sub/file &&\n git-diff --cached --name-status\n\nAnd it correctly reports\n\n A sub/file\n\nat least since v1.4.1. I didn't look further, but the example is from\nthe 0.99 era, so I suspect this behavior was changed with the\nlibification of the revision machinery and the reworking of git-diff.\n\nOr maybe I just totally don't understand what '-r' is supposed to be\ndoing.\n\n Documentation/git-diff.txt | 7 ++-----\n 1 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 639b969..b1f5e7f 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -102,17 +102,14 @@ Limiting the diff output::\n +\n ------------\n $ git diff --diff-filter=MRC <1>\n-$ git diff --name-status -r <2>\n+$ git diff --name-status <2>\n $ git diff arch/i386 include/asm-i386 <3>\n ------------\n +\n <1> show only modification, rename and copy, but not addition\n nor deletion.\n <2> show only names and the nature of change, but not actual\n-diff output. --name-status disables usual patch generation\n-which in turn also disables recursive behavior, so without -r\n-you would only see the directory name if there is a change in a\n-file in a subdirectory.\n+diff output.\n <3> limit diff output to named subtrees.\n \n Munging the diff output::\n-- \n1.5.3.rc3.845.g88e3-dirty\n"},{"id":"48947","messageId":"alpine.LFD.0.999.0707281905050.3442@woody.linux-foundation.org","threadId":"9282","inReplyTo":"20070729002427.GA1566@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-29T02:06:38Z","receivedAt":"2007-07-29T02:06:38Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 28 Jul 2007, Jeff King wrote:\n> \n> The '-r' now seems to be superfluous.\n\nFor diffing against (or using) the index, the \"-r\" is superfluous.\n\nWhy? Because the index is always the *full* list of files. It's \"flat\".\n\nHowever, when you diff two trees, the -r makes a difference.\n\nSo I think you'd find a difference if you actually diffed two commits \nwith \"git diff tree2..tree2\".\n\n\t\tLinus\n"},{"id":"48950","messageId":"20070729041159.GA5544@coredump.intra.peff.net","threadId":"9282","inReplyTo":"alpine.LFD.0.999.0707281905050.3442@woody.linux-foundation.org","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T04:11:59Z","receivedAt":"2007-07-29T04:11:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2007 at 07:06:38PM -0700, Linus Torvalds wrote:\n\n> For diffing against (or using) the index, the \"-r\" is superfluous.\n> \n> Why? Because the index is always the *full* list of files. It's \"flat\".\n> \n> However, when you diff two trees, the -r makes a difference.\n> \n> So I think you'd find a difference if you actually diffed two commits \n> with \"git diff tree2..tree2\".\n\nAh...right you are.\n\nSo if I \"git diff\" two commits with --raw or --name-status, we don't\nrecurse into recurse into subdirectories (because they are actually\nsubtrees). If I \"git diff\" a commit against the index using --raw or\n--name-status, I we do recurse (since the index is actually flat). But\nif I \"git diff\" using -p, --stat, or --summary, it _does_ recurse no\nmatter what I'm diffing.\n\nDoes anybody else find this behavior confusing? I can understand why\ndiff-tree might not recurse by default, but I wonder if porcelain like\ngit-diff should try to be a little more consistent and always recurse.\n\nSomething like:\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7f367b6..b48121e 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"diff_setup_done failed\");\n \t}\n \trev.diffopt.allow_external = 1;\n+\trev.diffopt.recursive = 1;\n \n \t/* Do we have --cached and not have a pending object, then\n \t * default to HEAD by hand. Eek.\n\nFor comparison, whatchanged, show, and format-patch are already always\nrecursive. log is not.\n\n-Peff\n"},{"id":"48951","messageId":"alpine.LFD.0.999.0707282119010.3442@woody.linux-foundation.org","threadId":"9282","inReplyTo":"20070729041159.GA5544@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-29T04:27:18Z","receivedAt":"2007-07-29T04:27:18Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 29 Jul 2007, Jeff King wrote:\n> \n> So if I \"git diff\" two commits with --raw or --name-status, we don't\n> recurse into recurse into subdirectories (because they are actually\n> subtrees).\n\nExactly.\n\nAs it pertains to \"git diff\", none of this actually makes sense, and I \nthink it might make sense to make \"git diff\" automatically set \n\"recursive\", exactly because \"git diff\" sometimes operates on trees, and \nsometimes on the index.\n\n> If I \"git diff\" a commit against the index using --raw or\n> --name-status, I we do recurse (since the index is actually flat). But\n> if I \"git diff\" using -p, --stat, or --summary, it _does_ recurse no\n> matter what I'm diffing.\n\nWhen you ask for patches, it has to recurse, because there is no way to \nshow a patch for a directory. So yes, anything non-raw will always enable \nrecursing.\n\nThe fact that \"--name-status\" is not considered a \"patch\" is an \nimplementation detail, and I would _almost_ suggest that we just make it \nalways recurse, and leave thenon-recursing case for _just_ \"--raw\". But \nthat is a separate decision.\n\n> Does anybody else find this behavior confusing? I can understand why\n> diff-tree might not recurse by default, but I wonder if porcelain like\n> git-diff should try to be a little more consistent and always recurse.\n\nI do agree.\n\nThe behaviour is obviously historical, and comes from \"git diff\" being \njust a shell-script wrapper around the different versions of diffing trees \nand indexes etc.\n\nSo it makes sense in that historical setting (and realizing that the \n\"HEAD<->index\" and \"index<->files\" cases were really a totally different \noperations), but it makes no sense in the modern world where people don't \neven *know* about \"git diff-tree\", but just use \"git diff\" for everything.\n\nSo:\n\n> Something like:\n\nAck. Patch looks fine, makes sense, and is obviously good.\n\nIt *is* a change in behaviour, though, so I can understand if Junio \ndoesn't think it's appropriate this late in the 1.5.3 series.\n\n> For comparison, whatchanged, show, and format-patch are already always\n> recursive. log is not.\n\nYeah. The other cases already default to patches, so they get the \nrecursive from there.\n\n\"git log\", of course, defaults to no output at all, so the only way to get \nnon-recursive behaviour is to ask for \"--raw\", and then having to specify \nexplicitly whether to get recursion or not make sense. Once you want raw \noutput, it really _is_ your choice.\n\nWhether we should make \"--name-status\" default to \"-r\" is worth \ndiscussing. I don't have any really strong opinion, although I _suspect_ \nthat we should. The non-recursive case is useful, but in a very limited \nsense, and I think we might as well limit it to just the --raw case.\n\n\t\tLinus\n"},{"id":"48952","messageId":"7vbqdvolww.fsf@assigned-by-dhcp.cox.net","threadId":"9282","inReplyTo":"alpine.LFD.0.999.0707282119010.3442@woody.linux-foundation.org","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-29T04:48:15Z","receivedAt":"2007-07-29T04:48:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n>> Does anybody else find this behavior confusing? I can understand why\n>> diff-tree might not recurse by default, but I wonder if porcelain like\n>> git-diff should try to be a little more consistent and always recurse.\n>\n> I do agree.\n>\n> The behaviour is obviously historical, and comes from \"git diff\" being \n> just a shell-script wrapper around the different versions of diffing trees \n> and indexes etc.\n>\n> So it makes sense in that historical setting (and realizing that the \n> \"HEAD<->index\" and \"index<->files\" cases were really a totally different \n> operations), but it makes no sense in the modern world where people don't \n> even *know* about \"git diff-tree\", but just use \"git diff\" for everything.\n>\n> So:\n>\n>> Something like:\n>\n> Ack. Patch looks fine, makes sense, and is obviously good.\n\nThat makes it two of us. ... eh, excuse me, there is one issue\nI mention at the end.\n\n> It *is* a change in behaviour, though, so I can understand if Junio \n> doesn't think it's appropriate this late in the 1.5.3 series.\n\nOne minor objection I do have is that, just as a matter of\nprinciple, in order to avoid setting precedence of making a\nfundamental semantics change in late -rc stage in the game, we\nshould not swallow it. I do not mind if this were clearly a\ngood change.\n\nHowever, I think Jeff's patch always makes it recursive even\nwhen the user asks for --raw, which makes it inappropriate for\ninclusion whether before or after 1.5.3.\n"},{"id":"48953","messageId":"20070729045217.GA6858@coredump.intra.peff.net","threadId":"9282","inReplyTo":"alpine.LFD.0.999.0707282119010.3442@woody.linux-foundation.org","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T04:52:17Z","receivedAt":"2007-07-29T04:52:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2007 at 09:27:18PM -0700, Linus Torvalds wrote:\n\n> The fact that \"--name-status\" is not considered a \"patch\" is an \n> implementation detail, and I would _almost_ suggest that we just make it \n> always recurse, and leave thenon-recursing case for _just_ \"--raw\". But \n> that is a separate decision.\n\nI think it makes some sense, but I'm not sure it is worth it for two\nreasons:\n 1. --raw and --name-status are very linked in the diff code. Turning\n on recursion for --name-status without --raw will be somewhat\n hairy.\n 2. If all of the porcelain tools turn on recursion anyway, that\n implementation detail just goes away anyway.\n\nOn the other hand, it looks like we just set options->recursive = 1 for\nmost formats anyway. So doing \"git-log --raw\" will not recurse, but\n\"git-log --raw -p\" will recurse for _both_ the raw and the log formats.\nWhich I think is a bit non-intuitive, but it certainly makes the patch\nmuch simpler:\n\ndiff --git a/diff.c b/diff.c\nindex a5fc56b..3137cac 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2151,6 +2151,7 @@ int diff_setup_done(struct diff_options *options)\n \t\t\t\t DIFF_FORMAT_DIFFSTAT |\n \t\t\t\t DIFF_FORMAT_SHORTSTAT |\n \t\t\t\t DIFF_FORMAT_SUMMARY |\n+\t\t\t\t DIFF_FORMAT_NAME_STATUS |\n \t\t\t\t DIFF_FORMAT_CHECKDIFF))\n \t\toptions->recursive = 1;\n \t/*\n\n> It *is* a change in behaviour, though, so I can understand if Junio \n> doesn't think it's appropriate this late in the 1.5.3 series.\n\nAgreed.\n\n> \"git log\", of course, defaults to no output at all, so the only way to get \n> non-recursive behaviour is to ask for \"--raw\", and then having to specify \n> explicitly whether to get recursion or not make sense. Once you want raw \n> output, it really _is_ your choice.\n\nI wonder if git-log should match the behavior of the other commands. I\nhave often see git-whatchanged explained as the equivalent of \"git-log\n-p\" but that's not exactly true. Similarly, git-show has been explained\nas \"git-log -p for the first commit.\" And using \"-p\" they are basically\nthe same. But it means I can't just replace all use of \"git-whatchanged\"\nwith \"git-log\" and get the exact same behavior. I think it makes to\nharmonize these little \"gotchas\".\n\n-Peff\n"},{"id":"48954","messageId":"20070729045646.GB6858@coredump.intra.peff.net","threadId":"9282","inReplyTo":"7vbqdvolww.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T04:56:46Z","receivedAt":"2007-07-29T04:56:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 28, 2007 at 09:48:15PM -0700, Junio C Hamano wrote:\n\n> However, I think Jeff's patch always makes it recursive even\n> when the user asks for --raw, which makes it inappropriate for\n> inclusion whether before or after 1.5.3.\n\nRight, that's the point. git-diff is currently inconsistent (unless you\nunderstand that index comparisons are always recursed, and tree\ncomparisons need it explicitly -- but part of the point of git-diff is\nto abstract those sorts of details), so this attempts to harmonize the\nbehavior no matter what you're diffing (whether it be --name-status or\n--raw).\n\nIf you really want not to recurse, then you have to know you are\ncomparing trees anyway, at which point it makes sense to use the\ngit-diff-tree plumbing.\n\n-Peff\n"},{"id":"48965","messageId":"85r6mrzkib.fsf@lola.goethe.zz","threadId":"9282","inReplyTo":"7vbqdvolww.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-07-29T08:23:08Z","receivedAt":"2007-07-29T08:23:08Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n> One minor objection I do have is that, just as a matter of\n> principle, in order to avoid setting precedence of making a\n> fundamental semantics change in late -rc stage in the game, we\n> should not swallow it. I do not mind if this were clearly a\n> good change.\n>\n> However, I think Jeff's patch always makes it recursive even\n> when the user asks for --raw, which makes it inappropriate for\n> inclusion whether before or after 1.5.3.\n\nPersonally, I think -r should be the default while one can use\n--directory to switch it off.\n\nWhether --raw should imply --directory might be a different question.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"48972","messageId":"7vwswjmu08.fsf@assigned-by-dhcp.cox.net","threadId":"9282","inReplyTo":"7vbqdvolww.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-29T09:36:23Z","receivedAt":"2007-07-29T09:36:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n> ...\n>> Ack. Patch looks fine, makes sense, and is obviously good.\n>\n> That makes it two of us. ... eh, excuse me, there is one issue\n> I mention at the end.\n>\n>> It *is* a change in behaviour, though, so I can understand if Junio \n>> doesn't think it's appropriate this late in the 1.5.3 series.\n>\n> One minor objection I do have is that, just as a matter of\n> principle, in order to avoid setting precedence of making a\n> fundamental semantics change in late -rc stage in the game, we\n> should not swallow it. I do not mind if this were clearly a\n> good change.\n>\n> However, I think Jeff's patch always makes it recursive even\n> when the user asks for --raw, which makes it inappropriate for\n> inclusion whether before or after 1.5.3.\n\nActually, I would like to take this back. The highest level UI\n\"git-diff\" is for the end-users, and most of them typically do\nnot seem to want non recursive \"birds-eye\" view of changes.\n\nThe top-level non-recursive behaviour is not always useful as a\nbirds-eye view. Even in Linux kernel context, when you are\ninterested in one area e.g. i386, the answer the non recursive\nbehaviour gives us would not show what you typically want to see\n(i.e. include/asm-i386 and arch/i386), but it stops at the\n\"toplevel\", i.e. include and arch.\n\nIn addition, it is not like our tree diff is too slow and not\nrecursing into lower level makes it usable.\n\nIn short, I think recursive behaviour by default makes sense for\n\"git diff\".\n\nWhen people want to use lower level git-diff-* family, I think\nthe traditional behaviour is fine. We _might_ want to default\nthe --name-status and --name-only to recursive behaviour as\nwell, but (1) that is an independent topic, and (2) we should\nre-examine what the appropriate recursiveness for the other\noptions.\n"},{"id":"48974","messageId":"20070729094955.GA14915@coredump.intra.peff.net","threadId":"9282","inReplyTo":"7vwswjmu08.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T09:49:56Z","receivedAt":"2007-07-29T09:49:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2007 at 02:36:23AM -0700, Junio C Hamano wrote:\n\n> In short, I think recursive behaviour by default makes sense for\n> \"git diff\".\n\nYour reasoning sounds sane to me.\n\nHere is the patch with a commit message more succint than all of these\nemails.\n\n-- >8 --\ngit-diff: turn on recursion by default\n\nThe tree recursion behavior of git-diff may appear\ninconsistent to the user because it depends on the format of\nthe patch as well as whether one is diffing between trees or\nagainst the index.\n\nSince git-diff is a porcelain wrapper for low-level diff\ncommands, it makes sense for its behavior to be consistent\nno matter what is being diffed. This patch turns on\nrecursion in all cases.\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7f367b6..b48121e 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"diff_setup_done failed\");\n \t}\n \trev.diffopt.allow_external = 1;\n+\trev.diffopt.recursive = 1;\n \n \t/* Do we have --cached and not have a pending object, then\n \t * default to HEAD by hand. Eek.\n"},{"id":"48975","messageId":"Pine.LNX.4.64.0707291214060.14781@racer.site","threadId":"9282","inReplyTo":"20070729094955.GA14915@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-29T11:14:49Z","receivedAt":"2007-07-29T11:14:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 29 Jul 2007, Jeff King wrote:\n\n> -- >8 --\n> git-diff: turn on recursion by default\n> \n> The tree recursion behavior of git-diff may appear\n> inconsistent to the user because it depends on the format of\n> the patch as well as whether one is diffing between trees or\n> against the index.\n> \n> Since git-diff is a porcelain wrapper for low-level diff\n> commands, it makes sense for its behavior to be consistent\n> no matter what is being diffed. This patch turns on\n> recursion in all cases.\n> \n> diff --git a/builtin-diff.c b/builtin-diff.c\n> index 7f367b6..b48121e 100644\n> --- a/builtin-diff.c\n> +++ b/builtin-diff.c\n> @@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(\"diff_setup_done failed\");\n> \t}\n> \trev.diffopt.allow_external = 1;\n> +\trev.diffopt.recursive = 1;\n\nHow about\n\n\tif (!rev.diffopt.quiet)\n\t\trev.diffopt.recursive = 1;\n\ninstead?\n\nCiao,\nDscho\n"},{"id":"48977","messageId":"85bqdvxx4o.fsf@lola.goethe.zz","threadId":"9282","inReplyTo":"Pine.LNX.4.64.0707291214060.14781@racer.site","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-07-29T11:33:27Z","receivedAt":"2007-07-29T11:33:27Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Johannes Schindelin writes:\n\n\n>> \t}\n>> \trev.diffopt.allow_external = 1;\n>> +\trev.diffopt.recursive = 1;\n>\n> How about\n>\n> \tif (!rev.diffopt.quiet)\n> \t\trev.diffopt.recursive = 1;\n>\n> instead?\n\nI think that the optimization \"don't descend if we can figure this out\nat the top level\" should be rather implemented at the program flow\nlevel than in the option processing, and quietness does not actually\nplay into this: _any_ diff operation can skip trees with identical\ntop-level SHA1.\n\nSo the above optimization should not cause a performance difference\n(if it does, this is better fixed elsewhere), and makes it obscure to\nguess the \"-q\" behavior which should be more or less equivalent to\n--exit-status >/dev/null\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"48978","messageId":"20070729113850.GA17796@coredump.intra.peff.net","threadId":"9282","inReplyTo":"Pine.LNX.4.64.0707291214060.14781@racer.site","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T11:38:51Z","receivedAt":"2007-07-29T11:38:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2007 at 12:14:49PM +0100, Johannes Schindelin wrote:\n\n> How about\n> \n> \tif (!rev.diffopt.quiet)\n> \t\trev.diffopt.recursive = 1;\n> \n> instead?\n\nCan you explain?\n\nMy assumption is that you want the diff machinery to exit as quickly as\npossible. But the \"recursive\" option is _already_ turned on for most\noutput formats in that case, so I had assumed that it was the quiet\noption itself which caused the early return, not a failure to set\n\"recursive\".\n\nIf you don't want recursive set for quiet runs, I think there is an\nerror in diff.c, which sets the output format from quiet to NO_FORMAT\nonly _after_ having set recursive based on the output format.\n\n-Peff\n"},{"id":"48981","messageId":"Pine.LNX.4.64.0707291258410.14781@racer.site","threadId":"9282","inReplyTo":"20070729113850.GA17796@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-29T12:04:13Z","receivedAt":"2007-07-29T12:04:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 29 Jul 2007, Jeff King wrote:\n\n> On Sun, Jul 29, 2007 at 12:14:49PM +0100, Johannes Schindelin wrote:\n> \n> > How about\n> > \n> > \tif (!rev.diffopt.quiet)\n> > \t\trev.diffopt.recursive = 1;\n> > \n> > instead?\n> \n> Can you explain?\n\nThe idea is this: when \"--quiet\" was given, we do not output anything, and \ntherefore do not have to recurse into the directories, because we already \nknow that there are differences when a _tree_ is different. I do not \nremember all details of the \"--quiet\" implementation, but I think that it\n\n- exits early (as you said)\n\n- does not turn on \"recursive\" to avoid unnecessary work.\n\nImagine something like this: tree \"a\" and \"b\" contain 100,000 elements \neach, which are identical except for the last entry. \"--quiet\" does not \nneed to check the 99,999 elements before that one, since the tree hashes \nare already different.\n\nOf course, this reasoning breaks down fatally when you specify something \nlike \"--ignore-whitespace\", but those cases should turn on recursive \nexplicitely, so that the performance penalty of \"recursive = 1\" does not \npercolate back to the (much more common) trivial cases.\n\nCiao,\nDscho\n"},{"id":"48983","messageId":"20070729121948.GA21983@coredump.intra.peff.net","threadId":"9282","inReplyTo":"Pine.LNX.4.64.0707291258410.14781@racer.site","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T12:19:48Z","receivedAt":"2007-07-29T12:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2007 at 01:04:13PM +0100, Johannes Schindelin wrote:\n\n> The idea is this: when \"--quiet\" was given, we do not output anything, and \n> therefore do not have to recurse into the directories, because we already \n> know that there are differences when a _tree_ is different. I do not \n> remember all details of the \"--quiet\" implementation, but I think that it\n> \n> - exits early (as you said)\n> \n> - does not turn on \"recursive\" to avoid unnecessary work.\n\nOK, looking through the code, this works _sometimes_. If I say \"git-diff\n--quiet\" then it will not recurse. If I say \"git-diff -p --quiet\" then\nit will (even though we never show the -p output).\n\nSince --quiet supersedes all output formats, I think it probably should\njust clear the recursive option entirely. In which case rather than\nspecial-casing quiet to avoid recursion in git-diff, we can simply turn\non recursion before parsing options (and it will get turned off\ncorrectly by any diff options that need to do so).\n\nSomething like:\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex 7f367b6..e6d10bd 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -229,6 +229,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\targc = setup_revisions(argc, argv, &rev, NULL);\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\t\trev.diffopt.recursive = 1;\n \t\tif (diff_setup_done(&rev.diffopt) < 0)\n \t\t\tdie(\"diff_setup_done failed\");\n \t}\ndiff --git a/diff.c b/diff.c\nindex a5fc56b..aeae1a3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2182,6 +2182,7 @@ int diff_setup_done(struct diff_options *options)\n \tif (options->quiet) {\n \t\toptions->output_format = DIFF_FORMAT_NO_OUTPUT;\n \t\toptions->exit_with_status = 1;\n+\t\toptions->recursive = 0;\n \t}\n \n \t/*\n\nBut maybe that doesn't work because some of the options need recursion\nturned on, even if --quiet is specified. I have to admit, there are a\nlot of code paths here and I'm not sure how all of them interact with\nthe recursive option. So perhaps it is better to just special case\noptions->quiet as you suggested.\n\n-Peff\n"},{"id":"48985","messageId":"Pine.LNX.4.64.0707291323150.14781@racer.site","threadId":"9282","inReplyTo":"20070729121948.GA21983@coredump.intra.peff.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-29T12:24:20Z","receivedAt":"2007-07-29T12:24:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 29 Jul 2007, Jeff King wrote:\n\n> On Sun, Jul 29, 2007 at 01:04:13PM +0100, Johannes Schindelin wrote:\n> \n> > The idea is this: when \"--quiet\" was given, we do not output anything, and \n> > therefore do not have to recurse into the directories, because we already \n> > know that there are differences when a _tree_ is different. I do not \n> > remember all details of the \"--quiet\" implementation, but I think that it\n> > \n> > - exits early (as you said)\n> > \n> > - does not turn on \"recursive\" to avoid unnecessary work.\n> \n> OK, looking through the code, this works _sometimes_. If I say \"git-diff\n> --quiet\" then it will not recurse. If I say \"git-diff -p --quiet\" then\n> it will (even though we never show the -p output).\n\nThat is expected behaviour. If you ask for it, it should be done (however \nsilly it might be).\n\n> Since --quiet supersedes all output formats, I think it probably should \n> just clear the recursive option entirely. In which case rather than \n> special-casing quiet to avoid recursion in git-diff, we can simply turn \n> on recursion before parsing options (and it will get turned off \n> correctly by any diff options that need to do so).\n\nNo. Just think about \"git diff -p -w --quiet\". In some cases it _has_ to \nrecurse.\n\nCiao,\nDscho\n"},{"id":"48986","messageId":"20070729122614.GA23698@coredump.intra.peff.net","threadId":"9282","inReplyTo":"Pine.LNX.4.64.0707291323150.14781@racer.site","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-07-29T12:26:14Z","receivedAt":"2007-07-29T12:26:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 29, 2007 at 01:24:20PM +0100, Johannes Schindelin wrote:\n\n> No. Just think about \"git diff -p -w --quiet\". In some cases it _has_ to \n> recurse.\n\nOK, I'm convinced. It should be as you originally suggested.\n\n-Peff\n"},{"id":"48997","messageId":"alpine.LFD.0.999.0707290949150.3442@woody.linux-foundation.org","threadId":"9282","inReplyTo":"7vbqdvolww.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Documentation/git-diff: remove -r from --name-status example","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-29T16:51:17Z","receivedAt":"2007-07-29T16:51:17Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 28 Jul 2007, Junio C Hamano wrote:\n> \n> However, I think Jeff's patch always makes it recursive even\n> when the user asks for --raw, which makes it inappropriate for\n> inclusion whether before or after 1.5.3.\n\nWell, that was kind of the point. Yes, it makrs it recursive even for \n--raw, but the fact is, \"git diff\" is _always_ recursive (even for --raw) \nfor most common usage: anything that involves the index.\n\nSo \"git diff\" is fundamentally different from \"git log\" and \"git show\" and \nfriends. \"git diff\" can work on the files and index, which \"git log\" and \n\"git show\" never do.\n\n\t\tLinus\n"}]}