Re: [PATCH] commit: Avoid redundant scissor line with --cleanup=scissors -v

[Josh Triplett <josh@joshtriplett.org>]

On Mon, Feb 26, 2024 at 10:03:22AM -0800, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> 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.