Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Thu, Nov 08 2018, Eric Sunshine wrote:
> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
> > than introducing this new conditional, I'm thinking that a more
> > correct fix would be:
> >
> >     opts.output_format |= DIFF_FORMAT_PATCH;
> >
> > (note the '|=' operator). This would result in 'opts.output_format'
> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
> > prior to 73a834e9e2 when --no-patch was specified.
>
> Maybe I'm misunderstanding, but if you mean this on top:
>
>     -               if (!opts.output_format)
>     -                       opts.output_format = DIFF_FORMAT_PATCH;
>     +               opts.output_format |= DIFF_FORMAT_PATCH;

That is indeed what I mean.

> Then the --stat test I've added here fails, because unlike "diff" the
> "--stat" (and others) will implicitly "--patch" and you need
> "--no-patch" as well (again, unlike with "diff").

This new --stat test also fails with Dscho's original git-range-diff
implementation, even before 73a834e9e2 regressed the --no-patch case.
The commit message seems to sell this patch as a pure regression-fix,
so it feels wrong for it to be conflating a regression fix
(--no-patch) with preparation for potential future improvements to
other options (--stat, etc.).

I could see this as a two-patch series in which patch 1/2 fixes the
regression (with, say, "|="), and patch 2/2 generalizes setting
'opts.output_format' for the future. Alternately, if left as a single
patch, perhaps the commit message could be fleshed out to better
explain that it is doing more than merely fixing a regression (since
it wasn't at all obvious to me, even after digging into the code
earlier to come up with "|=", or now when trying to understand your
response).

> Right now --stat is pretty useless, but it could be made to make sense,
> and at that point (and earlier) I think it would be confusing if
> "range-diff" had different semantics with no options v.s. one option
> like "--stat" v.s. "--stat -p" compared to "diff".

Perhaps this sort of rationale, along with some explanatory examples
could be added to the commit message to help the reader more fully
understand the situation.

Thanks for working on this.