[0/2] format-patch: pre-2.20 range-diff regression fix

Message

Ævar Arnfjörð Bjarmason Nov. 22, 2018, 9:12 p.m. UTC

[Dropping LKML & git-packagers from CC]

On Thu, Nov 22 2018, Eric Sunshine wrote:

> On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason
> <[e-mail]> wrote:
>> There's a regression related to this that I wanted to send a headsup
>> for, but don't have time to fix today. Now range-diff in format-patch
>> includes --stat output. See e.g. my
>> https://public-inbox.org/git/[e-mail]/
>
> Umf. Unfortunate fallout from [1].
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>         if (rev->rdiff1) {
>> +               const int oldfmt = rev->diffopt.output_format;
>>                 fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> +               rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY);
>>                 show_range_diff(rev->rdiff1, rev->rdiff2,
>>                                 rev->creation_factor, 1, &rev->diffopt);
>> +               rev->diffopt.output_format = oldfmt;
>>         }
>>  }
>
> A few questions/observations:
>
> Does this same "fix" need to be applied to the --interdiff case just
> above this --range-diff block?
>
> Does the same "fix" need to be applied to the --interdiff and
> --range-diff cases in log-tree.c:show_log()?

No, that seems to do the right thing, but perhaps tests are lacking
there too. I haven't looked.

> Aside from fixing the broken --no-patches option[2], a goal of the
> series was to some day make --stat do something useful. Doesn't this
> "fix" go against that goal?

The goal was to fix the regression introduced in 73a834e9e2
("range-diff: relieve callers of low-level configuration burden",
2018-07-22). One aspect of having fixed that is we might in the future
do stuff with --stat.

> The way this change needs to be spread around at various locations is
> making it feel like a BandAid "fix" rather than a proper solution. It
> seems like it should be fixed at a different level, though I'm not
> sure yet if that level is higher (where the options get set) or lower
> (where they actually affect the operation).
>
> Until we figure out the answers to these questions, I wonder if a more
> sensible short-term solution would be to back out [1] and just keep
> [2], which fixed the --no-patches regression.

I think that patch leaves range-diff itself in a good state without
any bugs, and it would be a mistake to revert it.

But this interaction with format-patch --range-diff is another
matter. As noted in 2/2 I think in practice this series sweeps the
current bugs under the rug.

But as also noted there I think re-using what we get from
setup_revisions() in format-patch for the range-diff was a mistake,
and is always going to lead to some confusion. It should be split up
so we can supply those diff options independently.

> [...]
> [1]: https://public-inbox.org/git/[e-mail]/
> [2]: https://public-inbox.org/git/[e-mail]/

Ævar Arnfjörð Bjarmason (2):
  format-patch: add a more exhaustive --range-diff test
  format-patch: don't include --stat with --range-diff output

 builtin/log.c         |  7 ++++++-
 t/t3206-range-diff.sh | 15 ++++++++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)