[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 > <ava...@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-ava...@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. > 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-ava...@gmail.com/ > [2]: https://public-inbox.org/git/20181113185558.23438-3-ava...@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
