[PATCH 0/2] cherry-pick/revert cleanup scissors fix

[PATCH 0/2] cherry-pick/revert cleanup scissors fix

[Denton Liu]

As I stated earlier[1], I discovered today that git revert (and thus,
git cherry-pick as well) do not place the scissors line properly as
well. This patchset adds a scissors line when conflicts arise in both
cherry-pick and revert, which fixes the same bug present in git-merge
earlier.

This patchset should apply on top of dl/merge-cleanup-scissors-fix.

[1]: https://public-inbox.org/git/20190306014143.GA2580@dev-l/

Denton Liu (2):
  t3507: cleanup space after redirection operators
  cherry-pick/revert: add scissors line on merge conflict

 Documentation/git-cherry-pick.txt |   7 ++
 Documentation/git-revert.txt      |   7 ++
 builtin/merge.c                   |  13 +---
 builtin/revert.c                  |   5 ++
 sequencer.c                       |  22 +++---
 sequencer.h                       |   3 +-
 t/t3507-cherry-pick-conflict.sh   | 122 +++++++++++++++++++++++++-----
 7 files changed, 138 insertions(+), 41 deletions(-)

-- 
2.19.1.6.g4de041ebd8.dirty

[PATCH 1/2] t3507: cleanup space after redirection operators

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 34 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0db166152a..74ff925526 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -88,7 +88,7 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
 
 test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -96,7 +96,7 @@ test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 test_expect_success \
 	'cherry-pick --strategy=resolve w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick --strategy=resolve base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -175,23 +175,23 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 		git ls-files --stage foo &&
 		git checkout picked -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected &&
+	" <stages >expected &&
 	git read-tree -u --reset HEAD &&
 
 	test_must_fail git cherry-pick picked &&
-	git ls-files --stage --unmerged > actual &&
+	git ls-files --stage --unmerged >actual &&
 
 	test_cmp expected actual
 '
 
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -201,14 +201,14 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| parent of objid picked
@@ -220,14 +220,14 @@ test_expect_success 'diff3 -m style' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'revert also handles conflicts sanely' '
 	git config --unset merge.conflictstyle &&
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -241,24 +241,24 @@ test_expect_success 'revert also handles conflicts sanely' '
 		git ls-files --stage foo &&
 		git checkout base -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected-stages &&
+	" <stages >expected-stages &&
 	git read-tree -u --reset HEAD &&
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git revert picked &&
 	newhead=$(git rev-parse HEAD) &&
-	git ls-files --stage --unmerged > actual-stages &&
+	git ls-files --stage --unmerged >actual-stages &&
 
 	test "$head" = "$newhead" &&
 	test_must_fail git update-index --refresh -q &&
 	test_must_fail git diff-index --exit-code HEAD &&
 	test_cmp expected-stages actual-stages &&
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
@@ -284,7 +284,7 @@ test_expect_success 'revert --no-commit sets REVERT_HEAD' '
 
 test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' '
 	pristine_detach base &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git revert base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
 	test_must_fail git rev-parse --verify REVERT_HEAD
@@ -319,7 +319,7 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '
 test_expect_success 'revert conflict, diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| objid picked
@@ -331,7 +331,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 
 	test_must_fail git revert picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
-- 
2.19.1.6.g4de041ebd8.dirty

[PATCH 2/2] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Fix a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
commit.cleanup = scissors.

Note that the removal of the if-else tower in git_sequencer_config may
appear to be a no-op refactor but it actually isn't. First of all, we
now accept "default" as a configuration option and also we die on an
invalid cleanup mode. Most importantly, though, if
commit.cleanup = scissors, the cleanup enum will be set to
COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This
allows us to append scissors to MERGE_MSG in the case of a conflict.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-cherry-pick.txt |  7 +++
 Documentation/git-revert.txt      |  7 +++
 builtin/merge.c                   | 13 ++---
 builtin/revert.c                  |  5 ++
 sequencer.c                       | 22 ++++----
 sequencer.h                       |  3 +-
 t/t3507-cherry-pick-conflict.sh   | 88 +++++++++++++++++++++++++++++++
 7 files changed, 121 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc8..5c086d78c8 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -57,6 +57,13 @@ OPTIONS
 	With this option, 'git cherry-pick' will let you edit the commit
 	message prior to committing.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be prepended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -x::
 	When recording the commit, append a line that says
 	"(cherry picked from commit ...)" to the original commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..1894010e60 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -66,6 +66,13 @@ more details.
 	With this option, 'git revert' will not start the commit
 	message editor.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be prepended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -n::
 --no-commit::
 	Usually the command automatically creates some commits with
diff --git a/builtin/merge.c b/builtin/merge.c
index 92efc3d8fa..d4217ebcf5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,17 +913,10 @@ static int suggest_conflicts(void)
 	 * We can't use cleanup_mode because if we're not using the editor,
 	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
 	 * though the message is meant to be processed later by git-commit.
-	 * Thus, we will get the cleanup mode is returned we _are_ using an
-	 * editor.
+	 * Thus, we will get the cleanup mode which is returned when we _are_ using
+	 * an editor.
 	 */
-	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
-	    fputc('\n', fp);
-	    wt_status_add_cut_line(fp);
-	    /* comments out the newline from append_conflicts_hint */
-	    fputc(comment_line_char, fp);
-	}
-
-	append_conflicts_hint(&msgbuf);
+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
 	fclose(fp);
diff --git a/builtin/revert.c b/builtin/revert.c
index 9a66720cfc..fe18036be7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
+	const char *cleanup_arg = NULL;
 	int cmd = 0;
 	struct option base_options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
+		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
 		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->keep_redundant_commits)
 		opts->allow_empty = 1;
 
