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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2024, 18:03 UTC
Message-ID: <xmqqbk83nlw5.fsf@gitster.g>
In-Reply-To: <9c09cea2679e14258720ee63e932e3b9459dbd8c.1708921369.git.josh@joshtriplett.org>

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.

Show 18 quoted lines

> 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.

Show 5 quoted lines

>  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.