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

[Junio C Hamano <gitster@pobox.com>]

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.

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