Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Jeff King <peff@peff.net>]

On Fri, Jun 15, 2012 at 01:31:03PM -0400, Tim Henigan wrote:

> When running 'git diff --quiet <file1> <file2>', if file1 or file2
> is outside the repository, it will exit(0) even if the files differ.
> It should exit(1) when they differ.

>From your description, I would expect the fix to be in builtin/diff.c,
or in diff-no-index.c, since that is where the code paths diverge.

> This was the least invasive fix that I found.  I considered adding
> the following when the '--quiet' option is parsed instead:
> 
> +   DIFF_OPT_SET(options, EXIT_WITH_STATUS)
> +   DIFF_OPT_SET(options, DIFF_FROM_CONTENTS)

We already set EXIT_WITH_STATUS when we see --quiet (we just do it a
little later, during diff_setup_done). We would not want to set
DIFF_FROM_CONTENTS all the time with --quiet. The point of that flag is
"we cannot know just from seeing the path sha1s whether they are
different or not, because we are doing content-level munging" (for
example, things like ignoring whitespace changes).

So we would not want to always set it whenever --quiet is given, because
it means we must do a lot of extra work comparing file content.

> diff --git a/diff.c b/diff.c
> index 77edd50..b1d74fe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4432,9 +4432,10 @@ void diff_flush(struct diff_options *options)
>  		separator++;
>  	}
>  
> -	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> +	if ((output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
> -	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
> +	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) ||
> +		DIFF_OPT_TST(options, QUICK)) {
>  		/*
>  		 * run diff_flush_patch for the exit status. setting
>  		 * options->file to /dev/null should be safe, becaue we

And this is equally bad, because it means that --quiet gets much slower
for _all_ cases, not just the no-index case.

I suspect what you actually want is to set DIFF_FROM_CONTENTS in the
no-index case, since we by definition do not have a pair of sha1s to
compare. But it may also be that diff.c could detect this case
automatically. I'd have to look closer.

-Peff