[PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Karl Chen]

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(-)

diff --git a/diff.c b/diff.c
index 135dec4..986cec3 100644
--- a/diff.c
+++ b/diff.c
@@ -3386,22 +3386,24 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
 
 void diffcore_std(struct diff_options *options)
 {
-	if (DIFF_OPT_TST(options, QUIET))
-		return;
-
-	if (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))
-		diffcore_skip_stat_unmatch(options);
-	if (options->break_opt != -1)
-		diffcore_break(options->break_opt);
-	if (options->detect_rename)
-		diffcore_rename(options);
-	if (options->break_opt != -1)
-		diffcore_merge_broken();
-	if (options->pickaxe)
-		diffcore_pickaxe(options->pickaxe, options->pickaxe_opts);
-	if (options->orderfile)
-		diffcore_order(options->orderfile);
-	diff_resolve_rename_copy();
+	if (DIFF_OPT_TST(options, QUIET)) {
+		if (options->skip_stat_unmatch)
+			diffcore_skip_stat_unmatch(options);
+	} else {
+		if (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))
+			diffcore_skip_stat_unmatch(options);
+		if (options->break_opt != -1)
+			diffcore_break(options->break_opt);
+		if (options->detect_rename)
+			diffcore_rename(options);
+		if (options->break_opt != -1)
+			diffcore_merge_broken();
+		if (options->pickaxe)
+			diffcore_pickaxe(options->pickaxe, options->pickaxe_opts);
+		if (options->orderfile)
+			diffcore_order(options->orderfile);
+		diff_resolve_rename_copy();
+	}
 	diffcore_apply_filter(options->filter);
 
 	if (diff_queued_diff.nr)
-- 
1.5.6.3

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Junio C Hamano]

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.

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?

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Karl Chen]

>>>>> On 2008-09-01 03:31 PDT, Junio C Hamano writes:

    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 ?

Re* [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Junio C Hamano]

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

>>>>>> On 2008-09-01 03:31 PDT, Junio C Hamano writes:
>
>     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 ?

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(-)

diff --git c/diff.c w/diff.c
index 7b4300a..5014a72 100644
--- c/diff.c
+++ w/diff.c
@@ -2392,13 +2392,6 @@ int diff_setup_done(struct diff_options *options)
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
-	/*
-	 * If we postprocess in diffcore, we cannot simply return
-	 * upon the first hit.  We need to run diff as usual.
-	 */
-	if (options->pickaxe || options->filter)
-		DIFF_OPT_CLR(options, QUIET);
-
 	return 0;
 }
 
@@ -3388,9 +3381,6 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
 
 void diffcore_std(struct diff_options *options)
 {
-	if (DIFF_OPT_TST(options, QUIET))
-		return;
-
 	if (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		diffcore_skip_stat_unmatch(options);
 	if (options->break_opt != -1)

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Karl Chen]

>>>>> On 2008-09-01 23:20 PDT, Junio C Hamano writes:

    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.

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Junio C Hamano]

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

> 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.

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.

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Karl Chen]

>>>>> On 2008-09-02 10:47 PDT, Junio C Hamano writes:

    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?

Re: [PATCH] Support diff.autorefreshindex=true in `git-diff --quiet'

[Junio C Hamano]

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

>>>>>> On 2008-09-02 10:47 PDT, Junio C Hamano writes:
>
>     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?

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