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

[Junio C Hamano <gitster@pobox.com>]

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)