Re: [PATCH] diff: ensure consistent diff behavior with -I<regex> across output formats

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

Jeff King <peff@peff.net> writes:

> We already have the diff_from_contents flag which is used for
> --exit-code. We should be able to see where that logic is applied and do
> something similar. It looks like it happens in diff_flush(), which makes
> sense:
>
>           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);
>                           if (options->found_changes)
>                                   break;
>                   }
>           }
>
> So here's a naive application of the same technique:
>
> diff --git a/diff.c b/diff.c
> index 76291e238c..0fe6eb7443 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6845,8 +6845,28 @@ 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))
> -				flush_one_pair(p, options);
> +
> +			if (!check_pair_status(p))
> +				continue;
> +
> +			if (options->flags.diff_from_contents) {
> +				FILE *orig_out = options->file;
> +				int orig_changes = options->found_changes;
> +				int skip;
> +
> +				options->file = xfopen("/dev/null", "w");
> +				diff_flush_patch(p, options);
> +				skip = !options->found_changes;
> +
> +				fclose(options->file);
> +				options->file = orig_out;
> +				options->found_changes = orig_changes;
> +
> +				if (skip)
> +					continue;
> +			}
> +
> +			flush_one_pair(p, options);
>  		}
>  		separator++;
>  	}
>
> which works on a trivial example. It affects all of raw, name-only,
> name-status, and checkdiff. I know Junio said that --raw should not be
> affected, but I'm not sure I agree. Anyway, it should be possible to
> split the logic by output type.

This is much more palatable to avoid code duplication and
repetition, compared to what was posted before.  A new code that
loops over q->queue[] one more time (which is yucky but cannot be
helped due to output order as you noted earlier) and runs
diff_flush_patch(), like the one we see above, is probably the best
we can do.  The beauty of the above approach is that the new code
only needs to know about a single .diff_from_contents bit, and does
not have to care what features cause the .diff_from_contents bit to
be flipped on.  Doing anything more would be maintenance nightmare.

Thanks.