+	if (cleanup_arg)
+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
+
 	/* Check for incompatible command line arguments */
 	if (cmd) {
 		char *this_operation;
diff --git a/sequencer.c b/sequencer.c
index 707e72fb39..85ad58555d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 		if (status)
 			return status;
 
-		if (!strcmp(s, "verbatim"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
-		else if (!strcmp(s, "whitespace"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else if (!strcmp(s, "strip"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
-		else if (!strcmp(s, "scissors"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else
-			warning(_("invalid commit message cleanup mode '%s'"),
-				  s);
+		opts->default_msg_cleanup = get_cleanup_mode(s, 1);
 
 		free((char *)s);
 		return status;
@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 }
 
-void append_conflicts_hint(struct strbuf *msgbuf)
+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
 {
 	int i;
 
+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+		strbuf_addch(msgbuf, '\n');
+		wt_status_append_cut_line(msgbuf);
+		strbuf_addch(msgbuf, comment_line_char);
+	}
+
 	strbuf_addch(msgbuf, '\n');
 	strbuf_commented_addf(msgbuf, "Conflicts:\n");
 	for (i = 0; i < active_nr;) {
@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 			_(action_name(opts)));
 
 	if (!clean)
-		append_conflicts_hint(msgbuf);
+		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
 
 	return !clean;
 }
diff --git a/sequencer.h b/sequencer.h
index 5690e0c27e..aa99503dd7 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -91,7 +91,8 @@ int rearrange_squash(void);
 extern const char sign_off_header[];
 
 void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
-void append_conflicts_hint(struct strbuf *msgbuf);
+void append_conflicts_hint(struct strbuf *msgbuf,
+		enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 	int use_editor);
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 74ff925526..c94e44dad0 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
+test_expect_success \
+	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick --cleanup=scissors picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
 	cat <<-EOF >expected &&
@@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit objid.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
+test_expect_success \
+	'revert conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit objid.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert --cleanup=scissors picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'failed cherry-pick does not forget -s' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
-- 
2.19.1.6.g4de041ebd8.dirty

Re: [PATCH 2/2] cherry-pick/revert: add scissors line on merge conflict

[Phillip Wood]

Hi Denton

On 06/03/2019 10:30, Denton Liu wrote:
> Fix a bug where the scissors line is placed after the Conflicts:
> section, in the case where a merge conflict occurs and
> commit.cleanup = scissors.

Was that the case with cherry-pick and revert as well as merge, or were 
they missing the scissors line entirely as the subject line suggests?

> Note that the removal of the if-else tower in git_sequencer_config may
> appear to be a no-op refactor but it actually isn't. First of all, we
> now accept "default" as a configuration option and also we die on an
> invalid cleanup mode. Most importantly, though, if
> commit.cleanup = scissors, the cleanup enum will be set to
> COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This
> allows us to append scissors to MERGE_MSG in the case of a conflict.

I've got some comments about this change below

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   Documentation/git-cherry-pick.txt |  7 +++
>   Documentation/git-revert.txt      |  7 +++
>   builtin/merge.c                   | 13 ++---
>   builtin/revert.c                  |  5 ++
>   sequencer.c                       | 22 ++++----
>   sequencer.h                       |  3 +-
>   t/t3507-cherry-pick-conflict.sh   | 88 +++++++++++++++++++++++++++++++
>   7 files changed, 121 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index d35d771fc8..5c086d78c8 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -57,6 +57,13 @@ OPTIONS
>   	With this option, 'git cherry-pick' will let you edit the commit
>   	message prior to committing.
>   
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be prepended to MERGE_MSG before being passed on in the case of a
> +	conflict.
> +
>   -x::
>   	When recording the commit, append a line that says
>   	"(cherry picked from commit ...)" to the original commit
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index 837707a8fd..1894010e60 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -66,6 +66,13 @@ more details.
>   	With this option, 'git revert' will not start the commit
>   	message editor.
>   
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be prepended to MERGE_MSG before being passed on in the case of a
> +	conflict.
> +
>   -n::
>   --no-commit::
>   	Usually the command automatically creates some commits with
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 92efc3d8fa..d4217ebcf5 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -913,17 +913,10 @@ static int suggest_conflicts(void)
>   	 * We can't use cleanup_mode because if we're not using the editor,
>   	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
>   	 * though the message is meant to be processed later by git-commit.
> -	 * Thus, we will get the cleanup mode is returned we _are_ using an
> -	 * editor.
> +	 * Thus, we will get the cleanup mode which is returned when we _are_ using
> +	 * an editor.
>   	 */
> -	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
> -	    fputc('\n', fp);
> -	    wt_status_add_cut_line(fp);
> -	    /* comments out the newline from append_conflicts_hint */
> -	    fputc(comment_line_char, fp);
> -	}
> -
> -	append_conflicts_hint(&msgbuf);
> +	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
>   	fputs(msgbuf.buf, fp);
>   	strbuf_release(&msgbuf);
>   	fclose(fp);
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9a66720cfc..fe18036be7 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   {
>   	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
>   	const char *me = action_name(opts);
> +	const char *cleanup_arg = NULL;
>   	int cmd = 0;
>   	struct option base_options[] = {
>   		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
>   		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
>   		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> +		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
>   		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
>   		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
>   		OPT_NOOP_NOARG('r', NULL),
> @@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   	if (opts->keep_redundant_commits)
>   		opts->allow_empty = 1;
>   
> +	if (cleanup_arg)
> +		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> +
>   	/* Check for incompatible command line arguments */
>   	if (cmd) {
>   		char *this_operation;
> diff --git a/sequencer.c b/sequencer.c
> index 707e72fb39..85ad58555d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
>   		if (status)
>   			return status;
>   
> -		if (!strcmp(s, "verbatim"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
> -		else if (!strcmp(s, "whitespace"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> -		else if (!strcmp(s, "strip"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
> -		else if (!strcmp(s, "scissors"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> -		else
> -			warning(_("invalid commit message cleanup mode '%s'"),
> -				  s);
> +		opts->default_msg_cleanup = get_cleanup_mode(s, 1);

If we're rebasing then I'm not sure we want to pass 1 here, we won't be 
using an editor unless the user is rewording a commit message. Also I'm 
not clear about the implications of changing the cleanup mode for 
scissors - will whitespace and comments still be cleaned up?

I'm not terribly happy with the idea that this will now die while 
reading the config file if there's on invalid cleanup option. If you 
really want to enforce a valid value then it would be better to return 
an error from get_cleanup_mode().

Best Wishes

Phillip

>   
>   		free((char *)s);
>   		return status;
> @@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>   		die(_("Invalid cleanup mode %s"), cleanup_arg);
>   }
>   
> -void append_conflicts_hint(struct strbuf *msgbuf)
> +void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
>   {
>   	int i;
>   
> +	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> +		strbuf_addch(msgbuf, '\n');
> +		wt_status_append_cut_line(msgbuf);
> +		strbuf_addch(msgbuf, comment_line_char);
> +	}
> +
>   	strbuf_addch(msgbuf, '\n');
>   	strbuf_commented_addf(msgbuf, "Conflicts:\n");
>   	for (i = 0; i < active_nr;) {
> @@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>   			_(action_name(opts)));
>   
>   	if (!clean)
> -		append_conflicts_hint(msgbuf);
> +		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
>   
>   	return !clean;
>   }
> diff --git a/sequencer.h b/sequencer.h
> index 5690e0c27e..aa99503dd7 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -91,7 +91,8 @@ int rearrange_squash(void);
>   extern const char sign_off_header[];
>   
>   void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> -void append_conflicts_hint(struct strbuf *msgbuf);
> +void append_conflicts_hint(struct strbuf *msgbuf,
> +		enum commit_msg_cleanup_mode cleanup_mode);
>   enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>   	int use_editor);
>   
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 74ff925526..c94e44dad0 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success \
> +	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config commit.cleanup scissors &&
> +	cat <<-EOF >expected &&
> +		picked
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git cherry-pick picked &&
> +
> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
> +test_expect_success \
> +	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config --unset commit.cleanup &&
> +	cat <<-EOF >expected &&
> +		picked
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git cherry-pick --cleanup=scissors picked &&
> +
> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
>   test_expect_success 'failed cherry-pick describes conflict in work tree' '
>   	pristine_detach initial &&
>   	cat <<-EOF >expected &&
> @@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success \
> +	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config commit.cleanup scissors &&
> +	cat >expected <<-EOF &&
> +		Revert "picked"
> +
> +		This reverts commit objid.
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git revert picked &&
> +
> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
> +test_expect_success \
> +	'revert conflict, ensure cleanup=scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config --unset commit.cleanup &&
> +	cat >expected <<-EOF &&
> +		Revert "picked"
> +
> +		This reverts commit objid.
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git revert --cleanup=scissors picked &&
> +
> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
>   test_expect_success 'failed cherry-pick does not forget -s' '
>   	pristine_detach initial &&
>   	test_must_fail git cherry-pick -s picked &&
> 

Re: [PATCH 2/2] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Hi Phillip,

On Wed, Mar 06, 2019 at 04:29:20PM +0000, Phillip Wood wrote:
> Hi Denton
> 
> On 06/03/2019 10:30, Denton Liu wrote:
> >Fix a bug where the scissors line is placed after the Conflicts:
> >section, in the case where a merge conflict occurs and
> >commit.cleanup = scissors.
> 
> Was that the case with cherry-pick and revert as well as merge, or were they
> missing the scissors line entirely as the subject line suggests?
> 
> >Note that the removal of the if-else tower in git_sequencer_config may
> >appear to be a no-op refactor but it actually isn't. First of all, we
> >now accept "default" as a configuration option and also we die on an
> >invalid cleanup mode. Most importantly, though, if
> >commit.cleanup = scissors, the cleanup enum will be set to
> >COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This
> >allows us to append scissors to MERGE_MSG in the case of a conflict.
> 
> I've got some comments about this change below
> 
> >Signed-off-by: Denton Liu <liu.denton@gmail.com>
> >---
> >  Documentation/git-cherry-pick.txt |  7 +++
> >  Documentation/git-revert.txt      |  7 +++
> >  builtin/merge.c                   | 13 ++---
> >  builtin/revert.c                  |  5 ++
> >  sequencer.c                       | 22 ++++----
> >  sequencer.h                       |  3 +-
> >  t/t3507-cherry-pick-conflict.sh   | 88 +++++++++++++++++++++++++++++++
> >  7 files changed, 121 insertions(+), 24 deletions(-)
> >
> >diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> >index d35d771fc8..5c086d78c8 100644
> >--- a/Documentation/git-cherry-pick.txt
> >+++ b/Documentation/git-cherry-pick.txt
> >@@ -57,6 +57,13 @@ OPTIONS
> >  	With this option, 'git cherry-pick' will let you edit the commit
> >  	message prior to committing.
> >+--cleanup=<mode>::
> >+	This option determines how the commit message will be cleaned up before
> >+	being passed on. See linkgit:git-commit[1] for more details. In
> >+	addition, if the '<mode>' is given a value of `scissors`, scissors will
> >+	be prepended to MERGE_MSG before being passed on in the case of a
> >+	conflict.
> >+
> >  -x::
> >  	When recording the commit, append a line that says
> >  	"(cherry picked from commit ...)" to the original commit
> >diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> >index 837707a8fd..1894010e60 100644
> >--- a/Documentation/git-revert.txt
> >+++ b/Documentation/git-revert.txt
> >@@ -66,6 +66,13 @@ more details.
> >  	With this option, 'git revert' will not start the commit
> >  	message editor.
> >+--cleanup=<mode>::
> >+	This option determines how the commit message will be cleaned up before
> >+	being passed on. See linkgit:git-commit[1] for more details. In
> >+	addition, if the '<mode>' is given a value of `scissors`, scissors will
> >+	be prepended to MERGE_MSG before being passed on in the case of a
> >+	conflict.
> >+
> >  -n::
> >  --no-commit::
> >  	Usually the command automatically creates some commits with
> >diff --git a/builtin/merge.c b/builtin/merge.c
> >index 92efc3d8fa..d4217ebcf5 100644
> >--- a/builtin/merge.c
> >+++ b/builtin/merge.c
> >@@ -913,17 +913,10 @@ static int suggest_conflicts(void)
> >  	 * We can't use cleanup_mode because if we're not using the editor,
> >  	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
> >  	 * though the message is meant to be processed later by git-commit.
> >-	 * Thus, we will get the cleanup mode is returned we _are_ using an
> >-	 * editor.
> >+	 * Thus, we will get the cleanup mode which is returned when we _are_ using
> >+	 * an editor.
> >  	 */
> >-	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
> >-	    fputc('\n', fp);
> >-	    wt_status_add_cut_line(fp);
> >-	    /* comments out the newline from append_conflicts_hint */
> >-	    fputc(comment_line_char, fp);
> >-	}
> >-
> >-	append_conflicts_hint(&msgbuf);
> >+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
> >  	fputs(msgbuf.buf, fp);
> >  	strbuf_release(&msgbuf);
> >  	fclose(fp);
> >diff --git a/builtin/revert.c b/builtin/revert.c
> >index 9a66720cfc..fe18036be7 100644
> >--- a/builtin/revert.c
> >+++ b/builtin/revert.c
> >@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> >  {
> >  	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
> >  	const char *me = action_name(opts);
> >+	const char *cleanup_arg = NULL;
> >  	int cmd = 0;
> >  	struct option base_options[] = {
> >  		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> >  		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> >  		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> >+		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
> >  		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
> >  		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
> >  		OPT_NOOP_NOARG('r', NULL),
> >@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> >  	if (opts->keep_redundant_commits)
> >  		opts->allow_empty = 1;
> >+	if (cleanup_arg)
> >+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> >+
> >  	/* Check for incompatible command line arguments */
> >  	if (cmd) {
> >  		char *this_operation;
> >diff --git a/sequencer.c b/sequencer.c
> >index 707e72fb39..85ad58555d 100644
> >--- a/sequencer.c
> >+++ b/sequencer.c
> >@@ -165,17 +165,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
> >  		if (status)
> >  			return status;
> >-		if (!strcmp(s, "verbatim"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
> >-		else if (!strcmp(s, "whitespace"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> >-		else if (!strcmp(s, "strip"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
> >-		else if (!strcmp(s, "scissors"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> >-		else
> >-			warning(_("invalid commit message cleanup mode '%s'"),
> >-				  s);
> >+		opts->default_msg_cleanup = get_cleanup_mode(s, 1);
> 
> If we're rebasing then I'm not sure we want to pass 1 here, we won't be
> using an editor unless the user is rewording a commit message. Also I'm not
> clear about the implications of changing the cleanup mode for scissors -
> will whitespace and comments still be cleaned up?

I believe that rebase -i should be safe with the change but to be sure,
I'll change the line to read:

	opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));

(Note to self, this will require a change in rebase--helper.c too.)

> I'm not terribly happy with the idea that this will now die while reading
> the config file if there's on invalid cleanup option. If you really want to
> enforce a valid value then it would be better to return an error from
> get_cleanup_mode().

The reason why I made it die was because I wanted to reuse that function
as much as possible and it already happened to be implemented that way.
However, there is prescedent in the code for dieing on invalid
configuration options, namely in date.c:

	if (mode->type == DATE_STRFTIME) {
		if (!skip_prefix(p, ":", &p))
			die("date format missing colon separator: %s", format);
		mode->strftime_fmt = xstrdup(p);
	} else if (*p)
		die("unknown date format %s", format);

That being said, I'm pretty ambivalent between making it a warning and a
die. If you have any strong opinions on this, I can change it to a
warning.

Thanks,

Denton

> 
> Best Wishes
> 
> Phillip
> 
> >  		free((char *)s);
> >  		return status;
> >@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> >  		die(_("Invalid cleanup mode %s"), cleanup_arg);
> >  }
> >-void append_conflicts_hint(struct strbuf *msgbuf)
> >+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
> >  {
> >  	int i;
> >+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> >+		strbuf_addch(msgbuf, '\n');
> >+		wt_status_append_cut_line(msgbuf);
> >+		strbuf_addch(msgbuf, comment_line_char);
> >+	}
> >+
> >  	strbuf_addch(msgbuf, '\n');
> >  	strbuf_commented_addf(msgbuf, "Conflicts:\n");
> >  	for (i = 0; i < active_nr;) {
> >@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >  			_(action_name(opts)));
> >  	if (!clean)
> >-		append_conflicts_hint(msgbuf);
> >+		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
> >  	return !clean;
> >  }
> >diff --git a/sequencer.h b/sequencer.h
> >index 5690e0c27e..aa99503dd7 100644
> >--- a/sequencer.h
> >+++ b/sequencer.h
> >@@ -91,7 +91,8 @@ int rearrange_squash(void);
> >  extern const char sign_off_header[];
> >  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> >-void append_conflicts_hint(struct strbuf *msgbuf);
> >+void append_conflicts_hint(struct strbuf *msgbuf,
> >+		enum commit_msg_cleanup_mode cleanup_mode);
> >  enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> >  	int use_editor);
> >diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> >index 74ff925526..c94e44dad0 100755
> >--- a/t/t3507-cherry-pick-conflict.sh
> >+++ b/t/t3507-cherry-pick-conflict.sh
> >@@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' '
> >  	test_cmp expected actual
> >  '
> >+test_expect_success \
> >+	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config commit.cleanup scissors &&
> >+	cat <<-EOF >expected &&
> >+		picked
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git cherry-pick picked &&
> >+
> >+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >+test_expect_success \
> >+	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config --unset commit.cleanup &&
> >+	cat <<-EOF >expected &&
> >+		picked
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git cherry-pick --cleanup=scissors picked &&
> >+
> >+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >  test_expect_success 'failed cherry-pick describes conflict in work tree' '
> >  	pristine_detach initial &&
> >  	cat <<-EOF >expected &&
> >@@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
> >  	test_cmp expected actual
> >  '
> >+test_expect_success \
> >+	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config commit.cleanup scissors &&
> >+	cat >expected <<-EOF &&
> >+		Revert "picked"
> >+
> >+		This reverts commit objid.
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git revert picked &&
> >+
> >+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >+test_expect_success \
> >+	'revert conflict, ensure cleanup=scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config --unset commit.cleanup &&
> >+	cat >expected <<-EOF &&
> >+		Revert "picked"
> >+
> >+		This reverts commit objid.
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git revert --cleanup=scissors picked &&
> >+
> >+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >  test_expect_success 'failed cherry-pick does not forget -s' '
> >  	pristine_detach initial &&
> >  	test_must_fail git cherry-pick -s picked &&
> >

Re: [PATCH 2/2] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Hi Phillip, sorry I forgot to address one of your replies.

On Wed, Mar 06, 2019 at 04:29:20PM +0000, Phillip Wood wrote:
> Hi Denton
> 
> On 06/03/2019 10:30, Denton Liu wrote:
> >Fix a bug where the scissors line is placed after the Conflicts:
> >section, in the case where a merge conflict occurs and
> >commit.cleanup = scissors.
> 
> Was that the case with cherry-pick and revert as well as merge, or were they
> missing the scissors line entirely as the subject line suggests?

The bug is that git-{cherry-pick,revert,merge} all place the conflicts
hint into MERGE_MSG, which then gets read by git-commit which will place
a scissors line under all that stuff.

The fix is to make git-{cherry-pick,revert,merge} put the scissors line
before the conflicts hint, so the scissors are actually placed in that
command instead of in git-commit. Is the commit message still unclear or
does it make sense now?

Thanks,

Denton

[PATCH v2 0/3] cherry-pick/revert cleanup scissors fix

[Denton Liu]

As I stated earlier[1], I discovered today that git revert (and thus,
git cherry-pick as well) do not place the scissors line properly as
well. This patchset adds a scissors line when conflicts arise in both
cherry-pick and revert, which fixes the same bug present in git-merge
earlier.

This patchset should apply on top of dl/merge-cleanup-scissors-fix.

Changes since v1:

* Address Phillip's concern of calling get_cleanup_mode with use_editor = 1;
  only set to 1 if we are calling for revert or cherry-pick, else 0 to maintain
  original behaviour (for rebasing).

* Do not die if provided an invalid cleanup option, instead just warn and
  fallback to default option.

[1]: https://public-inbox.org/git/20190306014143.GA2580@dev-l/


Denton Liu (3):
  t3507: cleanup space after redirection operators
  cherry-pick/revert: add scissors line on merge conflict
  sequencer.c: don't die on invalid cleanup_arg

 Documentation/git-cherry-pick.txt |   7 ++
 Documentation/git-revert.txt      |   7 ++
 builtin/merge.c                   |  13 +---
 builtin/rebase--helper.c          |   2 +-
 builtin/revert.c                  |   5 ++
 sequencer.c                       |  39 +++++-----
 sequencer.h                       |   3 +-
 t/t3507-cherry-pick-conflict.sh   | 122 +++++++++++++++++++++++++-----
 8 files changed, 149 insertions(+), 49 deletions(-)

Range-diff against v1:
1:  70a508ca0b ! 1:  8fdc5bfb15 cherry-pick/revert: add scissors line on merge conflict
    @@ -8,11 +8,11 @@
     
         Note that the removal of the if-else tower in git_sequencer_config may
         appear to be a no-op refactor but it actually isn't. First of all, we
    -    now accept "default" as a configuration option and also we die on an
    -    invalid cleanup mode. Most importantly, though, if
    +    now accept "default". More importantly, though, if
         commit.cleanup = scissors, the cleanup enum will be set to
    -    COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE. This
    -    allows us to append scissors to MERGE_MSG in the case of a conflict.
    +    COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
    +    are reverting or cherry-picking. This allows us to append scissors to
    +    MERGE_MSG in the case of a conflict.
     
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
     
    @@ -77,6 +77,22 @@
      	strbuf_release(&msgbuf);
      	fclose(fp);
     
    + diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
    + --- a/builtin/rebase--helper.c
    + +++ b/builtin/rebase--helper.c
    +@@
    + 		OPT_END()
    + 	};
    + 
    ++	opts.action = REPLAY_INTERACTIVE_REBASE;
    + 	sequencer_init_config(&opts);
    + 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
    + 
    +-	opts.action = REPLAY_INTERACTIVE_REBASE;
    + 	opts.allow_ff = 1;
    + 	opts.allow_empty = 1;
    + 
    +
      diff --git a/builtin/revert.c b/builtin/revert.c
      --- a/builtin/revert.c
      +++ b/builtin/revert.c
    @@ -108,6 +124,18 @@
      diff --git a/sequencer.c b/sequencer.c
      --- a/sequencer.c
      +++ b/sequencer.c
    +@@
    + static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
    + static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
    + 
    ++static inline int is_rebase_i(const struct replay_opts *opts)
    ++{
    ++	return opts->action == REPLAY_INTERACTIVE_REBASE;
    ++}
    ++
    + static int git_sequencer_config(const char *k, const char *v, void *cb)
    + {
    + 	struct replay_opts *opts = cb;
     @@
      		if (status)
      			return status;
    @@ -123,10 +151,22 @@
     -		else
     -			warning(_("invalid commit message cleanup mode '%s'"),
     -				  s);
    -+		opts->default_msg_cleanup = get_cleanup_mode(s, 1);
    ++		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
      
      		free((char *)s);
      		return status;
    +@@
    + 	git_config(git_sequencer_config, opts);
    + }
    + 
    +-static inline int is_rebase_i(const struct replay_opts *opts)
    +-{
    +-	return opts->action == REPLAY_INTERACTIVE_REBASE;
    +-}
    +-
    + static const char *get_dir(const struct replay_opts *opts)
    + {
    + 	if (is_rebase_i(opts))
     @@
      		die(_("Invalid cleanup mode %s"), cleanup_arg);
      }
-:  ---------- > 2:  f3af8000ae sequencer.c: don't die on invalid cleanup_arg
-- 
2.21.0.368.gbf248417d7

[PATCH v2 1/3] t3507: cleanup space after redirection operators

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 34 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0db166152a..74ff925526 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -88,7 +88,7 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
 
 test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -96,7 +96,7 @@ test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 test_expect_success \
 	'cherry-pick --strategy=resolve w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick --strategy=resolve base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -175,23 +175,23 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 		git ls-files --stage foo &&
 		git checkout picked -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected &&
+	" <stages >expected &&
 	git read-tree -u --reset HEAD &&
 
 	test_must_fail git cherry-pick picked &&
-	git ls-files --stage --unmerged > actual &&
+	git ls-files --stage --unmerged >actual &&
 
 	test_cmp expected actual
 '
 
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -201,14 +201,14 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| parent of objid picked
@@ -220,14 +220,14 @@ test_expect_success 'diff3 -m style' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'revert also handles conflicts sanely' '
 	git config --unset merge.conflictstyle &&
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -241,24 +241,24 @@ test_expect_success 'revert also handles conflicts sanely' '
 		git ls-files --stage foo &&
 		git checkout base -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected-stages &&
+	" <stages >expected-stages &&
 	git read-tree -u --reset HEAD &&
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git revert picked &&
 	newhead=$(git rev-parse HEAD) &&
-	git ls-files --stage --unmerged > actual-stages &&
+	git ls-files --stage --unmerged >actual-stages &&
 
 	test "$head" = "$newhead" &&
 	test_must_fail git update-index --refresh -q &&
 	test_must_fail git diff-index --exit-code HEAD &&
 	test_cmp expected-stages actual-stages &&
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
@@ -284,7 +284,7 @@ test_expect_success 'revert --no-commit sets REVERT_HEAD' '
 
 test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' '
 	pristine_detach base &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git revert base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
 	test_must_fail git rev-parse --verify REVERT_HEAD
@@ -319,7 +319,7 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '
 test_expect_success 'revert conflict, diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| objid picked
@@ -331,7 +331,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 
 	test_must_fail git revert picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
-- 
2.21.0.368.gbf248417d7

Re: [PATCH v2 1/3] t3507: cleanup space after redirection operators

[Junio C Hamano]

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

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t3507-cherry-pick-conflict.sh | 34 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

A good preliminary clean-up.  Looks good.

[PATCH v2 2/3] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Fix a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
commit.cleanup = scissors.

Note that the removal of the if-else tower in git_sequencer_config may
appear to be a no-op refactor but it actually isn't. First of all, we
now accept "default". More importantly, though, if
commit.cleanup = scissors, the cleanup enum will be set to
COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
are reverting or cherry-picking. This allows us to append scissors to
MERGE_MSG in the case of a conflict.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-cherry-pick.txt |  7 +++
 Documentation/git-revert.txt      |  7 +++
 builtin/merge.c                   | 13 ++---
 builtin/rebase--helper.c          |  2 +-
 builtin/revert.c                  |  5 ++
 sequencer.c                       | 32 +++++------
 sequencer.h                       |  3 +-
 t/t3507-cherry-pick-conflict.sh   | 88 +++++++++++++++++++++++++++++++
 8 files changed, 127 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc8..5c086d78c8 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -57,6 +57,13 @@ OPTIONS
 	With this option, 'git cherry-pick' will let you edit the commit
 	message prior to committing.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be prepended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -x::
 	When recording the commit, append a line that says
 	"(cherry picked from commit ...)" to the original commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..1894010e60 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -66,6 +66,13 @@ more details.
 	With this option, 'git revert' will not start the commit
 	message editor.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be prepended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -n::
 --no-commit::
 	Usually the command automatically creates some commits with
diff --git a/builtin/merge.c b/builtin/merge.c
index 92efc3d8fa..d4217ebcf5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,17 +913,10 @@ static int suggest_conflicts(void)
 	 * We can't use cleanup_mode because if we're not using the editor,
 	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
 	 * though the message is meant to be processed later by git-commit.
-	 * Thus, we will get the cleanup mode is returned we _are_ using an
-	 * editor.
+	 * Thus, we will get the cleanup mode which is returned when we _are_ using
+	 * an editor.
 	 */
-	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
-	    fputc('\n', fp);
-	    wt_status_add_cut_line(fp);
-	    /* comments out the newline from append_conflicts_hint */
-	    fputc(comment_line_char, fp);
-	}
-
-	append_conflicts_hint(&msgbuf);
+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
 	fclose(fp);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc8..5f1bc9d383 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -48,10 +48,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	opts.action = REPLAY_INTERACTIVE_REBASE;
 	sequencer_init_config(&opts);
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
-	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
 	opts.allow_empty = 1;
 
diff --git a/builtin/revert.c b/builtin/revert.c
index 9a66720cfc..fe18036be7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
+	const char *cleanup_arg = NULL;
 	int cmd = 0;
 	struct option base_options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
+		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
 		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->keep_redundant_commits)
 		opts->allow_empty = 1;
 
+	if (cleanup_arg)
+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
+
 	/* Check for incompatible command line arguments */
 	if (cmd) {
 		char *this_operation;
diff --git a/sequencer.c b/sequencer.c
index 707e72fb39..5c04bae7ac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -153,6 +153,11 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 
+static inline int is_rebase_i(const struct replay_opts *opts)
+{
+	return opts->action == REPLAY_INTERACTIVE_REBASE;
+}
+
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
 	struct replay_opts *opts = cb;
@@ -165,17 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 		if (status)
 			return status;
 
-		if (!strcmp(s, "verbatim"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
-		else if (!strcmp(s, "whitespace"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else if (!strcmp(s, "strip"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
-		else if (!strcmp(s, "scissors"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else
-			warning(_("invalid commit message cleanup mode '%s'"),
-				  s);
+		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
 
 		free((char *)s);
 		return status;
@@ -199,11 +194,6 @@ void sequencer_init_config(struct replay_opts *opts)
 	git_config(git_sequencer_config, opts);
 }
 
-static inline int is_rebase_i(const struct replay_opts *opts)
-{
-	return opts->action == REPLAY_INTERACTIVE_REBASE;
-}
-
 static const char *get_dir(const struct replay_opts *opts)
 {
 	if (is_rebase_i(opts))
@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 }
 
-void append_conflicts_hint(struct strbuf *msgbuf)
+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
 {
 	int i;
 
+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+		strbuf_addch(msgbuf, '\n');
+		wt_status_append_cut_line(msgbuf);
+		strbuf_addch(msgbuf, comment_line_char);
+	}
+
 	strbuf_addch(msgbuf, '\n');
 	strbuf_commented_addf(msgbuf, "Conflicts:\n");
 	for (i = 0; i < active_nr;) {
@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 			_(action_name(opts)));
 
 	if (!clean)
-		append_conflicts_hint(msgbuf);
+		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
 
 	return !clean;
 }
diff --git a/sequencer.h b/sequencer.h
index 5690e0c27e..aa99503dd7 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -91,7 +91,8 @@ int rearrange_squash(void);
 extern const char sign_off_header[];
 
 void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
-void append_conflicts_hint(struct strbuf *msgbuf);
+void append_conflicts_hint(struct strbuf *msgbuf,
+		enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 	int use_editor);
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 74ff925526..c94e44dad0 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -189,6 +189,48 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
+test_expect_success \
+	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick --cleanup=scissors picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
 	cat <<-EOF >expected &&
@@ -335,6 +377,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit objid.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
+test_expect_success \
+	'revert conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit objid.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert --cleanup=scissors picked &&
+
+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'failed cherry-pick does not forget -s' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
-- 
2.21.0.368.gbf248417d7

Re: [PATCH v2 2/3] cherry-pick/revert: add scissors line on merge conflict

[Junio C Hamano]

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

> Fix a bug where the scissors line is placed after the Conflicts:
> section, in the case where a merge conflict occurs and
> commit.cleanup = scissors.
>
> Note that the removal of the if-else tower in git_sequencer_config may
> appear to be a no-op refactor but it actually isn't. First of all, we
> now accept "default". More importantly, though, if
> commit.cleanup = scissors, the cleanup enum will be set to
> COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
> are reverting or cherry-picking. This allows us to append scissors to
> MERGE_MSG in the case of a conflict.

Good thing to point out in the log message.

> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index d35d771fc8..5c086d78c8 100644
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index 837707a8fd..1894010e60 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -66,6 +66,13 @@ more details.
>  	With this option, 'git revert' will not start the commit
>  	message editor.
>  
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be prepended to MERGE_MSG before being passed on in the case of a
> +	conflict.

These both say "prepended", but shouldn't the code

 - add the merge message proper, with the expectation that it would
   be used more or less intact in the final commit message; then

 - add the scissors; then

 - append informative cruft that would be removed, only to help
   human users.

in this order?  I'd expect that most people would consider that the
primary payload of MERGE_MSG file is the part that would become the
commit message, so I would have expected the second step would be
described as "appended to".

Another thing I notice is that this singles out "scissors" mode;
doesn't the code do anything worth describing with other clean-up
modes?

> +	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&

Use $_x40 (or $OID_REGEX) to make it more readable and also
future-proof?

Re: [PATCH v2 2/3] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Hi Junio,

On Thu, Mar 07, 2019 at 04:52:57PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> > index d35d771fc8..5c086d78c8 100644
> > diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> > index 837707a8fd..1894010e60 100644
> > --- a/Documentation/git-revert.txt
> > +++ b/Documentation/git-revert.txt
> > @@ -66,6 +66,13 @@ more details.
> >  	With this option, 'git revert' will not start the commit
> >  	message editor.
> >  
> > +--cleanup=<mode>::
> > +	This option determines how the commit message will be cleaned up before
> > +	being passed on. See linkgit:git-commit[1] for more details. In
> > +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> > +	be prepended to MERGE_MSG before being passed on in the case of a
> > +	conflict.
> 
> These both say "prepended", but shouldn't the code
> 
>  - add the merge message proper, with the expectation that it would
>    be used more or less intact in the final commit message; then
> 
>  - add the scissors; then
> 
>  - append informative cruft that would be removed, only to help
>    human users.
> 
> in this order?  I'd expect that most people would consider that the
> primary payload of MERGE_MSG file is the part that would become the
> commit message, so I would have expected the second step would be
> described as "appended to".

I copied this over from 6f06b6aeef (merge: add scissors line on merge
conflict, 2019-01-22). I guess I'll have to fix it up in git-merge too.

> 
> Another thing I notice is that this singles out "scissors" mode;
> doesn't the code do anything worth describing with other clean-up
> modes?

The revert/cherry-pick code doesn't really do anything with the cleanup
mode. It's only used to determine whether to generate the scissors line
in the case of a conflict.

[PATCH v2 3/3] sequencer.c: don't die on invalid cleanup_arg

[Denton Liu]

When get_cleanup_mode was provided with an invalid cleanup_arg, it used
to die. Warn user and fallback to default behaviour if an invalid
cleanup_arg is given.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 sequencer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5c04bae7ac..f9bdfa90ad 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -502,8 +502,11 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 	else if (!strcmp(cleanup_arg, "scissors"))
 		return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
 				    COMMIT_MSG_CLEANUP_SPACE;
-	else
-		die(_("Invalid cleanup mode %s"), cleanup_arg);
+	else {
+		warning(_("Invalid cleanup mode %s, falling back to default"), cleanup_arg);
+		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
+				    COMMIT_MSG_CLEANUP_SPACE;
+	}
 }
 
 void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
-- 
2.21.0.368.gbf248417d7

Re: [PATCH v2 3/3] sequencer.c: don't die on invalid cleanup_arg

[Junio C Hamano]

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

> When get_cleanup_mode was provided with an invalid cleanup_arg, it used
> to die. Warn user and fallback to default behaviour if an invalid
> cleanup_arg is given.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  sequencer.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5c04bae7ac..f9bdfa90ad 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -502,8 +502,11 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>  	else if (!strcmp(cleanup_arg, "scissors"))
>  		return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
>  				    COMMIT_MSG_CLEANUP_SPACE;
> -	else
> -		die(_("Invalid cleanup mode %s"), cleanup_arg);
> +	else {
> +		warning(_("Invalid cleanup mode %s, falling back to default"), cleanup_arg);
> +		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
> +				    COMMIT_MSG_CLEANUP_SPACE;
> +	}
>  }

In what different contexts does this get called, I wonder?

I think

	$ git cherry-pick --cleanup=bogus ...

should error out, instead of falling back to anything else, while
having

	[commit] cleanup = bogus

in .git/config should *not* say anything if you are not running a
command that does not get affected by the commit.cleanup variable,
and with such a bogus configuration, a command that does use the
variable should either also die, or fallback with a warning.

I have a suspicion that the change is breaking the error detection
done for the command line argument parsing, and if that is the case,
then it is a bad idea.  But I may have misread the code.

Thanks.

[PATCH v3 0/4] cherry-pick/revert cleanup scissors fix

[Denton Liu]

As I stated earlier[1], I discovered today that git revert (and thus,
git cherry-pick as well) do not place the scissors line properly as
well. This patchset adds a scissors line when conflicts arise in both
cherry-pick and revert, which fixes the same bug present in git-merge
earlier.

This patchset should apply on top of dl/merge-cleanup-scissors-fix.

Changes since v1:

* Address Phillip's concern of calling get_cleanup_mode with use_editor = 1;
  only set to 1 if we are calling for revert or cherry-pick, else 0 to maintain
  original behaviour (for rebasing).

* Do not die if provided an invalid cleanup option, instead just warn and
  fallback to default option.

Changes since v2:

* s/prepended/appended/

* Use $OID_REGEX instead of self-made regex

* Don't die if bad config option is given, otherwise die if a bad
  command-line argument is given

[1]: https://public-inbox.org/git/20190306014143.GA2580@dev-l/


Denton Liu (4):
  merge-options.txt: correct typo
  t3507: cleanup space after redirection operators
  cherry-pick/revert: add scissors line on merge conflict
  sequencer.c: don't die on invalid cleanup_arg

 Documentation/git-cherry-pick.txt |   7 ++
 Documentation/git-revert.txt      |   7 ++
 Documentation/merge-options.txt   |   4 +-
 builtin/commit.c                  |   2 +-
 builtin/merge.c                   |  15 +---
 builtin/rebase--helper.c          |   2 +-
 builtin/revert.c                  |   5 ++
 sequencer.c                       |  40 +++++-----
 sequencer.h                       |   5 +-
 t/t3507-cherry-pick-conflict.sh   | 120 +++++++++++++++++++++++++-----
 10 files changed, 153 insertions(+), 54 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  1076d4c451 merge-options.txt: correct typo
1:  4eee16cc5a = 2:  6bce4d4722 t3507: cleanup space after redirection operators
2:  8fdc5bfb15 ! 3:  14672ce10c cherry-pick/revert: add scissors line on merge conflict
    @@ -27,7 +27,7 @@
     +	This option determines how the commit message will be cleaned up before
     +	being passed on. See linkgit:git-commit[1] for more details. In
     +	addition, if the '<mode>' is given a value of `scissors`, scissors will
    -+	be prepended to MERGE_MSG before being passed on in the case of a
    ++	be appended to MERGE_MSG before being passed on in the case of a
     +	conflict.
     +
      -x::
    @@ -45,7 +45,7 @@
     +	This option determines how the commit message will be cleaned up before
     +	being passed on. See linkgit:git-commit[1] for more details. In
     +	addition, if the '<mode>' is given a value of `scissors`, scissors will
    -+	be prepended to MERGE_MSG before being passed on in the case of a
    ++	be appended to MERGE_MSG before being passed on in the case of a
     +	conflict.
     +
      -n::
    @@ -233,8 +233,7 @@
     +
     +	test_must_fail git cherry-pick picked &&
     +
    -+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
    -+	test_i18ncmp expected actual
    ++	test_i18ncmp expected .git/MERGE_MSG
     +'
     +
     +test_expect_success \
    @@ -254,8 +253,7 @@
     +
     +	test_must_fail git cherry-pick --cleanup=scissors picked &&
     +
    -+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
    -+	test_i18ncmp expected actual
    ++	test_i18ncmp expected .git/MERGE_MSG
     +'
     +
      test_expect_success 'failed cherry-pick describes conflict in work tree' '
    @@ -272,7 +270,7 @@
     +	cat >expected <<-EOF &&
     +		Revert "picked"
     +
    -+		This reverts commit objid.
    ++		This reverts commit OBJID.
     +
     +		# ------------------------ >8 ------------------------
     +		# Do not modify or remove the line above.
    @@ -284,7 +282,7 @@
     +
     +	test_must_fail git revert picked &&
     +
    -+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
    ++	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
     +	test_i18ncmp expected actual
     +'
     +
    @@ -295,7 +293,7 @@
     +	cat >expected <<-EOF &&
     +		Revert "picked"
     +
    -+		This reverts commit objid.
    ++		This reverts commit OBJID.
     +
     +		# ------------------------ >8 ------------------------
     +		# Do not modify or remove the line above.
    @@ -307,7 +305,7 @@
     +
     +	test_must_fail git revert --cleanup=scissors picked &&
     +
    -+	sed "s/[a-f0-9]\{40\}/objid/" .git/MERGE_MSG >actual &&
    ++	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
     +	test_i18ncmp expected actual
     +'
     +
3:  f3af8000ae < -:  ---------- sequencer.c: don't die on invalid cleanup_arg
-:  ---------- > 4:  68ec2b7cd7 sequencer.c: don't die on invalid cleanup_arg
-- 
2.21.0.370.g4fdb13b891

[PATCH v3 1/4] merge-options.txt: correct typo

[Denton Liu]

The scissors line is included after the human commit message. Therefore,
saying "scissors will be prepended" was incorrect. Update to say
"appended" which is more correct.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

Notes:
    Note that this can be squashed into 6f06b6aeef (merge: add scissors line on
    merge conflict, 2019-01-22) if it makes life easier.

 Documentation/merge-options.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index c2a263ba74..6fff647409 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -31,8 +31,8 @@ set to `no` at the beginning of them.
 	This option determines how the merge message will be cleaned up
 	before commiting or being passed on. See linkgit:git-commit[1] for more
 	details. In addition, if the '<mode>' is given a value of `scissors`,
-	scissors will be prepended to MERGE_MSG before being passed on in the case
-	of a merge conflict.
+	scissors will be appended to MERGE_MSG before being passed on in the
+	case of a merge conflict.
 
 --ff::
 	When the merge resolves as a fast-forward, only update the branch
-- 
2.21.0.370.g4fdb13b891

Re: [PATCH v3 1/4] merge-options.txt: correct typo

[Junio C Hamano]

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

> The scissors line is included after the human commit message. Therefore,
> saying "scissors will be prepended" was incorrect. Update to say
> "appended" which is more correct.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>
> Notes:
>     Note that this can be squashed into 6f06b6aeef (merge: add scissors line on
>     merge conflict, 2019-01-22) if it makes life easier.

The reason why you had to make this as an incremental on top of an
older codebase (cf. Phillip's comment on 3/4) is because it is
already in 'next' and we do not wholesale replace.  I suspect that
we merged it 'next' before it was ready---sorry about that.

As the original series dl/merge-cleanup-scissors-fix is a short
series that is only 4 patches long, let me kick it out of 'next' to
make our lives easier.  That way, you can send the squashed result
as a fresh iteration, solving what these new 4 patches tries to as
the proper part of the series, instead of making them as if they are
afterthought on top of the original 4-patch series that was
incomplete.

Thanks.

[PATCH v3 2/4] t3507: cleanup space after redirection operators

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 34 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0db166152a..74ff925526 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -88,7 +88,7 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
 
 test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -96,7 +96,7 @@ test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
 test_expect_success \
 	'cherry-pick --strategy=resolve w/dirty tree does not set CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git cherry-pick --strategy=resolve base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
@@ -175,23 +175,23 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 		git ls-files --stage foo &&
 		git checkout picked -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected &&
+	" <stages >expected &&
 	git read-tree -u --reset HEAD &&
 
 	test_must_fail git cherry-pick picked &&
-	git ls-files --stage --unmerged > actual &&
+	git ls-files --stage --unmerged >actual &&
 
 	test_cmp expected actual
 '
 
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -201,14 +201,14 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| parent of objid picked
@@ -220,14 +220,14 @@ test_expect_success 'diff3 -m style' '
 
 	test_must_fail git cherry-pick picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'revert also handles conflicts sanely' '
 	git config --unset merge.conflictstyle &&
 	pristine_detach initial &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	=======
@@ -241,24 +241,24 @@ test_expect_success 'revert also handles conflicts sanely' '
 		git ls-files --stage foo &&
 		git checkout base -- foo &&
 		git ls-files --stage foo
-	} > stages &&
+	} >stages &&
 	sed "
 		1 s/ 0	/ 1	/
 		2 s/ 0	/ 2	/
 		3 s/ 0	/ 3	/
-	" < stages > expected-stages &&
+	" <stages >expected-stages &&
 	git read-tree -u --reset HEAD &&
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git revert picked &&
 	newhead=$(git rev-parse HEAD) &&
-	git ls-files --stage --unmerged > actual-stages &&
+	git ls-files --stage --unmerged >actual-stages &&
 
 	test "$head" = "$newhead" &&
 	test_must_fail git update-index --refresh -q &&
 	test_must_fail git diff-index --exit-code HEAD &&
 	test_cmp expected-stages actual-stages &&
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
@@ -284,7 +284,7 @@ test_expect_success 'revert --no-commit sets REVERT_HEAD' '
 
 test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' '
 	pristine_detach base &&
-	echo foo > foo &&
+	echo foo >foo &&
 	test_must_fail git revert base &&
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
 	test_must_fail git rev-parse --verify REVERT_HEAD
@@ -319,7 +319,7 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '
 test_expect_success 'revert conflict, diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-	cat <<-EOF > expected &&
+	cat <<-EOF >expected &&
 	<<<<<<< HEAD
 	a
 	||||||| objid picked
@@ -331,7 +331,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 
 	test_must_fail git revert picked &&
 
-	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
+	sed "s/[a-f0-9]*\.\.\./objid/" foo >actual &&
 	test_cmp expected actual
 '
 
-- 
2.21.0.370.g4fdb13b891

[PATCH v3 3/4] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Fix a bug where the scissors line is placed after the Conflicts:
section, in the case where a merge conflict occurs and
commit.cleanup = scissors.

Note that the removal of the if-else tower in git_sequencer_config may
appear to be a no-op refactor but it actually isn't. First of all, we
now accept "default". More importantly, though, if
commit.cleanup = scissors, the cleanup enum will be set to
COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
are reverting or cherry-picking. This allows us to append scissors to
MERGE_MSG in the case of a conflict.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-cherry-pick.txt |  7 +++
 Documentation/git-revert.txt      |  7 +++
 builtin/merge.c                   | 13 ++---
 builtin/rebase--helper.c          |  2 +-
 builtin/revert.c                  |  5 ++
 sequencer.c                       | 32 +++++-------
 sequencer.h                       |  3 +-
 t/t3507-cherry-pick-conflict.sh   | 86 +++++++++++++++++++++++++++++++
 8 files changed, 125 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc8..92235bd1c4 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -57,6 +57,13 @@ OPTIONS
 	With this option, 'git cherry-pick' will let you edit the commit
 	message prior to committing.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be appended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -x::
 	When recording the commit, append a line that says
 	"(cherry picked from commit ...)" to the original commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 837707a8fd..bd4ad395a9 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -66,6 +66,13 @@ more details.
 	With this option, 'git revert' will not start the commit
 	message editor.
 
+--cleanup=<mode>::
+	This option determines how the commit message will be cleaned up before
+	being passed on. See linkgit:git-commit[1] for more details. In
+	addition, if the '<mode>' is given a value of `scissors`, scissors will
+	be appended to MERGE_MSG before being passed on in the case of a
+	conflict.
+
 -n::
 --no-commit::
 	Usually the command automatically creates some commits with
diff --git a/builtin/merge.c b/builtin/merge.c
index 92efc3d8fa..d4217ebcf5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,17 +913,10 @@ static int suggest_conflicts(void)
 	 * We can't use cleanup_mode because if we're not using the editor,
 	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
 	 * though the message is meant to be processed later by git-commit.
-	 * Thus, we will get the cleanup mode is returned we _are_ using an
-	 * editor.
+	 * Thus, we will get the cleanup mode which is returned when we _are_ using
+	 * an editor.
 	 */
-	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
-	    fputc('\n', fp);
-	    wt_status_add_cut_line(fp);
-	    /* comments out the newline from append_conflicts_hint */
-	    fputc(comment_line_char, fp);
-	}
-
-	append_conflicts_hint(&msgbuf);
+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
 	fclose(fp);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc8..5f1bc9d383 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -48,10 +48,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	opts.action = REPLAY_INTERACTIVE_REBASE;
 	sequencer_init_config(&opts);
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
-	opts.action = REPLAY_INTERACTIVE_REBASE;
 	opts.allow_ff = 1;
 	opts.allow_empty = 1;
 
diff --git a/builtin/revert.c b/builtin/revert.c
index 9a66720cfc..fe18036be7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
+	const char *cleanup_arg = NULL;
 	int cmd = 0;
 	struct option base_options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
+		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
 		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->keep_redundant_commits)
 		opts->allow_empty = 1;
 
+	if (cleanup_arg)
+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
+
 	/* Check for incompatible command line arguments */
 	if (cmd) {
 		char *this_operation;
diff --git a/sequencer.c b/sequencer.c
index 707e72fb39..5c04bae7ac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -153,6 +153,11 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
 static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 
+static inline int is_rebase_i(const struct replay_opts *opts)
+{
+	return opts->action == REPLAY_INTERACTIVE_REBASE;
+}
+
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
 	struct replay_opts *opts = cb;
@@ -165,17 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 		if (status)
 			return status;
 
-		if (!strcmp(s, "verbatim"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
-		else if (!strcmp(s, "whitespace"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else if (!strcmp(s, "strip"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
-		else if (!strcmp(s, "scissors"))
-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
-		else
-			warning(_("invalid commit message cleanup mode '%s'"),
-				  s);
+		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
 
 		free((char *)s);
 		return status;
@@ -199,11 +194,6 @@ void sequencer_init_config(struct replay_opts *opts)
 	git_config(git_sequencer_config, opts);
 }
 
-static inline int is_rebase_i(const struct replay_opts *opts)
-{
-	return opts->action == REPLAY_INTERACTIVE_REBASE;
-}
-
 static const char *get_dir(const struct replay_opts *opts)
 {
 	if (is_rebase_i(opts))
@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 }
 
-void append_conflicts_hint(struct strbuf *msgbuf)
+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
 {
 	int i;
 
+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
+		strbuf_addch(msgbuf, '\n');
+		wt_status_append_cut_line(msgbuf);
+		strbuf_addch(msgbuf, comment_line_char);
+	}
+
 	strbuf_addch(msgbuf, '\n');
 	strbuf_commented_addf(msgbuf, "Conflicts:\n");
 	for (i = 0; i < active_nr;) {
@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 			_(action_name(opts)));
 
 	if (!clean)
-		append_conflicts_hint(msgbuf);
+		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
 
 	return !clean;
 }
diff --git a/sequencer.h b/sequencer.h
index 5690e0c27e..aa99503dd7 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -91,7 +91,8 @@ int rearrange_squash(void);
 extern const char sign_off_header[];
 
 void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
-void append_conflicts_hint(struct strbuf *msgbuf);
+void append_conflicts_hint(struct strbuf *msgbuf,
+		enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 	int use_editor);
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 74ff925526..c3894ca9d6 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -189,6 +189,46 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick picked &&
+
+	test_i18ncmp expected .git/MERGE_MSG
+'
+
+test_expect_success \
+	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat <<-EOF >expected &&
+		picked
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git cherry-pick --cleanup=scissors picked &&
+
+	test_i18ncmp expected .git/MERGE_MSG
+'
+
 test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	pristine_detach initial &&
 	cat <<-EOF >expected &&
@@ -335,6 +375,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	test_cmp expected actual
 '
 
+test_expect_success \
+	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config commit.cleanup scissors &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit OBJID.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert picked &&
+
+	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
+test_expect_success \
+	'revert conflict, ensure cleanup=scissors places scissors line properly' '
+	pristine_detach initial &&
+	git config --unset commit.cleanup &&
+	cat >expected <<-EOF &&
+		Revert "picked"
+
+		This reverts commit OBJID.
+
+		# ------------------------ >8 ------------------------
+		# Do not modify or remove the line above.
+		# Everything below it will be ignored.
+		#
+		# Conflicts:
+		#	foo
+		EOF
+
+	test_must_fail git revert --cleanup=scissors picked &&
+
+	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'failed cherry-pick does not forget -s' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s picked &&
-- 
2.21.0.370.g4fdb13b891

Re: [PATCH v3 3/4] cherry-pick/revert: add scissors line on merge conflict

[Phillip Wood]

Hi Denton

On 07/03/2019 09:58, Denton Liu wrote:
> Fix a bug where the scissors line is placed after the Conflicts:
> section, in the case where a merge conflict occurs and
> commit.cleanup = scissors.
> 
> Note that the removal of the if-else tower in git_sequencer_config may
> appear to be a no-op refactor but it actually isn't. First of all, we
> now accept "default". More importantly, though, if
> commit.cleanup = scissors, the cleanup enum will be set to
> COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
> are reverting or cherry-picking. This allows us to append scissors to
> MERGE_MSG in the case of a conflict.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   Documentation/git-cherry-pick.txt |  7 +++
>   Documentation/git-revert.txt      |  7 +++
>   builtin/merge.c                   | 13 ++---
>   builtin/rebase--helper.c          |  2 +-
>   builtin/revert.c                  |  5 ++
>   sequencer.c                       | 32 +++++-------
>   sequencer.h                       |  3 +-
>   t/t3507-cherry-pick-conflict.sh   | 86 +++++++++++++++++++++++++++++++
>   8 files changed, 125 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index d35d771fc8..92235bd1c4 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -57,6 +57,13 @@ OPTIONS
>   	With this option, 'git cherry-pick' will let you edit the commit
>   	message prior to committing.
>   
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be appended to MERGE_MSG before being passed on in the case of a
> +	conflict.
> +
>   -x::
>   	When recording the commit, append a line that says
>   	"(cherry picked from commit ...)" to the original commit
> diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> index 837707a8fd..bd4ad395a9 100644
> --- a/Documentation/git-revert.txt
> +++ b/Documentation/git-revert.txt
> @@ -66,6 +66,13 @@ more details.
>   	With this option, 'git revert' will not start the commit
>   	message editor.
>   
> +--cleanup=<mode>::
> +	This option determines how the commit message will be cleaned up before
> +	being passed on. See linkgit:git-commit[1] for more details. In
> +	addition, if the '<mode>' is given a value of `scissors`, scissors will
> +	be appended to MERGE_MSG before being passed on in the case of a
> +	conflict.
> +
>   -n::
>   --no-commit::
>   	Usually the command automatically creates some commits with
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 92efc3d8fa..d4217ebcf5 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -913,17 +913,10 @@ static int suggest_conflicts(void)
>   	 * We can't use cleanup_mode because if we're not using the editor,
>   	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
>   	 * though the message is meant to be processed later by git-commit.
> -	 * Thus, we will get the cleanup mode is returned we _are_ using an
> -	 * editor.
> +	 * Thus, we will get the cleanup mode which is returned when we _are_ using
> +	 * an editor.
>   	 */
> -	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
> -	    fputc('\n', fp);
> -	    wt_status_add_cut_line(fp);
> -	    /* comments out the newline from append_conflicts_hint */
> -	    fputc(comment_line_char, fp);
> -	}
> -
> -	append_conflicts_hint(&msgbuf);
> +	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
>   	fputs(msgbuf.buf, fp);
>   	strbuf_release(&msgbuf);
>   	fclose(fp);
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index f7c2a5fdc8..5f1bc9d383 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -48,10 +48,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>   		OPT_END()
>   	};
>   
> +	opts.action = REPLAY_INTERACTIVE_REBASE;
>   	sequencer_init_config(&opts);
>   	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
>   
> -	opts.action = REPLAY_INTERACTIVE_REBASE;
>   	opts.allow_ff = 1;
>   	opts.allow_empty = 1;


What are you basing this series on? builtin/rebase--helper.c was removed 
last September in 34b47315d9 ("rebase -i: move rebase--helper modes to 
rebase--interactive", 2018-09-27)

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9a66720cfc..fe18036be7 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   {
>   	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
>   	const char *me = action_name(opts);
> +	const char *cleanup_arg = NULL;
>   	int cmd = 0;
>   	struct option base_options[] = {
>   		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
>   		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
>   		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> +		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
>   		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
>   		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
>   		OPT_NOOP_NOARG('r', NULL),
> @@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   	if (opts->keep_redundant_commits)
>   		opts->allow_empty = 1;
>   
> +	if (cleanup_arg)
> +		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> +
>   	/* Check for incompatible command line arguments */
>   	if (cmd) {
>   		char *this_operation;
> diff --git a/sequencer.c b/sequencer.c
> index 707e72fb39..5c04bae7ac 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -153,6 +153,11 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
>   static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
>   static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
>   
> +static inline int is_rebase_i(const struct replay_opts *opts)
> +{
> +	return opts->action == REPLAY_INTERACTIVE_REBASE;
> +}
> +

This is already in sequencer.c in a different place in recent versions

Best Wishes

Phillip

>   static int git_sequencer_config(const char *k, const char *v, void *cb)
>   {
>   	struct replay_opts *opts = cb;
> @@ -165,17 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
>   		if (status)
>   			return status;
>   
> -		if (!strcmp(s, "verbatim"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
> -		else if (!strcmp(s, "whitespace"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> -		else if (!strcmp(s, "strip"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
> -		else if (!strcmp(s, "scissors"))
> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> -		else
> -			warning(_("invalid commit message cleanup mode '%s'"),
> -				  s);
> +		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
>   
>   		free((char *)s);
>   		return status;
> @@ -199,11 +194,6 @@ void sequencer_init_config(struct replay_opts *opts)
>   	git_config(git_sequencer_config, opts);
>   }
>   
> -static inline int is_rebase_i(const struct replay_opts *opts)
> -{
> -	return opts->action == REPLAY_INTERACTIVE_REBASE;
> -}
> -
>   static const char *get_dir(const struct replay_opts *opts)
>   {
>   	if (is_rebase_i(opts))
> @@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>   		die(_("Invalid cleanup mode %s"), cleanup_arg);
>   }
>   
> -void append_conflicts_hint(struct strbuf *msgbuf)
> +void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
>   {
>   	int i;
>   
> +	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> +		strbuf_addch(msgbuf, '\n');
> +		wt_status_append_cut_line(msgbuf);
> +		strbuf_addch(msgbuf, comment_line_char);
> +	}
> +
>   	strbuf_addch(msgbuf, '\n');
>   	strbuf_commented_addf(msgbuf, "Conflicts:\n");
>   	for (i = 0; i < active_nr;) {
> @@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>   			_(action_name(opts)));
>   
>   	if (!clean)
> -		append_conflicts_hint(msgbuf);
> +		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
>   
>   	return !clean;
>   }
> diff --git a/sequencer.h b/sequencer.h
> index 5690e0c27e..aa99503dd7 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -91,7 +91,8 @@ int rearrange_squash(void);
>   extern const char sign_off_header[];
>   
>   void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> -void append_conflicts_hint(struct strbuf *msgbuf);
> +void append_conflicts_hint(struct strbuf *msgbuf,
> +		enum commit_msg_cleanup_mode cleanup_mode);
>   enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>   	int use_editor);
>   
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 74ff925526..c3894ca9d6 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -189,6 +189,46 @@ test_expect_success 'failed cherry-pick registers participants in index' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success \
> +	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config commit.cleanup scissors &&
> +	cat <<-EOF >expected &&
> +		picked
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git cherry-pick picked &&
> +
> +	test_i18ncmp expected .git/MERGE_MSG
> +'
> +
> +test_expect_success \
> +	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config --unset commit.cleanup &&
> +	cat <<-EOF >expected &&
> +		picked
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git cherry-pick --cleanup=scissors picked &&
> +
> +	test_i18ncmp expected .git/MERGE_MSG
> +'
> +
>   test_expect_success 'failed cherry-pick describes conflict in work tree' '
>   	pristine_detach initial &&
>   	cat <<-EOF >expected &&
> @@ -335,6 +375,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
>   	test_cmp expected actual
>   '
>   
> +test_expect_success \
> +	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config commit.cleanup scissors &&
> +	cat >expected <<-EOF &&
> +		Revert "picked"
> +
> +		This reverts commit OBJID.
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git revert picked &&
> +
> +	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
> +test_expect_success \
> +	'revert conflict, ensure cleanup=scissors places scissors line properly' '
> +	pristine_detach initial &&
> +	git config --unset commit.cleanup &&
> +	cat >expected <<-EOF &&
> +		Revert "picked"
> +
> +		This reverts commit OBJID.
> +
> +		# ------------------------ >8 ------------------------
> +		# Do not modify or remove the line above.
> +		# Everything below it will be ignored.
> +		#
> +		# Conflicts:
> +		#	foo
> +		EOF
> +
> +	test_must_fail git revert --cleanup=scissors picked &&
> +
> +	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
> +	test_i18ncmp expected actual
> +'
> +
>   test_expect_success 'failed cherry-pick does not forget -s' '
>   	pristine_detach initial &&
>   	test_must_fail git cherry-pick -s picked &&
> 

Re: [PATCH v3 3/4] cherry-pick/revert: add scissors line on merge conflict

[Denton Liu]

Hi Phillip,

On Thu, Mar 07, 2019 at 03:24:16PM +0000, Phillip Wood wrote:
> Hi Denton
> 
> On 07/03/2019 09:58, Denton Liu wrote:
> >Fix a bug where the scissors line is placed after the Conflicts:
> >section, in the case where a merge conflict occurs and
> >commit.cleanup = scissors.
> >
> >Note that the removal of the if-else tower in git_sequencer_config may
> >appear to be a no-op refactor but it actually isn't. First of all, we
> >now accept "default". More importantly, though, if
> >commit.cleanup = scissors, the cleanup enum will be set to
> >COMMIT_MSG_CLEANUP_SCISSORS instead of COMMIT_MSG_CLEANUP_SPACE when we
> >are reverting or cherry-picking. This allows us to append scissors to
> >MERGE_MSG in the case of a conflict.
> >
> >Signed-off-by: Denton Liu <liu.denton@gmail.com>
> >---
> >  Documentation/git-cherry-pick.txt |  7 +++
> >  Documentation/git-revert.txt      |  7 +++
> >  builtin/merge.c                   | 13 ++---
> >  builtin/rebase--helper.c          |  2 +-
> >  builtin/revert.c                  |  5 ++
> >  sequencer.c                       | 32 +++++-------
> >  sequencer.h                       |  3 +-
> >  t/t3507-cherry-pick-conflict.sh   | 86 +++++++++++++++++++++++++++++++
> >  8 files changed, 125 insertions(+), 30 deletions(-)
> >
> >diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> >index d35d771fc8..92235bd1c4 100644
> >--- a/Documentation/git-cherry-pick.txt
> >+++ b/Documentation/git-cherry-pick.txt
> >@@ -57,6 +57,13 @@ OPTIONS
> >  	With this option, 'git cherry-pick' will let you edit the commit
> >  	message prior to committing.
> >+--cleanup=<mode>::
> >+	This option determines how the commit message will be cleaned up before
> >+	being passed on. See linkgit:git-commit[1] for more details. In
> >+	addition, if the '<mode>' is given a value of `scissors`, scissors will
> >+	be appended to MERGE_MSG before being passed on in the case of a
> >+	conflict.
> >+
> >  -x::
> >  	When recording the commit, append a line that says
> >  	"(cherry picked from commit ...)" to the original commit
> >diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
> >index 837707a8fd..bd4ad395a9 100644
> >--- a/Documentation/git-revert.txt
> >+++ b/Documentation/git-revert.txt
> >@@ -66,6 +66,13 @@ more details.
> >  	With this option, 'git revert' will not start the commit
> >  	message editor.
> >+--cleanup=<mode>::
> >+	This option determines how the commit message will be cleaned up before
> >+	being passed on. See linkgit:git-commit[1] for more details. In
> >+	addition, if the '<mode>' is given a value of `scissors`, scissors will
> >+	be appended to MERGE_MSG before being passed on in the case of a
> >+	conflict.
> >+
> >  -n::
> >  --no-commit::
> >  	Usually the command automatically creates some commits with
> >diff --git a/builtin/merge.c b/builtin/merge.c
> >index 92efc3d8fa..d4217ebcf5 100644
> >--- a/builtin/merge.c
> >+++ b/builtin/merge.c
> >@@ -913,17 +913,10 @@ static int suggest_conflicts(void)
> >  	 * We can't use cleanup_mode because if we're not using the editor,
> >  	 * get_cleanup_mode will return COMMIT_MSG_CLEANUP_SPACE instead, even
> >  	 * though the message is meant to be processed later by git-commit.
> >-	 * Thus, we will get the cleanup mode is returned we _are_ using an
> >-	 * editor.
> >+	 * Thus, we will get the cleanup mode which is returned when we _are_ using
> >+	 * an editor.
> >  	 */
> >-	if (get_cleanup_mode(cleanup_arg, 1) == COMMIT_MSG_CLEANUP_SCISSORS) {
> >-	    fputc('\n', fp);
> >-	    wt_status_add_cut_line(fp);
> >-	    /* comments out the newline from append_conflicts_hint */
> >-	    fputc(comment_line_char, fp);
> >-	}
> >-
> >-	append_conflicts_hint(&msgbuf);
> >+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
> >  	fputs(msgbuf.buf, fp);
> >  	strbuf_release(&msgbuf);
> >  	fclose(fp);
> >diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> >index f7c2a5fdc8..5f1bc9d383 100644
> >--- a/builtin/rebase--helper.c
> >+++ b/builtin/rebase--helper.c
> >@@ -48,10 +48,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> >  		OPT_END()
> >  	};
> >+	opts.action = REPLAY_INTERACTIVE_REBASE;
> >  	sequencer_init_config(&opts);
> >  	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
> >-	opts.action = REPLAY_INTERACTIVE_REBASE;
> >  	opts.allow_ff = 1;
> >  	opts.allow_empty = 1;
> 
> 
> What are you basing this series on? builtin/rebase--helper.c was removed
> last September in 34b47315d9 ("rebase -i: move rebase--helper modes to
> rebase--interactive", 2018-09-27)

I was basing this patch on the tip of dl/merge-cleanup-scissors-fix. I
can rebase or merge my work to something else but I'll wait for
additional directions since I don't know what would be best.

> 
> >diff --git a/builtin/revert.c b/builtin/revert.c
> >index 9a66720cfc..fe18036be7 100644
> >--- a/builtin/revert.c
> >+++ b/builtin/revert.c
> >@@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> >  {
> >  	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
> >  	const char *me = action_name(opts);
> >+	const char *cleanup_arg = NULL;
> >  	int cmd = 0;
> >  	struct option base_options[] = {
> >  		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> >  		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> >  		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> >+		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
> >  		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
> >  		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
> >  		OPT_NOOP_NOARG('r', NULL),
> >@@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> >  	if (opts->keep_redundant_commits)
> >  		opts->allow_empty = 1;
> >+	if (cleanup_arg)
> >+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> >+
> >  	/* Check for incompatible command line arguments */
> >  	if (cmd) {
> >  		char *this_operation;
> >diff --git a/sequencer.c b/sequencer.c
> >index 707e72fb39..5c04bae7ac 100644
> >--- a/sequencer.c
> >+++ b/sequencer.c
> >@@ -153,6 +153,11 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
> >  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
> >  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
> >+static inline int is_rebase_i(const struct replay_opts *opts)
> >+{
> >+	return opts->action == REPLAY_INTERACTIVE_REBASE;
> >+}
> >+
> 
> This is already in sequencer.c in a different place in recent versions

I was just moving this function from below here so that the compiler
would pick it up.

> 
> Best Wishes
> 
> Phillip
> 
> >  static int git_sequencer_config(const char *k, const char *v, void *cb)
> >  {
> >  	struct replay_opts *opts = cb;
> >@@ -165,17 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
> >  		if (status)
> >  			return status;
> >-		if (!strcmp(s, "verbatim"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
> >-		else if (!strcmp(s, "whitespace"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> >-		else if (!strcmp(s, "strip"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
> >-		else if (!strcmp(s, "scissors"))
> >-			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
> >-		else
> >-			warning(_("invalid commit message cleanup mode '%s'"),
> >-				  s);
> >+		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
> >  		free((char *)s);
> >  		return status;
> >@@ -199,11 +194,6 @@ void sequencer_init_config(struct replay_opts *opts)
> >  	git_config(git_sequencer_config, opts);
> >  }
> >-static inline int is_rebase_i(const struct replay_opts *opts)
> >-{
> >-	return opts->action == REPLAY_INTERACTIVE_REBASE;
> >-}
> >-
> >  static const char *get_dir(const struct replay_opts *opts)
> >  {
> >  	if (is_rebase_i(opts))
> >@@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> >  		die(_("Invalid cleanup mode %s"), cleanup_arg);
> >  }
> >-void append_conflicts_hint(struct strbuf *msgbuf)
> >+void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
> >  {
> >  	int i;
> >+	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> >+		strbuf_addch(msgbuf, '\n');
> >+		wt_status_append_cut_line(msgbuf);
> >+		strbuf_addch(msgbuf, comment_line_char);
> >+	}
> >+
> >  	strbuf_addch(msgbuf, '\n');
> >  	strbuf_commented_addf(msgbuf, "Conflicts:\n");
> >  	for (i = 0; i < active_nr;) {
> >@@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >  			_(action_name(opts)));
> >  	if (!clean)
> >-		append_conflicts_hint(msgbuf);
> >+		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
> >  	return !clean;
> >  }
> >diff --git a/sequencer.h b/sequencer.h
> >index 5690e0c27e..aa99503dd7 100644
> >--- a/sequencer.h
> >+++ b/sequencer.h
> >@@ -91,7 +91,8 @@ int rearrange_squash(void);
> >  extern const char sign_off_header[];
> >  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> >-void append_conflicts_hint(struct strbuf *msgbuf);
> >+void append_conflicts_hint(struct strbuf *msgbuf,
> >+		enum commit_msg_cleanup_mode cleanup_mode);
> >  enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> >  	int use_editor);
> >diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> >index 74ff925526..c3894ca9d6 100755
> >--- a/t/t3507-cherry-pick-conflict.sh
> >+++ b/t/t3507-cherry-pick-conflict.sh
> >@@ -189,6 +189,46 @@ test_expect_success 'failed cherry-pick registers participants in index' '
> >  	test_cmp expected actual
> >  '
> >+test_expect_success \
> >+	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config commit.cleanup scissors &&
> >+	cat <<-EOF >expected &&
> >+		picked
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git cherry-pick picked &&
> >+
> >+	test_i18ncmp expected .git/MERGE_MSG
> >+'
> >+
> >+test_expect_success \
> >+	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config --unset commit.cleanup &&
> >+	cat <<-EOF >expected &&
> >+		picked
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git cherry-pick --cleanup=scissors picked &&
> >+
> >+	test_i18ncmp expected .git/MERGE_MSG
> >+'
> >+
> >  test_expect_success 'failed cherry-pick describes conflict in work tree' '
> >  	pristine_detach initial &&
> >  	cat <<-EOF >expected &&
> >@@ -335,6 +375,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
> >  	test_cmp expected actual
> >  '
> >+test_expect_success \
> >+	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config commit.cleanup scissors &&
> >+	cat >expected <<-EOF &&
> >+		Revert "picked"
> >+
> >+		This reverts commit OBJID.
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git revert picked &&
> >+
> >+	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >+test_expect_success \
> >+	'revert conflict, ensure cleanup=scissors places scissors line properly' '
> >+	pristine_detach initial &&
> >+	git config --unset commit.cleanup &&
> >+	cat >expected <<-EOF &&
> >+		Revert "picked"
> >+
> >+		This reverts commit OBJID.
> >+
> >+		# ------------------------ >8 ------------------------
> >+		# Do not modify or remove the line above.
> >+		# Everything below it will be ignored.
> >+		#
> >+		# Conflicts:
> >+		#	foo
> >+		EOF
> >+
> >+	test_must_fail git revert --cleanup=scissors picked &&
> >+
> >+	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
> >+	test_i18ncmp expected actual
> >+'
> >+
> >  test_expect_success 'failed cherry-pick does not forget -s' '
> >  	pristine_detach initial &&
> >  	test_must_fail git cherry-pick -s picked &&
> >

Thanks,

Denton

Re: [PATCH v3 3/4] cherry-pick/revert: add scissors line on merge conflict

[Phillip Wood]

On 07/03/2019 17:56, Denton Liu wrote:
> Hi Phillip,
> 
> On Thu, Mar 07, 2019 at 03:24:16PM +0000, Phillip Wood wrote:
>> Hi Denton
>>
>> On 07/03/2019 09:58, Denton Liu wrote:
>>> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
>>> index f7c2a5fdc8..5f1bc9d383 100644
>>> --- a/builtin/rebase--helper.c
>>> +++ b/builtin/rebase--helper.c
>>> @@ -48,10 +48,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>>>  		OPT_END()
>>>  	};
>>> +	opts.action = REPLAY_INTERACTIVE_REBASE;
>>>  	sequencer_init_config(&opts);
>>>  	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
>>> -	opts.action = REPLAY_INTERACTIVE_REBASE;
>>>  	opts.allow_ff = 1;
>>>  	opts.allow_empty = 1;
>>
>>
>> What are you basing this series on? builtin/rebase--helper.c was removed
>> last September in 34b47315d9 ("rebase -i: move rebase--helper modes to
>> rebase--interactive", 2018-09-27)
> 
> I was basing this patch on the tip of dl/merge-cleanup-scissors-fix. I
> can rebase or merge my work to something else but I'll wait for
> additional directions since I don't know what would be best.

See what Junio says, I think it might be simple enough for him to fix
that up when he merges it into pu.


I've just realized that if you're cherry-picking a range of commits and
it stops for a conflict resolution then the new option is not saved.
You'll need to update populate_opts_cb() and save_opts() in sequencer.c
to do that.

Best Wishes

Phillip


>>> diff --git a/builtin/revert.c b/builtin/revert.c
>>> index 9a66720cfc..fe18036be7 100644
>>> --- a/builtin/revert.c
>>> +++ b/builtin/revert.c
>>> @@ -95,11 +95,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>>>  {
>>>  	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
>>>  	const char *me = action_name(opts);
>>> +	const char *cleanup_arg = NULL;
>>>  	int cmd = 0;
>>>  	struct option base_options[] = {
>>>  		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
>>>  		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
>>>  		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
>>> +		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
>>>  		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
>>>  		OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
>>>  		OPT_NOOP_NOARG('r', NULL),
>>> @@ -136,6 +138,9 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>>>  	if (opts->keep_redundant_commits)
>>>  		opts->allow_empty = 1;
>>> +	if (cleanup_arg)
>>> +		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
>>> +
>>>  	/* Check for incompatible command line arguments */
>>>  	if (cmd) {
>>>  		char *this_operation;
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 707e72fb39..5c04bae7ac 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -153,6 +153,11 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy")
>>>  static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
>>>  static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
>>> +static inline int is_rebase_i(const struct replay_opts *opts)
>>> +{
>>> +	return opts->action == REPLAY_INTERACTIVE_REBASE;
>>> +}
>>> +
>>
>> This is already in sequencer.c in a different place in recent versions
> 
> I was just moving this function from below here so that the compiler
> would pick it up.
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>>>  static int git_sequencer_config(const char *k, const char *v, void *cb)
>>>  {
>>>  	struct replay_opts *opts = cb;
>>> @@ -165,17 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
>>>  		if (status)
>>>  			return status;
>>> -		if (!strcmp(s, "verbatim"))
>>> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;
>>> -		else if (!strcmp(s, "whitespace"))
>>> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
>>> -		else if (!strcmp(s, "strip"))
>>> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;
>>> -		else if (!strcmp(s, "scissors"))
>>> -			opts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;
>>> -		else
>>> -			warning(_("invalid commit message cleanup mode '%s'"),
>>> -				  s);
>>> +		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
>>>  		free((char *)s);
>>>  		return status;
>>> @@ -199,11 +194,6 @@ void sequencer_init_config(struct replay_opts *opts)
>>>  	git_config(git_sequencer_config, opts);
>>>  }
>>> -static inline int is_rebase_i(const struct replay_opts *opts)
>>> -{
>>> -	return opts->action == REPLAY_INTERACTIVE_REBASE;
>>> -}
>>> -
>>>  static const char *get_dir(const struct replay_opts *opts)
>>>  {
>>>  	if (is_rebase_i(opts))
>>> @@ -516,10 +506,16 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>>>  		die(_("Invalid cleanup mode %s"), cleanup_arg);
>>>  }
>>> -void append_conflicts_hint(struct strbuf *msgbuf)
>>> +void append_conflicts_hint(struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode)
>>>  {
>>>  	int i;
>>> +	if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
>>> +		strbuf_addch(msgbuf, '\n');
>>> +		wt_status_append_cut_line(msgbuf);
>>> +		strbuf_addch(msgbuf, comment_line_char);
>>> +	}
>>> +
>>>  	strbuf_addch(msgbuf, '\n');
>>>  	strbuf_commented_addf(msgbuf, "Conflicts:\n");
>>>  	for (i = 0; i < active_nr;) {
>>> @@ -586,7 +582,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>>>  			_(action_name(opts)));
>>>  	if (!clean)
>>> -		append_conflicts_hint(msgbuf);
>>> +		append_conflicts_hint(msgbuf, opts->default_msg_cleanup);
>>>  	return !clean;
>>>  }
>>> diff --git a/sequencer.h b/sequencer.h
>>> index 5690e0c27e..aa99503dd7 100644
>>> --- a/sequencer.h
>>> +++ b/sequencer.h
>>> @@ -91,7 +91,8 @@ int rearrange_squash(void);
>>>  extern const char sign_off_header[];
>>>  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
>>> -void append_conflicts_hint(struct strbuf *msgbuf);
>>> +void append_conflicts_hint(struct strbuf *msgbuf,
>>> +		enum commit_msg_cleanup_mode cleanup_mode);
>>>  enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>>>  	int use_editor);
>>> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
>>> index 74ff925526..c3894ca9d6 100755
>>> --- a/t/t3507-cherry-pick-conflict.sh
>>> +++ b/t/t3507-cherry-pick-conflict.sh
>>> @@ -189,6 +189,46 @@ test_expect_success 'failed cherry-pick registers participants in index' '
>>>  	test_cmp expected actual
>>>  '
>>> +test_expect_success \
>>> +	'cherry-pick conflict, ensure commit.cleanup = scissors places scissors line properly' '
>>> +	pristine_detach initial &&
>>> +	git config commit.cleanup scissors &&
>>> +	cat <<-EOF >expected &&
>>> +		picked
>>> +
>>> +		# ------------------------ >8 ------------------------
>>> +		# Do not modify or remove the line above.
>>> +		# Everything below it will be ignored.
>>> +		#
>>> +		# Conflicts:
>>> +		#	foo
>>> +		EOF
>>> +
>>> +	test_must_fail git cherry-pick picked &&
>>> +
>>> +	test_i18ncmp expected .git/MERGE_MSG
>>> +'
>>> +
>>> +test_expect_success \
>>> +	'cherry-pick conflict, ensure cleanup=scissors places scissors line properly' '
>>> +	pristine_detach initial &&
>>> +	git config --unset commit.cleanup &&
>>> +	cat <<-EOF >expected &&
>>> +		picked
>>> +
>>> +		# ------------------------ >8 ------------------------
>>> +		# Do not modify or remove the line above.
>>> +		# Everything below it will be ignored.
>>> +		#
>>> +		# Conflicts:
>>> +		#	foo
>>> +		EOF
>>> +
>>> +	test_must_fail git cherry-pick --cleanup=scissors picked &&
>>> +
>>> +	test_i18ncmp expected .git/MERGE_MSG
>>> +'
>>> +
>>>  test_expect_success 'failed cherry-pick describes conflict in work tree' '
>>>  	pristine_detach initial &&
>>>  	cat <<-EOF >expected &&
>>> @@ -335,6 +375,52 @@ test_expect_success 'revert conflict, diff3 -m style' '
>>>  	test_cmp expected actual
>>>  '
>>> +test_expect_success \
>>> +	'revert conflict, ensure commit.cleanup = scissors places scissors line properly' '
>>> +	pristine_detach initial &&
>>> +	git config commit.cleanup scissors &&
>>> +	cat >expected <<-EOF &&
>>> +		Revert "picked"
>>> +
>>> +		This reverts commit OBJID.
>>> +
>>> +		# ------------------------ >8 ------------------------
>>> +		# Do not modify or remove the line above.
>>> +		# Everything below it will be ignored.
>>> +		#
>>> +		# Conflicts:
>>> +		#	foo
>>> +		EOF
>>> +
>>> +	test_must_fail git revert picked &&
>>> +
>>> +	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
>>> +	test_i18ncmp expected actual
>>> +'
>>> +
>>> +test_expect_success \
>>> +	'revert conflict, ensure cleanup=scissors places scissors line properly' '
>>> +	pristine_detach initial &&
>>> +	git config --unset commit.cleanup &&
>>> +	cat >expected <<-EOF &&
>>> +		Revert "picked"
>>> +
>>> +		This reverts commit OBJID.
>>> +
>>> +		# ------------------------ >8 ------------------------
>>> +		# Do not modify or remove the line above.
>>> +		# Everything below it will be ignored.
>>> +		#
>>> +		# Conflicts:
>>> +		#	foo
>>> +		EOF
>>> +
>>> +	test_must_fail git revert --cleanup=scissors picked &&
>>> +
>>> +	sed "s/$OID_REGEX/OBJID/" .git/MERGE_MSG >actual &&
>>> +	test_i18ncmp expected actual
>>> +'
>>> +
>>>  test_expect_success 'failed cherry-pick does not forget -s' '
>>>  	pristine_detach initial &&
>>>  	test_must_fail git cherry-pick -s picked &&
>>>
> 
> Thanks,
> 
> Denton
> 

Re: [PATCH v3 3/4] cherry-pick/revert: add scissors line on merge conflict

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>> What are you basing this series on? builtin/rebase--helper.c was removed
>>> last September in 34b47315d9 ("rebase -i: move rebase--helper modes to
>>> rebase--interactive", 2018-09-27)
>> 
>> I was basing this patch on the tip of dl/merge-cleanup-scissors-fix. I
>> can rebase or merge my work to something else but I'll wait for
>> additional directions since I don't know what would be best.
>
> See what Junio says, I think it might be simple enough for him to fix
> that up when he merges it into pu.

Perhaps it is a good time to kick dl/merge-cleanup-scissors-fix out
of the 'next' branch, so that the whole thing can be rebuilt on top
of a more recent base like v2.21.0, without the need for these
"oops, that was wrong, so let's patch it up" fixes.

> I've just realized that if you're cherry-picking a range of commits and
> it stops for a conflict resolution then the new option is not saved.
> You'll need to update populate_opts_cb() and save_opts() in sequencer.c
> to do that.

Yeah, good point.

[PATCH v3 4/4] sequencer.c: don't die on invalid cleanup_arg

[Denton Liu]

When get_cleanup_mode was provided with an invalid cleanup_arg, it used
to die. Warn user and fallback to default behaviour if an invalid
cleanup_arg is given.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/commit.c |  2 +-
 builtin/merge.c  |  4 ++--
 builtin/revert.c |  2 +-
 sequencer.c      | 10 +++++++---
 sequencer.h      |  2 +-
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 43291d79bd..0072a5817a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1167,7 +1167,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend && !allow_empty)))
 		die(_("No paths with --include/--only does not make sense."));
-	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
+	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor, 1);
 
 	handle_untracked_files_arg(s);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index d4217ebcf5..3b597ec540 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -916,7 +916,7 @@ static int suggest_conflicts(void)
 	 * Thus, we will get the cleanup mode which is returned when we _are_ using
 	 * an editor.
 	 */
-	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
+	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1, 1));
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
 	fclose(fp);
@@ -1424,7 +1424,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (option_edit < 0)
 		option_edit = default_edit_option();
 
-	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit);
+	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit, 1);
 
 	if (!use_strategies) {
 		if (!remoteheads)
diff --git a/builtin/revert.c b/builtin/revert.c
index fe18036be7..a96f2ecd8a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -139,7 +139,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		opts->allow_empty = 1;
 
 	if (cleanup_arg)
-		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
+		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1, 1);
 
 	/* Check for incompatible command line arguments */
 	if (cmd) {
diff --git a/sequencer.c b/sequencer.c
index 5c04bae7ac..7d18c55223 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -170,7 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 		if (status)
 			return status;
 
-		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
+		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts), 0);
 
 		free((char *)s);
 		return status;
@@ -488,7 +488,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
 }
 
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
-	int use_editor)
+	int use_editor, int die_on_error)
 {
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
 		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
@@ -502,7 +502,11 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
 	else if (!strcmp(cleanup_arg, "scissors"))
 		return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
 				    COMMIT_MSG_CLEANUP_SPACE;
-	else
+	else if (!die_on_error) {
+		warning(_("Invalid cleanup mode %s, falling back to default"), cleanup_arg);
+		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
+				    COMMIT_MSG_CLEANUP_SPACE;
+	} else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 }
 
diff --git a/sequencer.h b/sequencer.h
index aa99503dd7..c4c80051ea 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -94,7 +94,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
 void append_conflicts_hint(struct strbuf *msgbuf,
 		enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
-	int use_editor);
+	int use_editor, int die_on_error);
 
 void cleanup_message(struct strbuf *msgbuf,
 	enum commit_msg_cleanup_mode cleanup_mode, int verbose);
-- 
2.21.0.370.g4fdb13b891

Re: [PATCH v3 4/4] sequencer.c: don't die on invalid cleanup_arg

[Phillip Wood]

Hi Denton

Thanks for doing this, I think it looks ok but it would be better to 
make these changes earlier in the series so they come before what is now 
patch 3

Best Wishes

Phillip

On 07/03/2019 09:58, Denton Liu wrote:
> When get_cleanup_mode was provided with an invalid cleanup_arg, it used
> to die. Warn user and fallback to default behaviour if an invalid
> cleanup_arg is given.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   builtin/commit.c |  2 +-
>   builtin/merge.c  |  4 ++--
>   builtin/revert.c |  2 +-
>   sequencer.c      | 10 +++++++---
>   sequencer.h      |  2 +-
>   5 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 43291d79bd..0072a5817a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1167,7 +1167,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>   		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
>   	if (argc == 0 && (also || (only && !amend && !allow_empty)))
>   		die(_("No paths with --include/--only does not make sense."));
> -	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor);
> +	cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor, 1);
>   
>   	handle_untracked_files_arg(s);
>   
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d4217ebcf5..3b597ec540 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -916,7 +916,7 @@ static int suggest_conflicts(void)
>   	 * Thus, we will get the cleanup mode which is returned when we _are_ using
>   	 * an editor.
>   	 */
> -	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1));
> +	append_conflicts_hint(&msgbuf, get_cleanup_mode(cleanup_arg, 1, 1));
>   	fputs(msgbuf.buf, fp);
>   	strbuf_release(&msgbuf);
>   	fclose(fp);
> @@ -1424,7 +1424,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>   	if (option_edit < 0)
>   		option_edit = default_edit_option();
>   
> -	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit);
> +	cleanup_mode = get_cleanup_mode(cleanup_arg, 0 < option_edit, 1);
>   
>   	if (!use_strategies) {
>   		if (!remoteheads)
> diff --git a/builtin/revert.c b/builtin/revert.c
> index fe18036be7..a96f2ecd8a 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -139,7 +139,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>   		opts->allow_empty = 1;
>   
>   	if (cleanup_arg)
> -		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> +		opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1, 1);
>   
>   	/* Check for incompatible command line arguments */
>   	if (cmd) {
> diff --git a/sequencer.c b/sequencer.c
> index 5c04bae7ac..7d18c55223 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -170,7 +170,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
>   		if (status)
>   			return status;
>   
> -		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts));
> +		opts->default_msg_cleanup = get_cleanup_mode(s, !is_rebase_i(opts), 0);
>   
>   		free((char *)s);
>   		return status;
> @@ -488,7 +488,7 @@ static int fast_forward_to(const struct object_id *to, const struct object_id *f
>   }
>   
>   enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> -	int use_editor)
> +	int use_editor, int die_on_error)
>   {
>   	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
>   		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
> @@ -502,7 +502,11 @@ enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
>   	else if (!strcmp(cleanup_arg, "scissors"))
>   		return use_editor ? COMMIT_MSG_CLEANUP_SCISSORS :
>   				    COMMIT_MSG_CLEANUP_SPACE;
> -	else
> +	else if (!die_on_error) {
> +		warning(_("Invalid cleanup mode %s, falling back to default"), cleanup_arg);
> +		return use_editor ? COMMIT_MSG_CLEANUP_ALL :
> +				    COMMIT_MSG_CLEANUP_SPACE;
> +	} else
>   		die(_("Invalid cleanup mode %s"), cleanup_arg);
>   }
>   
> diff --git a/sequencer.h b/sequencer.h
> index aa99503dd7..c4c80051ea 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -94,7 +94,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
>   void append_conflicts_hint(struct strbuf *msgbuf,
>   		enum commit_msg_cleanup_mode cleanup_mode);
>   enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
> -	int use_editor);
> +	int use_editor, int die_on_error);
>   
>   void cleanup_message(struct strbuf *msgbuf,
>   	enum commit_msg_cleanup_mode cleanup_mode, int verbose);
>