Re: [PATCH v3] diff: ensure consistent diff behavior with ignore options

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

Lidong Yan <yldhome2d2@gmail.com> writes:

> +static int quick_consume(void *priv, char *line, unsigned long len)
> +{
> +	struct emit_callback *ecbdata = priv;
> +	struct diff_options *o = ecbdata->opt;
> +
> +	o->found_changes = 1;
> +	return 1;
> +}

"make DEVELOPER=YesPlease" would not be very happy, without

-static int quick_consume(void *priv, char *line, unsigned long len)
+static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED)

> +/* return 1 if any change is found; otherwise, return 0 */
> +static int diff_flush_patch_quiet(struct diff_filepair *p, struct diff_options *o)
> +{
> +	int diff_opt = o->diff_optimize;
> +	int found_changes = o->found_changes;
> +	int ret;
> +
> +	o->diff_optimize = DIFF_OPT_DRY_RUN;
> +	o->found_changes = 0;
> +	diff_flush_patch(p, o);
> +	ret = o->found_changes;
> +	o->diff_optimize = diff_opt;
> +	o->found_changes |= found_changes;
> +	return ret;
> +}




>  static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o,
>  			    struct diffstat_t *diffstat)
>  {
> @@ -6778,7 +6810,19 @@ void diff_flush(struct diff_options *options)
>  			     DIFF_FORMAT_CHECKDIFF)) {
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			if (check_pair_status(p))
> +			int need_flush = 1;
> +
> +			if (!check_pair_status(p))
> +				continue;
> +
> +			if (options->flags.diff_from_contents) {
> +				if (diff_flush_patch_quiet(p, options))
> +					need_flush = 1;
> +				else
> +					need_flush = 0;
> +			}
> +
> +			if (need_flush)
>  				flush_one_pair(p, options);
>  		}

I am having a hard time understanding the logic in this loop.  Is it
equivalent to the following loop, and ...

		for (i = 0; i < q->nr; i++) {
			struct diff_filepair *p = q->queue[i];

			if (!check_pair_status(p))
				continue;
			if (options->flags.diff_from_contents &&
			    !diff_flush_patch_quietly(p, options))
				continue; /* no changes */

			flush_one_pair(p, options);
		}

... if so, isn't the above rewrite easier to follow?

The idea is that we cannot do anything with DIFF_STATUS_UNKNOWN
pairs (hence we continue), and when we are filtering output by the
"has the pair changed in a way that the user cares about?" criteria,
we check with flush_patch_quietly, and if there is no change worth
talking about, we do not do anything with such a pair, either.

Only when these conditions are not met, we call flush_one_pair() to
show the name or name status or whatever.

>  		separator++;
> @@ -6831,19 +6875,10 @@ void diff_flush(struct diff_options *options)
>  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    options->flags.exit_with_status &&
>  	    options->flags.diff_from_contents) {
> -		/*
> -		 * run diff_flush_patch for the exit status. setting
> -		 * options->file to /dev/null should be safe, because we
> -		 * aren't supposed to produce any output anyway.
> -		 */
> -		diff_free_file(options);
> -		options->file = xfopen("/dev/null", "w");
> -		options->close_file = 1;
> -		options->color_moved = 0;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
>  			if (check_pair_status(p))
> -				diff_flush_patch(p, options);
> +				diff_flush_patch_quiet(p, options);
>  			if (options->found_changes)
>  				break;
>  		}

It is a natural consequence of having a helper that does the "quiet"
thing that we can now lose the /dev/null hack, which is very nice.

Overall, the changes are easy to follow and look good.

Thanks.