format-patch: pre-2.20 range-diff regression fix
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest2 patches 14 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/2 | format-patch: add a more exhaustive --range-diff testt/ | +22 | 2 replies | |
| 2/2 | format-patch: don't include --stat with --range-diff outputbuiltin/ t/ | +6 | 4 replies | |
| 2/2 | format-patch: allow for independent diff & range-diff optionsDocumentation/ builtin/ t/ | +82 | 20 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH 0/2] format-patch: pre-2.20 range-diff regression fix
[Dropping LKML & git-packagers from CC]
Quoted 23 lines · Eric Sunshine
Quoted 4 lines · Ævar Arnfjörð Bjarmason
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/[email protected]/Umf. Unfortunate fallout from [1].
builtin/log.c static void make_cover_letter(struct rev_info *rev, int use_stdout, · line 1102
Quoted 11 lines
builtin/log.c +3
-0@@ -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:
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 9 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.
Quoted 3 lines
[...]
[1]: https://public-inbox.org/git/[email protected]/
[2]: https://public-inbox.org/git/[email protected]/
Æ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(-)
Signature · 1 line
2.20.0.rc0.387.gc7a69e6b6c