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

From: Jeff King <peff@peff.net>
Date: Jun 15, 2012, 19:37 UTC
Message-ID: <20120615193724.GB26473@sigill.intra.peff.net>
In-Reply-To: <7vzk849zxg.fsf@alter.siamese.dyndns.org>

On Fri, Jun 15, 2012 at 11:45:47AM -0700, Junio C Hamano wrote:

Show 26 quoted lines

> I think the following may be a lot closer to the correct fix; I
> didn't test many combinations of options with it, though.
> 
>  diff-no-index.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f0b0010..ed74e27 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -172,7 +172,7 @@ void diff_no_index(struct rev_info *revs,
>  		   int argc, const char **argv,
>  		   int nongit, const char *prefix)
>  {
> -	int i;
> +	int i, result;
>  	int no_index = 0;
>  	unsigned options = 0;
>  
> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
>  	 * The return code for --no-index imitates diff(1):
>  	 * 0 = no changes, 1 = changes, else error
>  	 */
> -	exit(revs->diffopt.found_changes);
> +	result = !!diff_result_code(&revs->diffopt, 0);
> +	exit(result);

Hmm. This works because is checks HAS_CHANGES instead of found_changes. I was somewhat surprised that the former works at all, but it is because diffcore_std sets it if there is anything in diff_queued_diff. Which makes sense; found_changes seems to be only about communicating changes up from builtin_diff to the rest of the diff code; nobody should be reading it outside of there.

Which left me to wonder how we tell when two files are actually the same. The answer is that diffcore_skip_stat_unmatch actually loads and memcmps each pair, removing ones that are identical. And diff_no_index always sets skip_stat_unmatch, whether diff.autorefreshindex is on or not.

So the patch I posted elsewhere in the thread is not right; we do not need to do diff_flush_patch to actually compare, because the stat_unmatch code will have done everything we want (unless DIFF_FROM_CONTENTS really is set). And the bug is purely one of looking at the wrong output flag.

-Peff