Re: [PATCH v2 1/2] commit: don't add scissors line if one exists in MERGE_MSG

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 17, 2018, 08:06 UTC
Message-ID: <xmqq36s0i0vg.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1698fe0d7b7356ea1762f410767dcaf2807ea5c2.1542380865.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

> If commit.cleanup = scissors is specified, don't produce a scissors line
> if one already exists in the MERGE_MSG file.

Are we already sometimes adding a scissors line in that file? The impression I was getting was that the change in the step 2/2 is the only reason why this becomes necessary. In which case this change makes no sense as a standalone patch and requires 2/2 to be a sensible change, no?

Show 10 quoted lines

> @@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		struct ident_split ci, ai;
>  
>  		if (whence != FROM_COMMIT) {
> -			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> +			if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> +				!merge_contains_scissors)
>  				wt_status_add_cut_line(s->fp);
>  			status_printf_ln(s, GIT_COLOR_NORMAL,
>  			    whence == FROM_MERGE

This one is done before we show a block of text, which looks correct.

Show 10 quoted lines

> @@ -824,7 +834,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				  " Lines starting\nwith '%c' will be ignored, and an empty"
>  				  " message aborts the commit.\n"), comment_line_char);
>  		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> -			 whence == FROM_COMMIT)
> +			 whence == FROM_COMMIT &&
> +			 !merge_contains_scissors)
>  			wt_status_add_cut_line(s->fp);
>  		else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
>  			status_printf(s, GIT_COLOR_NORMAL,

The correctness of this one in an if/elseif/else cascade is less clear. When "contains scissors" logic does not kick in, we just have a scissors line here (i.e. the original behaviour). Now when the new logic kicks in, we not just omit the (extra) scissors line, but also say "Please enter the commit message..." which is the message used with the "comment line char" mode of the clean-up.

I wonder if this is what you really meant to have instead:

Show 9 quoted lines

>  		else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> -			 whence == FROM_COMMIT)
> - 			wt_status_add_cut_line(s->fp);
> +			 whence == FROM_COMMIT) {
> +			 if (!merge_contains_scissors)
> +				wt_status_add_cut_line(s->fp);
> +		}
>  		else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
>  			status_printf(s, GIT_COLOR_NORMAL,

That is, the only behaviour change in "merge contains scissors" mode is to omit the cut line and nothing else.