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

From: Jeff King <peff@peff.net>
Date: Jun 15, 2012, 18:40 UTC
Message-ID: <20120615184030.GC14843@sigill.intra.peff.net>
In-Reply-To: <1339781463-13536-1-git-send-email-tim.henigan@gmail.com>

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.

Show 5 quoted lines

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

Show 17 quoted lines

> 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