Web lists-archives.com

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




Denton Liu <liu.denton@xxxxxxxxx> 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?

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

> @@ -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:

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