Re: [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v
From: Junio C Hamano <hidden>
Date: 2024-02-26 18:03:25
Possibly related (same subject, not in this thread)
- 2024-02-27 · Re: [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v · Junio C Hamano <hidden>
- 2024-02-26 · [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v · Josh Triplett <josh@joshtriplett.org>
Josh Triplett [off-list ref] writes:
`git commit --cleanup=scissors -v` currently prints two scissors lines: one at the start of the comment lines, and the other right before the diff. This is redundant, and pushes the diff further down in the user's editor than it needs to be.
Interesting discovery.
quoted hunk
diff --git a/wt-status.c b/wt-status.c index b5a29083df..459d399baa 100644 --- a/wt-status.c +++ b/wt-status.c@@ -1143,11 +1143,13 @@ static void wt_longstatus_print_verbose(struct wt_status *s) * file (and even the "auto" setting won't work, since it * will have checked isatty on stdout). But we then do want * to insert the scissor line here to reliably remove the - * diff before committing. + * diff before committing, if we didn't already include one + * before. */ if (s->fp != stdout) { rev.diffopt.use_color = 0; - wt_status_add_cut_line(s->fp); + if (s->cleanup_mode != COMMIT_MSG_CLEANUP_SCISSORS) + wt_status_add_cut_line(s->fp); }
The machinery to populate the log message buffer should ideally be taught to remember if it already has added a scissors-line and to refrain from adding redundant ones. That way, we do not have to rely on the order of places that make wt_status_add_cut_line() calls or what condition they use to decide to make these calls. This hunk for example knows not just this one produces cut-line after the other one potentially added one, but also the logic used by the other one to decide to add one, which is even worse. I find the solution presented here a bit unsatisfactory, for this reason, but for now it may be OK, as we probably are not adding any more places and conditions to emit a scissors line.
builtin/commit.c | 2 ++ sequencer.h | 7 ------- wt-status.c | 6 ++++-- wt-status.h | 8 ++++++++ 4 files changed, 14 insertions(+), 9 deletions(-)
If this change did not break any existing tests that checked the combination of options and output when they are used together, it means we have a gap in the test coverage. We needs a test or two to protect this fix from future breakages. Thanks.