[PATCH 0/2] format-patch: pre-2.20 range-diff regression fix
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 22, 2018, 21:12 UTC
- Message-ID
- <20181122211248.24546-1-avarab@gmail.com>
- In-Reply-To
- <CAPig+cSzyT5N5=YeX+VgRq1t0VbWqXLHSB=g=V=O-nLdCWrE9g@mail.gmail.com>
[Dropping LKML & git-packagers from CC]
On Thu, Nov 22 2018, Eric Sunshine wrote:
Show 28 quoted lines
> On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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/20181122132823.9883-1-avarab@gmail.com/
>
> 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.
Show 9 quoted lines
> 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/20181113185558.23438-4-avarab@gmail.com/ > [2]: https://public-inbox.org/git/20181113185558.23438-3-avarab@gmail.com/
Æ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(-)
-- 2.20.0.rc0.387.gc7a69e6b6c