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

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

> I'm not sure if stuff like --stat would need something similar. It's
> already doing a content comparison, so presumably it handles it
> internally. Maybe stuff like --dirstat would need it, too? In which case
> we'd maybe want to annotate each filepair in an initial loop with
> whether it's modified at the content-level, and then take that into
> account in various code paths.
> 
> And of course it's horribly hacky looking. Some refactoring might help.
> Certainly it is silly to open /dev/null each time through the loop.
> There might also be a better way of checking whether the diff found
> anything than the found_changes flag.
> 
> So this is really just sketching out the direction, and somebody would
> need to figure out the details.

>> * Also, should we internally run diff twice, especially even when
>>   we are going to show the patch output and are not limited to
>>   FORMAT_NAME and FORMAT_NAME_STATUS?  Generally, running the real
>>   diff in any of the diffcore transformatin is a sign of trouble.
> 
> The patch above also runs the diff twice for "-Ifoo --name-only -p". But
> I think we are kind of stuck there. We want to show all name-only
> entries before any content diffs. So either we have to run the content
> diff twice, or we have to buffer it to show after we decide whether to
> show name-only entries.

> 
> The enthusiasm is appreciated, but the implementation raises two
> questions.
> 
> * This special cases -I<pattern>, but any option that causes us to
>   set the .diff_from_contents flag, not just -I<pattern>, can cause
>   the raw blob comparison to be potentially different from what the
>   blob contents are compared with various "ignore this class of
>   changes" criteria.  Shouldn't "git diff -w --name-status" and the
>   like get the same treatment?

> Also, the usual way to compose a log message of this project is to
> 
>     - Give an observation on how the current system works in the
>       present tense (so no need to say "Currently X is Y", or
>       "Previously X was Y" to describe the state before your change;
>       just "X is Y" is enough), and discuss what you perceive as a
>       problem in it.
> 
>     - Propose a solution (optional---often, problem description
>       trivially leads to an obvious solution in reader's minds).
> 
>     - Give commands to somebody editing the codebase to "make it so",
>       instead of saying "This commit does X".
> 
> in this order.