On Mon, Feb 26, 2024 at 10:03:22AM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@xxxxxxxxxxxxxxxx> 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.
>
> > 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.
I could add statefulness to wt_status_add_cut_line instead, on the
assumption that it's the only thing that should be adding a cut line,
and having it not add the line if previously added. For instance, it
could accept a pointer to the full wt_status rather than just the fp,
and keep a boolean state there.
> > 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.
I did run the testsuite, and it passed. I can add a simple test easily
enough.
