{"thread":{"id":"43501","subject":"Re: [PATCH] revision: add --no-full-diff command line option","startedAt":"2006-12-20T06:01:02Z","lastAt":"2006-12-20T19:55:05Z","messageCount":6,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"297924","messageId":"20061220060102.GA540@coredump.intra.peff.net","threadId":"43501","inReplyTo":null,"subject":"[PATCH] add log.fulldiff config option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-12-20T06:01:02Z","receivedAt":"2006-12-20T06:01:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This continues to default to 'off'.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Tue, Dec 19, 2006 at 05:17:17PM -0800, Junio C Hamano wrote:\n> I typically do:\n> \n>   git log --full-diff -p -SCOLLISION\n> \n> The --full-diff option helps because it shows the diff for other\n> files (that do not have different number of substring COLLISION\n> in the pre and postimage) in the same commit as well.\n\nI use --full-diff all the time, so this should save some typing. I can't\nthink of a time when I wouldn't want it on, but if there is, we probably\nneed a --no-full-diff.\n\nAlso, should this instead be diff.fulldiff?\n\nAlso also, I was going to submit a patch to document --full-diff,\nbut I had a few questions. Should it go in diff-options? That makes some\nsense to me, but the parsing actually happens in setup_revisions.\nFurthermore, it seems to do the same thing as --pickaxe-all. Should we\ntry to combine these?\n\n Documentation/config.txt |    6 ++++++\n builtin-log.c            |    6 ++++++\n 2 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 22482d6..8be1fa6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -248,6 +248,12 @@ i18n.commitEncoding::\n \tbrowser (and possibly at other places in the future or in other\n \tporcelains). See e.g. gitlink:git-mailinfo[1]. Defaults to 'utf-8'.\n \n+log.fulldiff::\n+\tIf true, log entries which have been selected through path-limiting\n+\tor pickaxe will show the diff for all files changed in that commit\n+\tinstead of only the files matching the specified path or pickaxe\n+\tstring.\n+\n log.showroot::\n \tIf true, the initial commit will be shown as a big creation event.\n \tThis is equivalent to a diff against an empty tree.\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 17014f7..57edd12 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -15,6 +15,7 @@\n #include <sys/time.h>\n \n static int default_show_root = 1;\n+static int default_full_diff = 0;\n \n /* this is in builtin-diff.c */\n void add_head(struct rev_info *revs);\n@@ -26,6 +27,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \trev->commit_format = CMIT_FMT_DEFAULT;\n \trev->verbose_header = 1;\n \trev->show_root_diff = default_show_root;\n+\trev->full_diff = default_full_diff;\n \targc = setup_revisions(argc, argv, rev, \"HEAD\");\n \tif (rev->diffopt.pickaxe || rev->diffopt.filter)\n \t\trev->always_show_header = 0;\n@@ -54,6 +56,10 @@ static int git_log_config(const char *var, const char *value)\n \t\tdefault_show_root = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"log.fulldiff\")) {\n+\t\tdefault_full_diff = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(var, value);\n }\n \n-- \n"},{"id":"295529","messageId":"7v8xh3j86h.fsf@assigned-by-dhcp.cox.net","threadId":"43501","inReplyTo":"20061220060102.GA540@coredump.intra.peff.net","subject":"Re: [PATCH] add log.fulldiff config option","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-20T08:14:14Z","receivedAt":"2006-12-20T08:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I use --full-diff all the time, so this should save some typing. I can't\n> think of a time when I wouldn't want it on, but if there is, we probably\n> need a --no-full-diff.\n\nAbsolutely.\n\n> Also, should this instead be diff.fulldiff?\n\nProbably not.\n\n> Also also, I was going to submit a patch to document --full-diff,\n> but I had a few questions. Should it go in diff-options? That makes some\n> sense to me, but the parsing actually happens in setup_revisions.\n\nNot at all.\n\n> Furthermore, it seems to do the same thing as --pickaxe-all. Should we\n> try to combine these?\n\nNo.\n\nTry this with various combinations of with and without\nthe two flags.\n\n    $ git log -1 -Sstate --full-diff --pickaxe-all b3d9899 -- git-fetch.sh\n\nYour understanding needs to be clarified on how paths are\nfiltered, and how log family and diff interacts, and probably\nthese need to be better documented.\n\nFirst, lets forget about 'log', but talk about diff (any of\ndiff-tree, diff-index --cached, diff-index, and diff-files).\nThey take paths as arguments.  The arguments limit the input\nside of the diff.\n\n    Side note: that is why \"diff -C arch/x86_64\" would not\n    care about renames or copies from arch/i386 to\n    arch/x86_64.  The diff machinery does not even see files\n    under arch/i386 with that command line.\n\nThe pickaxe (-S) is defined at the 'diff' layer, and works as\nthe output filter from the 'diff' machinery.  It omits the\nfilepair (pair of preimage and postimage) whose both sides have\nthe same number of specified string in them (\"pickaxe\ncriteria\").\n\n    $ git-diff-tree --name-status b3d9899\n\nwill show three paths, git-fetch.sh, git-ls-remote.sh, and\npkt-line.c.  But only git-fetch.sh matches the pickaxe criteria.\nThat's why:\n\n    $ git-diff-tree -p -Sstate b3d9899\n\nshows only git-fetch.sh and omits the other two paths.\n\nThe option --pickaxe-all changes this paths filtering to\nall-or-none.  If any of the incoming paths matches the pickaxe\ncriteria, all changed paths, even the ones that do not match the\ncriteria, are included in the output.  Thus,\n\n    $ git-diff-tree -p -Sstate --pickaxe-all b3d9899\n\ngives three paths, although the other two paths do not change\nthe number of string \"state\".\n\nWith me so far?\n\nNow, when you give paths to commands of the log family, they are\nused to filter commits considered for the output by the revision\ntraversal machinery.  The revision machinery feeds only commits\nwhose tree is different from its parents at the specified paths\nto the commands in the log family.  b3d9899 is eligible for\noutput in the first example because it changes \"git-fetch.sh\".\n\nThen the commit is fed to the tree-diff machinery, with the same\npaths limiter given to the log command.  If the diff machinery\nsays there is no interesting diff, the commit is not even\noutput.\n\n    Side note: that is why\n\n        $ git log -p -1 -Snosuchstring b3d9899 -- git-fetch.sh\n\n    does not say anything.  Pickaxe says git-fetch.sh does not\n    change \"nosuchstring\" and nothing comes out of the diff\n    machinery.\n\nWhat --full-diff does is to dissociate the paths limiter from\nthe diff machinery.  Before talking about pickaxe interaction,\nlet's try one without it.\n\nCompare:\n\n    $ git log -p -1             b3d9899 -- git-fetch.sh\n    $ git log -p -1 --full-diff b3d9899 -- git-fetch.sh\n\nIn both, git-fetch.sh is first used by the revision traversal to\ndetermine that b3d9899 is worth considering (the commit changes\nthat path).  Then the former uses the same paths limiter to\ncompute the diff for the commit.  Hence you will see only\ngit-fetch.sh.  The latter, however, does not use git-fetch.sh as\nthe paths limiter to generate diff because of --full-diff; it\nfeeds the full trees and that is why you can see three files.\n\nFinally.\n\nHow do all of the above interact with the pickaxe?\n\n    $ git log -p -1 -Sstate b3d9899 -- git-fetch.sh\n\nBecause git-fetch.sh is changed by b3d9899, revision traversal\nmakes the commit eligible.  It feeds that single path to diff\nmachinery and pickaxe says it changes \"state\".  Then it outputs\nthat path.\n\nHow about this one?\n\n    $ git log -p -1 -Sstate --full-diff b3d9899 -- git-fetch.sh\n\ngit-fetch.sh makes b3d9899 eligible, --full-diff makes all three\npaths touched by the commit (ignoring the paths you gave from\nthe command line) to be fed to the diff machinery, and pickaxe\npicks git-fetch.sh because that is the only one that changes\n\"state\".  For the same reason, you would see git-fetch.sh out of\nthis one:\n\n    $ git log -p -1 -Sstate --full-diff b3d9899 -- pkt-line.c\n\nTruly finally.\n\nIf you add --pickaxe-all to the last one:\n\n    $ git log -p -1 -Sstate --pickaxe-all --full-diff b3d9899 -- pkt-line.c\n\npkt-line.c makes b3d9899 eligible, --full-diff feeds three\nchanged paths to diff machinery, pickaxe notices that one of\nthem (git-fetch.sh which is different from what you gave from\nthe command line) changes \"state\", --pickaxe-all causes all\nthree incoming paths to be output.\n"},{"id":"298752","messageId":"20061220084107.GA6895@coredump.intra.peff.net","threadId":"43501","inReplyTo":"7v8xh3j86h.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] revision: add --no-full-diff command line option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-12-20T08:41:08Z","receivedAt":"2006-12-20T08:41:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Wed, Dec 20, 2006 at 12:14:14AM -0800, Junio C Hamano wrote:\n\n> > I use --full-diff all the time, so this should save some typing. I can't\n> > think of a time when I wouldn't want it on, but if there is, we probably\n> > need a --no-full-diff.\n> \n> Absolutely.\n\nI took this to mean \"absolutely we need --no-full-diff.\" :)\n\nI note that --full-diff sets rev->diff = 1. The log.fulldiff config\noption does not, and nor does --no-full-diff unset it. However, I'm not\nsure it makes sense to set it. Doing \"git-log --full-diff\" outputs an\nextra line (separating the diff from the commit log) but since we\nhaven't told it any type of diff to output, the diff is blank. And if we\nhad told it a type, then that would have turned on rev->diff. So I don't\nsee a point in setting it.\n\n revision.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 56819f8..ac4dbf2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -955,6 +955,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch\n \t\t\t\trevs->full_diff = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--no-full-diff\")) {\n+\t\t\t\trevs->full_diff = 0;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!strcmp(arg, \"--full-history\")) {\n \t\t\t\trevs->simplify_history = 0;\n \t\t\t\tcontinue;\n-- \n1.4.4.2.gee7ba-dirty\n"},{"id":"298218","messageId":"20061220085812.GB6895@coredump.intra.peff.net","threadId":"43501","inReplyTo":"7v8xh3j86h.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] add log.fulldiff config option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-12-20T08:58:12Z","receivedAt":"2006-12-20T08:58:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 20, 2006 at 12:14:14AM -0800, Junio C Hamano wrote:\n\n> > Also, should this instead be diff.fulldiff?\n> \n> Probably not.\n\nBased on your explanation, I think that makes sense.\n\n> > Also also, I was going to submit a patch to document --full-diff,\n> > but I had a few questions. Should it go in diff-options? That makes some\n> > sense to me, but the parsing actually happens in setup_revisions.\n> \n> Not at all.\n\nThen where should it go? whatchanged and log have a great deal of\noverlap in terms of parsing, but they don't seem to share any\ndocumentation (or note the fact that git-whatchanged is basically just\ngit-log -p).\n\n> > Furthermore, it seems to do the same thing as --pickaxe-all. Should we\n> > try to combine these?\n> \n> No.\n\nThanks for the thorough explanation. I think this makes my config option\ndescription a little bit off, then, since it doesn't actually show all\nof the changed files for a commit in the case of pickaxe.\n\nWhat has me confused, then, is that you said this:\n\n> I typically do:\n> \n>         git log --full-diff -p -SCOLLISION\n> \n> The --full-diff option helps because it shows the diff for other\n> files (that do not have different number of substring COLLISION\n> in the pre and postimage) in the same commit as well.\n\nBut that isn't the case, AFAIK (e.g., the command line you mention shows\nonly changes to sha1_file.c in commit aac17941, but cache.h was also\nchanged). Did you mean --pickaxe-all?\n\n> Your understanding needs to be clarified on how paths are\n> filtered, and how log family and diff interacts, and probably\n> these need to be better documented.\n\nTo be honest, I'm not sure when one would really _want_ the flexibility\nthis provides. It might be more intuitive to git-{log,whatchanged} users\nto have a single option (config and command line) to set --full-diff and\n--pickaxe-all together. At the very least, I think I now want a\ndiff.pickaxeall config option.\n\n"},{"id":"296306","messageId":"7vr6uuga2o.fsf@assigned-by-dhcp.cox.net","threadId":"43501","inReplyTo":"7v8xh3j86h.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] add log.fulldiff config option","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-20T10:01:35Z","receivedAt":"2006-12-20T10:01:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Your understanding needs to be clarified on how paths are\n> filtered, and how log family and diff interacts, and probably\n> these need to be better documented.\n\nHaving said that, I realized that I was making the same mistake\nthat our existing documentation was criticized for.\n\nLet me first digress to address the documentation criticism.\nThe description on plumbing were (and still are) far more\nthorough than porcelain-ish, and the conjecture by the person\nwho criticized them was because developers were keen documenting\nthe real work (plumbing) soon after they finished coding than\nthe frill (porcelain-ish).\n\nI think the real reason for this is somewhat different.  At\nleast to me, git.git was about the plumbing, and I did not want\nto be in the Porcelain business at all [*1*].  Instead, I wanted\nto encourage healthy competition among Porcelains.  What was\nshipped with git.git was meant to be \"barely usable examples\".\nBut this vision has not materialized fully yet, fortunately\nbecause we did not end up splitting the userbase too early on.\nBut this was unfortunate for two reasons.  One of them is that\ndifferent Porcelains would have brought completely different\nways of thinking and workflows into the picture.\n\nBecause git.git was primarily about the plumbing, the main focus\nof the interface the plumbing offers was not about integration\nand uniformity but modularity.  Designing them as independent\nbuilding blocks as much as practical, and describing them fully,\nwere part of completing the plumbing.  And the \"barely usable\nexamples\" started their lives as a set of trivial pipelines,\nwhose meaning was quite obvious and straightforward when you\nunderstand the plumbing parts used in them.  It also had UI\nwarts.  \"barely usable examples\" cheated their command line\nparsing, and the flags used by the plumbing showed through its\nthin veneer to the UI used by the end users.\n\nBut that was a long time ago.  Now we are in the v1.5.0 process,\nwhose theme is \"usability and teachability\".\n\nNow, viewing in that light, what I described in the previous\nmessage may have been a precise description of how things happen\nto work currently, but it was not about the reason how and why\nthey were designed to be (because there was little design at the\nUI level).  We should instead (or at least \"in addition\") be\ntalking more about how things ought to be.\n\nI described why --full-diff and --pickaxe-all are different\noptions, you can give 4 combinations of them, and how the\ncombinations work differently and why.  That does not justify\nthat all of these four combinations make sense.  In fact, I do\nnot think they do.\n\nWhen pickaxe is used and --full-diff is given to 'log', I think\nit does not make any sense not to use --pickaxe-all.  So, I\nthink at the level of log family commands, unifying these two\nflags make a lot of sense (although I earlier said \"no\").\n\nAlthough --diff-filter does not have a corresponding option that\nmakes it behave all-or-none (i.e. an equivalent to --pickaxe-all),\nit is conceivable that you might want to say:\n\n    $ git log --diff-filter=A --full-diff --diff-filter-all\n\nto view commits that add new files with full diffs (not just\nadded paths).  If we were to add that, I think it makes sense to\ninternally have a flag separate from pickaxe-all to express it\nbut when 'log' uses --full-diff, it does not make any sense not\nto enable it.  In other words,\n\n    $ git log -M --diff-filter=R -Sstring --full-diff\n\nshould internally enable pickaxe-all and diff-filter-all and\nshow all of diff that has renames in the paths that change\n\"string\" [*2*].\n\n\n[Footnote]\n\n*1* I once wanted to do a Porcelain of my own, after finding it\ndistasteful that Cogito tried to hide the index back when it did\nnot even use the index properly for merges.  But things have\nbeen improved quite a lot in Cogito land and I do not have\nanything against it anymore.  And as you all know, I like StGIT\nvery much.\n\n*2* This is actually in line with what we did in 582af688.  As a\nmember of 'log' family, 'git log' were originally meant to show\ncommit messages for all commits that revision traversal gave it,\nregardless of the diff output (and without the diffcore\ntransformation, when revision traversal said a commit is\ninteresting, the commit always has some diff or a merge).  When\nused in conjunction with --pickaxe and --diff-filter, however,\nthis layering made the command much less useful.  We fixed the\nproblem by violating the layering and made the log command aware\nof the underlying diff machinery's output filter.  I think the\nsame would apply to the issue we are currently discussing.\n"},{"id":"295153","messageId":"7vmz5ie412.fsf@assigned-by-dhcp.cox.net","threadId":"43501","inReplyTo":"20061220084107.GA6895@coredump.intra.peff.net","subject":"Re: [PATCH] revision: add --no-full-diff command line option","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-20T19:55:05Z","receivedAt":"2006-12-20T19:55:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> On Wed, Dec 20, 2006 at 12:14:14AM -0800, Junio C Hamano wrote:\n>\n>> > I use --full-diff all the time, so this should save some typing. I can't\n>> > think of a time when I wouldn't want it on, but if there is, we probably\n>> > need a --no-full-diff.\n>> \n>> Absolutely.\n>\n> I took this to mean \"absolutely we need --no-full-diff.\" :)\n\nModulo s/\\.$/, if we were to do log.fulldiff/, yes.\n\n> I note that --full-diff sets rev->diff = 1. The log.fulldiff config\n> option does not, and nor does --no-full-diff unset it. However, I'm not\n> sure it makes sense to set it. Doing \"git-log --full-diff\" outputs an\n> extra line (separating the diff from the commit log) but since we\n> haven't told it any type of diff to output, the diff is blank. And if we\n> had told it a type, then that would have turned on rev->diff. So I don't\n> see a point in setting it.\n\nI need to think about this one.\n"}]}