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

[Junio C Hamano]

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

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

[Denton Liu]

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

Signed-off-by: Denton Liu 
---
 builtin/commit.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29e..a74a324b7a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -659,6 +659,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
const char *hook_arg2 = NULL;
int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
int old_display_comment_prefix;
+   int merge_contains_scissors = 0;
 
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
@@ -719,6 +720,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
strbuf_addbuf(, );
hook_arg1 = "message";
} else if (!stat(git_path_merge_msg(the_repository), )) {
+   size_t merge_msg_start;
+
/*
 * prepend SQUASH_MSG here if it exists and a
 * "merge --squash" was originally performed
@@ -729,8 +732,14 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
hook_arg1 = "squash";
} else
hook_arg1 = "merge";
+
+   merge_msg_start = sb.len;
if (strbuf_read_file(, git_path_merge_msg(the_repository), 
0) < 0)
die_errno(_("could not read MERGE_MSG"));
+
+   if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
+   wt_status_locate_end(sb.buf + merge_msg_start, sb.len - 
merge_msg_start) < sb.len - merge_msg_start)
+   merge_contains_scissors = 1;
} else if (!stat(git_path_squash_msg(the_repository), )) {
if (strbuf_read_file(, git_path_squash_msg(the_repository), 
0) < 0)
die_errno(_("could not read SQUASH_MSG"));
@@ -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
@@ -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,
-- 
2.19.1