{"thread":{"id":"15314","subject":"[PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","startedAt":"2008-09-01T08:29:27Z","lastAt":"2008-09-02T20:19:27Z","messageCount":8,"participants":["Karl Chen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89374","messageId":"quack.20080901T0129.lth8wuci80o@roar.cs.berkeley.edu","threadId":"15314","inReplyTo":null,"subject":"[PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-09-01T08:29:27Z","receivedAt":"2008-09-01T08:29:27Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":"\nWhen diff.autorefreshindex is true, if a file has merely been 'touched' (mtime\nchanged, but contents unchanged), then `git-diff --quiet' will now return 0\n(indicating no change) instead of 1, and also silently refresh the index.\n\nSigned-off-by: Karl Chen <quarl@quarl.org>\n---\n\nIn current git master:\n   \n  git init\n  echo abc > file1\n  git add file1\n  git commit -m msg\n   \n  sleep 1; touch file1\n   \n  git diff --exit-code --quiet file1\n  echo $?\n      # 1 [I expected 0]\n   \n  git diff --exit-code file1\n  echo $?\n      # 0 [as expected]\n   \n  git diff --exit-code --quiet file1\n  echo $?\n      # 0 [the non-quiet diff refreshed the cache]\n\nI think `git diff --quiet file1' should return 0 when file1 only\ndiffers by mtime.  I got this to work by having diffcore_std()\ncall diffcore_skip_stat_unmatch() even when --quiet is specified.\n(I'm not sure about interaction with the other options.)\n\n\n diff.c |   34 ++++++++++++++++++----------------\n 1 files changed, 18 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 135dec4..986cec3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3386,22 +3386,24 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (DIFF_OPT_TST(options, QUIET))\n-\t\treturn;\n-\n-\tif (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))\n-\t\tdiffcore_skip_stat_unmatch(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_break(options->break_opt);\n-\tif (options->detect_rename)\n-\t\tdiffcore_rename(options);\n-\tif (options->break_opt != -1)\n-\t\tdiffcore_merge_broken();\n-\tif (options->pickaxe)\n-\t\tdiffcore_pickaxe(options->pickaxe, options->pickaxe_opts);\n-\tif (options->orderfile)\n-\t\tdiffcore_order(options->orderfile);\n-\tdiff_resolve_rename_copy();\n+\tif (DIFF_OPT_TST(options, QUIET)) {\n+\t\tif (options->skip_stat_unmatch)\n+\t\t\tdiffcore_skip_stat_unmatch(options);\n+\t} else {\n+\t\tif (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))\n+\t\t\tdiffcore_skip_stat_unmatch(options);\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_break(options->break_opt);\n+\t\tif (options->detect_rename)\n+\t\t\tdiffcore_rename(options);\n+\t\tif (options->break_opt != -1)\n+\t\t\tdiffcore_merge_broken();\n+\t\tif (options->pickaxe)\n+\t\t\tdiffcore_pickaxe(options->pickaxe, options->pickaxe_opts);\n+\t\tif (options->orderfile)\n+\t\t\tdiffcore_order(options->orderfile);\n+\t\tdiff_resolve_rename_copy();\n+\t}\n \tdiffcore_apply_filter(options->filter);\n \n \tif (diff_queued_diff.nr)\n-- \n1.5.6.3\n"},{"id":"89397","messageId":"7vskskw41j.fsf@gitster.siamese.dyndns.org","threadId":"15314","inReplyTo":"quack.20080901T0129.lth8wuci80o@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-01T10:31:36Z","receivedAt":"2008-09-01T10:31:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen <quarl@cs.berkeley.edu> writes:\n\n> When diff.autorefreshindex is true, if a file has merely been 'touched'\n> (mtime changed, but contents unchanged), then `git-diff --quiet' will\n> now return 0 (indicating no change) instead of 1, and also silently\n> refresh the index.\n\nMy knee-jerk reaction is that I do not particularly like this, but I\nhaven't thought things through.  What does --exit-code do with or without\nthe configuration variable?\n"},{"id":"89404","messageId":"quack.20080901T0350.lthzlmsgmx6@roar.cs.berkeley.edu","threadId":"15314","inReplyTo":"7vskskw41j.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-09-01T10:50:29Z","receivedAt":"2008-09-01T10:50:29Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-09-01 03:31 PDT, Junio C Hamano writes:\n\n    Junio> Karl Chen <quarl@cs.berkeley.edu> writes:\n    >> When diff.autorefreshindex is true, if a file has merely\n    >> been 'touched' (mtime changed, but contents unchanged),\n    >> then `git-diff --quiet' will now return 0 (indicating no\n    >> change) instead of 1, and also silently refresh the index.\n\n    Junio> My knee-jerk reaction is that I do not particularly\n    Junio> like this, but I haven't thought things through.  What\n    Junio> does --exit-code do with or without the configuration\n    Junio> variable?\n\ngit diff --exit-code silently refreshes the index and returns 0,\nas documented and as I expect.  So I further expect \"git diff\n--exit-code --quiet\" to be have the same semantics as \"git diff\n--exit-code >/dev/null\".\n\nWhat don't you like about this?  Isn't this the point of\ndiff.autorefreshindex ?\n"},{"id":"89483","messageId":"7vy72bnk5x.fsf_-_@gitster.siamese.dyndns.org","threadId":"15314","inReplyTo":"quack.20080901T0350.lthzlmsgmx6@roar.cs.berkeley.edu","subject":"Re* [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T06:20:26Z","receivedAt":"2008-09-02T06:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen <quarl@cs.berkeley.edu> writes:\n\n>>>>>> On 2008-09-01 03:31 PDT, Junio C Hamano writes:\n>\n>     Junio> My knee-jerk reaction is that I do not particularly\n>     Junio> like this, but I haven't thought things through.  What\n>     Junio> does --exit-code do with or without the configuration\n>     Junio> variable?\n>\n> git diff --exit-code silently refreshes the index and returns 0,\n> as documented and as I expect.  So I further expect \"git diff\n> --exit-code --quiet\" to be have the same semantics as \"git diff\n> --exit-code >/dev/null\".\n>\n> What don't you like about this?  Isn't this the point of\n> diff.autorefreshindex ?\n\nThe point of autorefreshindex was actually to reduce newbie confusion, and\nthe change to make it the default turned out to be a good thing for the\nPorcelain layer.\n\nI however do not think we are hurting anybody with the change to make it\nignore stat dirtiness; see the proposed commit log message in the attached\npatch for details of the reasoning behind this statement.\n\nI have to still wonder about your implementation, though.\n\nWhy do you have \"apply-filter\" and only that one in effect?\n\nSince you are skipping break/rename, \"diff --quiet -M --diff-filter=D\"\nwill not do what the user intended to do (which is to pair up the renamed\npaths and see if there is anything truly deleted, among many paths that\ndisappeared merely because they are renamed away).\n\nLooking at your patch and thinking about the issue very much tempt me to\nsuggest the attached patch which is at the other extreme of the spectrum.\n\n-- >8 --\ndiff --quiet: make it synonym to --exit-code >/dev/null\n\nThe point of --quiet was to return the status as early as possible without\ndoing any extra processing.  Well behaved scripts, when they expect to run\nmany diff operations inside, are supposed to run \"update-index --refresh\"\nupfront; we do not want them to pay the price of iterating over the index\nand comparing the contents to fix the stat dirtiness, and we avoided most\nof the processing in diffcore_std() when --quiet is in effect.\n\nBut scripts that adhere to the good practice won't have to pay any more\nprice than the necessary lstat(2) that will report stat cleanliness.\n\nMore importantly, users who do ask for \"--quiet -M --filter=D\" (in order\nto notice only the deletion, not paths that disappeared only because they\nhave been renamed away) deserve to get the result they asked for, even it\nmeans they have to pay the extra price; the alternative is to get a cheap\nearly return that gives a result they did not ask for, which is much\nworse.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * I also suspect that the check for FIND_COPIES_HARDER to decide if we\n   call skip-stat-unmatch is an incorrect optimization, but that is a\n   separate topic.  In short, these two gives different results:\n\n    $ >foo\n    $ git add foo\n    $ touch foo\n    $ git diff -C -C\n    $ git diff -C\n\n diff.c |   10 ----------\n 1 files changed, 0 insertions(+), 10 deletions(-)\n\ndiff --git c/diff.c w/diff.c\nindex 7b4300a..5014a72 100644\n--- c/diff.c\n+++ w/diff.c\n@@ -2392,13 +2392,6 @@ int diff_setup_done(struct diff_options *options)\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n \n-\t/*\n-\t * If we postprocess in diffcore, we cannot simply return\n-\t * upon the first hit.  We need to run diff as usual.\n-\t */\n-\tif (options->pickaxe || options->filter)\n-\t\tDIFF_OPT_CLR(options, QUIET);\n-\n \treturn 0;\n }\n \n@@ -3388,9 +3381,6 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (DIFF_OPT_TST(options, QUIET))\n-\t\treturn;\n-\n \tif (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))\n \t\tdiffcore_skip_stat_unmatch(options);\n \tif (options->break_opt != -1)\n"},{"id":"89514","messageId":"quack.20080902T1039.lthabeqmopo_-_@roar.cs.berkeley.edu","threadId":"15314","inReplyTo":"7vy72bnk5x.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-09-02T17:39:47Z","receivedAt":"2008-09-02T17:39:47Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-09-01 23:20 PDT, Junio C Hamano writes:\n\n    Junio> Looking at your patch and thinking about the issue very\n    Junio> much tempt me to suggest the attached patch which is at\n    Junio> the other extreme of the spectrum.\n\nI think it is the extreme at the same end of the spectrum and I\nlike it.  (I was tempted to do this but thought there was a reason\nthose options were ignored by --quiet.)  This is good because it\nmakes --quiet do what one thinks it does, and what's documented,\nwith no exceptions (i.e. equivalent to --exit-code > /dev/null).\n\nIf anyone actually used the old behavior of ignoring\ndiff.autorefreshindex (as an optimization, because the script ran\nupdate-index --refresh outside a loop), there could be a --quick\noption, or they could use git-diff-files.\n"},{"id":"89516","messageId":"7v4p4ymocc.fsf@gitster.siamese.dyndns.org","threadId":"15314","inReplyTo":"quack.20080902T1039.lthabeqmopo_-_@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T17:47:47Z","receivedAt":"2008-09-02T17:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen <quarl@cs.berkeley.edu> writes:\n\n> If anyone actually used the old behavior of ignoring\n> diff.autorefreshindex (as an optimization, because the script ran\n> update-index --refresh outside a loop), there could be a --quick\n> option, or they could use git-diff-files.\n\nNo, that actually is a wrong attitude.\n\nIf somebody's depending on the behaviour, it is this new behaviour of\ndoing everything asked without shortcut that needs to become a new\noption.\n\nWhat tempted me to suggest the simpler and potentially slower approach was\nmy hunch that no sane people depended on the old behaviour.\n"},{"id":"89520","messageId":"quack.20080902T1059.lthsksil97w@roar.cs.berkeley.edu","threadId":"15314","inReplyTo":"7v4p4ymocc.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Karl Chen","fromEmail":"quarl@cs.berkeley.edu","sentAt":"2008-09-02T17:59:47Z","receivedAt":"2008-09-02T17:59:47Z","isPatch":true,"sender":{"key":"quarl@cs.berkeley.edu","avatar":null},"body":">>>>> On 2008-09-02 10:47 PDT, Junio C Hamano writes:\n\n    Junio> If somebody's depending on the behaviour, it is this\n    Junio> new behaviour of doing everything asked without\n    Junio> shortcut that needs to become a new option.\n\nDo you agree that the old behavior is at odds with the\ndocumentation and expectations of new users like me, and\ninconsistent?\n"},{"id":"89534","messageId":"7vwshujo6o.fsf@gitster.siamese.dyndns.org","threadId":"15314","inReplyTo":"quack.20080902T1059.lthsksil97w@roar.cs.berkeley.edu","subject":"Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-02T20:19:27Z","receivedAt":"2008-09-02T20:19:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karl Chen <quarl@cs.berkeley.edu> writes:\n\n>>>>>> On 2008-09-02 10:47 PDT, Junio C Hamano writes:\n>\n>     Junio> If somebody's depending on the behaviour, it is this\n>     Junio> new behaviour of doing everything asked without\n>     Junio> shortcut that needs to become a new option.\n>\n> Do you agree that the old behavior is at odds with the\n> documentation and expectations of new users like me, and\n> inconsistent?\n\nYes, I was just trying to make you realize that it _could_ be the\ndocumentation that needs fixing.\n"}]}