patchSupport diff.autorefreshindex=true in `git-diff --quiet'

When diff.autorefreshindex is true, if a file has merely been 'touched' (mtime changed, but contents unchanged), then `git-diff --quiet' will now return 0 (indicating no change) instead of 1, and also silently refresh the index.

Signed-off-by: Karl Chen <quarl@quarl.org>
---

In current git master:
   
  git init
  echo abc > file1
  git add file1
  git commit -m msg
   
  sleep 1; touch file1
   
  git diff --exit-code --quiet file1
  echo $?
      # 1 [I expected 0]
   
  git diff --exit-code file1
  echo $?
      # 0 [as expected]
   
  git diff --exit-code --quiet file1
  echo $?
      # 0 [the non-quiet diff refreshed the cache]

I think `git diff --quiet file1' should return 0 when file1 only differs by mtime. I got this to work by having diffcore_std() call diffcore_skip_stat_unmatch() even when --quiet is specified. (I'm not sure about interaction with the other options.)

 diff.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

Karl Chen <quarl@cs.berkeley.edu> writes:

My knee-jerk reaction is that I do not particularly like this, but I haven't thought things through. What does --exit-code do with or without the configuration variable?

    Junio> Karl Chen <quarl@cs.berkeley.edu> writes:
    >> When diff.autorefreshindex is true, if a file has merely
    >> been 'touched' (mtime changed, but contents unchanged),
    >> then `git-diff --quiet' will now return 0 (indicating no
    >> change) instead of 1, and also silently refresh the index.

    Junio> My knee-jerk reaction is that I do not particularly
    Junio> like this, but I haven't thought things through.  What
    Junio> does --exit-code do with or without the configuration
    Junio> variable?

git diff --exit-code silently refreshes the index and returns 0, as documented and as I expect. So I further expect "git diff --exit-code --quiet" to be have the same semantics as "git diff --exit-code >/dev/null".

What don't you like about this? Isn't this the point of diff.autorefreshindex ?

Karl Chen <quarl@cs.berkeley.edu> writes:

The point of autorefreshindex was actually to reduce newbie confusion, and the change to make it the default turned out to be a good thing for the Porcelain layer.

I however do not think we are hurting anybody with the change to make it ignore stat dirtiness; see the proposed commit log message in the attached patch for details of the reasoning behind this statement.

I have to still wonder about your implementation, though.

Why do you have "apply-filter" and only that one in effect?

Since you are skipping break/rename, "diff --quiet -M --diff-filter=D" will not do what the user intended to do (which is to pair up the renamed paths and see if there is anything truly deleted, among many paths that disappeared merely because they are renamed away).

Looking at your patch and thinking about the issue very much tempt me to suggest the attached patch which is at the other extreme of the spectrum.

-- >8 -- diff --quiet: make it synonym to --exit-code >/dev/null

The point of --quiet was to return the status as early as possible without doing any extra processing. Well behaved scripts, when they expect to run many diff operations inside, are supposed to run "update-index --refresh" upfront; we do not want them to pay the price of iterating over the index and comparing the contents to fix the stat dirtiness, and we avoided most of the processing in diffcore_std() when --quiet is in effect.

But scripts that adhere to the good practice won't have to pay any more price than the necessary lstat(2) that will report stat cleanliness.

More importantly, users who do ask for "--quiet -M --filter=D" (in order to notice only the deletion, not paths that disappeared only because they have been renamed away) deserve to get the result they asked for, even it means they have to pay the extra price; the alternative is to get a cheap early return that gives a result they did not ask for, which is much worse.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I also suspect that the check for FIND_COPIES_HARDER to decide if we
   call skip-stat-unmatch is an incorrect optimization, but that is a
   separate topic.  In short, these two gives different results:

    $ >foo
    $ git add foo
    $ touch foo
    $ git diff -C -C
    $ git diff -C

 diff.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

    Junio> Looking at your patch and thinking about the issue very
    Junio> much tempt me to suggest the attached patch which is at
    Junio> the other extreme of the spectrum.

I think it is the extreme at the same end of the spectrum and I like it. (I was tempted to do this but thought there was a reason those options were ignored by --quiet.) This is good because it makes --quiet do what one thinks it does, and what's documented, with no exceptions (i.e. equivalent to --exit-code > /dev/null).

If anyone actually used the old behavior of ignoring diff.autorefreshindex (as an optimization, because the script ran update-index --refresh outside a loop), there could be a --quick option, or they could use git-diff-files.

Karl Chen <quarl@cs.berkeley.edu> writes:

No, that actually is a wrong attitude.

If somebody's depending on the behaviour, it is this new behaviour of doing everything asked without shortcut that needs to become a new option.

What tempted me to suggest the simpler and potentially slower approach was my hunch that no sane people depended on the old behaviour.

    Junio> If somebody's depending on the behaviour, it is this
    Junio> new behaviour of doing everything asked without
    Junio> shortcut that needs to become a new option.

Do you agree that the old behavior is at odds with the documentation and expectations of new users like me, and inconsistent?

Karl Chen <quarl@cs.berkeley.edu> writes:

Yes, I was just trying to make you realize that it _could_ be the documentation that needs fixing.