patchrebase -i: improve error message when picking merge

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The only todo commands that accept a merge commit are "merge" and
> "reset". All the other commands like "pick" or "reword" fail when they
> try to pick a a merge commit and print the message
> 
>      error: commit abc123 is a merge but no -m option was given.
> 
> followed by a hint about the command being rescheduled. This message is
> designed to help the user when they cherry-pick a merge and forget to
> pass "-m". For users who are rebasing the message is confusing as there
> is no way for rebase to cherry-pick the merge.
> 
> Improve the user experience by detecting the error when the todo list is
> parsed rather than waiting for the "pick" command to fail and print a
> message recommending the "merge" command instead. We recommend "merge"
> rather than "exec git cherry-pick -m ..." on the assumption that
> cherry-picking merges is relatively rare and it is more likely that the
> user chose "pick" by a mistake.
> 
> It would be possible to support cherry-picking merges by allowing the
> user to pass "-m" to "pick" commands but that adds complexity to do
> something that can already be achieved with
> 
>      exec git cherry-pick -m1 abc123
> 
> The change is relatively straight forward but is complicated slightly as
> we now need to tell the parser if we're rebasing or not.
> 
> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>      rebase -i: improve error message when picking merge
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1672
> 
>   builtin/rebase.c              |  2 +-
>   rebase-interactive.c          |  7 ++---
>   sequencer.c                   | 49 ++++++++++++++++++++++++++++++-----
>   sequencer.h                   |  2 +-
>   t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++
>   5 files changed, 81 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b086f651a6..a33e41c44da 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	else {
>   		discard_index(&the_index);
>   		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
> -						&todo_list))
> +						&todo_list, 1))
>   			BUG("unusable todo list");
>   
>   		ret = complete_action(the_repository, &replay, flags,
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index d9718409b3d..78d5ed1a41d 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>   	 * it.  If there is an error, we do not return, because the user
>   	 * might want to fix it in the first place. */
>   	if (!initial)
> -		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
> +		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,
> +							todo_list, 1) |
>   			file_exists(rebase_path_dropped());
>   
>   	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> @@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>   	if (initial && new_todo->buf.len == 0)
>   		return -3;
>   
> -	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> +	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {
>   		fprintf(stderr, _(edit_todo_list_advice));
>   		return -4;
>   	}
> @@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_
>   	int res = 0;
>   
>   	if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
> -		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
> +		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);
>   		res = todo_list_check(&backup, todo_list);
>   	}
>   
> diff --git a/sequencer.c b/sequencer.c
> index 91de546b323..cf808c24d20 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>   	return 0;
>   }
>   
> +static int error_merge_commit(enum todo_command command)
> +{
> +	switch(command) {
> +	case TODO_PICK:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -C");
> +
> +	case TODO_REWORD:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -c");
> +
> +	case TODO_EDIT:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s' followed by '%s'"),
> +			     todo_command_info[command].str,
> +			     "merge -C", "break");
> +
> +	case TODO_FIXUP:
> +	case TODO_SQUASH:
> +		return error(_("cannot squash merge commit into another commit"));
> +
> +	default:
> +		BUG("unexpected todo_command");
> +	}
> +}
> +
>   static int parse_insn_line(struct repository *r, struct todo_item *item,
> -			   const char *buf, const char *bol, char *eol)
> +			   const char *buf, const char *bol, char *eol,
> +			   int rebasing)
>   {
>   	struct object_id commit_oid;
>   	char *end_of_object_name;
> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>   		return status;
>   
>   	item->commit = lookup_commit_reference(r, &commit_oid);
> -	return item->commit ? 0 : -1;
> +	if (!item->commit)
> +		return -1;
> +	if (rebasing && item->command != TODO_MERGE &&
> +	    item->commit->parents && item->commit->parents->next)
> +		return error_merge_commit(item->command);
> +	return 0;
>   }
>   
>   int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
>   }
>   
>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list)
> +				struct todo_list *todo_list, int rebasing)
>   {
>   	struct todo_item *item;
>   	char *p = buf, *next_p;
> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>   
>   		item = append_new_todo(todo_list);
>   		item->offset_in_buf = p - todo_list->buf.buf;
> -		if (parse_insn_line(r, item, buf, p, eol)) {
> +		if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
>   			res = error(_("invalid line %d: %.*s"),
>   				i, (int)(eol - p), p);
>   			item->command = TODO_COMMENT + 1;
> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
>   	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>   		return -1;
>   
> -	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
> +	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
> +					  is_rebase_i(opts));
>   	if (res) {
>   		if (is_rebase_i(opts))
>   			return error(_("please fix this using "
> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
>   		struct todo_list done = TODO_LIST_INIT;
>   
>   		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
> -		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
> +		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
>   			todo_list->done_nr = count_commands(&done);
>   		else
>   			todo_list->done_nr = 0;
> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>   	strbuf_release(&buf2);
>   	/* Nothing is done yet, and we're reparsing, so let's reset the count */
>   	new_todo.total_nr = 0;
> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
>   		BUG("invalid todo list after expanding IDs:\n%s",
>   		    new_todo.buf.buf);
>   
> diff --git a/sequencer.h b/sequencer.h
> index dcef7bb99c0..ed2c4b38514 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -136,7 +136,7 @@ struct todo_list {
>   }
>   
>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list);
> +				struct todo_list *todo_list, int rebasing);
>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>   			    const char *file, const char *shortrevisions,
>   			    const char *shortonto, int num, unsigned flags);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 64b641002e4..20b8589ad07 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
>   	test_path_is_missing execed
>   '
>   
> +test_expect_success 'non-merge commands reject merge commits' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	git checkout E &&
> +	git merge I &&
> +	oid=$(git rev-parse HEAD) &&
> +	cat >todo <<-EOF &&
> +	pick $oid
> +	reword $oid
> +	edit $oid
> +	fixup $oid
> +	squash $oid
> +	EOF
> +	(
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i HEAD 2>actual
> +	) &&
> +	cat >expect <<-EOF &&
> +	error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
> +	error: invalid line 1: pick $oid
> +	error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
> +	error: invalid line 2: reword $oid
> +	error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
> +	error: invalid line 3: edit $oid
> +	error: cannot squash merge commit into another commit
> +	error: invalid line 4: fixup $oid
> +	error: cannot squash merge commit into another commit
> +	error: invalid line 5: squash $oid
> +	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
> +	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
> +	EOF
> +	test_cmp expect actual
> +'
> +
>   # This must be the last test in this file
>   test_expect_success '$EDITOR and friends are unchanged' '
>   	test_editor_unchanged
> 
> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c

> If anyone has time to review this I'd be very grateful. I've added a couple
> of people to the cc list who have recently contributed to the sequencer but
> if anyone else fancies taking a look please do.
> 
> Thanks
> 
> Phillip

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The only todo commands that accept a merge commit are "merge" and
> "reset". All the other commands like "pick" or "reword" fail when they
> try to pick a a merge commit and print the message
> 
>     error: commit abc123 is a merge but no -m option was given.
> 
> followed by a hint about the command being rescheduled. This message is
> designed to help the user when they cherry-pick a merge and forget to
> pass "-m". For users who are rebasing the message is confusing as there
> is no way for rebase to cherry-pick the merge.
> 
> Improve the user experience by detecting the error when the todo list is
> parsed rather than waiting for the "pick" command to fail and print a
> message recommending the "merge" command instead. We recommend "merge"
> rather than "exec git cherry-pick -m ..." on the assumption that
> cherry-picking merges is relatively rare and it is more likely that the
> user chose "pick" by a mistake.
> 
> It would be possible to support cherry-picking merges by allowing the
> user to pass "-m" to "pick" commands but that adds complexity to do
> something that can already be achieved with
> 
>     exec git cherry-pick -m1 abc123

> The change is relatively straight forward but is complicated slightly as
> we now need to tell the parser if we're rebasing or not.
> 
> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     rebase -i: improve error message when picking merge
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1672
> 
>  builtin/rebase.c              |  2 +-
>  rebase-interactive.c          |  7 ++---
>  sequencer.c                   | 49 ++++++++++++++++++++++++++++++-----
>  sequencer.h                   |  2 +-
>  t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++
>  5 files changed, 81 insertions(+), 12 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b086f651a6..a33e41c44da 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>  	else {
>  		discard_index(&the_index);
>  		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
> -						&todo_list))
> +						&todo_list, 1))

>  			BUG("unusable todo list");
>  
>  		ret = complete_action(the_repository, &replay, flags,
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index d9718409b3d..78d5ed1a41d 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -114,7 +114,8 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>  	 * it.  If there is an error, we do not return, because the user
>  	 * might want to fix it in the first place. */
>  	if (!initial)
> -		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
> +		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,
> +							todo_list, 1) |
>  			file_exists(rebase_path_dropped());
>  
>  	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> @@ -134,7 +135,7 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>  	if (initial && new_todo->buf.len == 0)
>  		return -3;
>  
> -	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> +	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {
>  		fprintf(stderr, _(edit_todo_list_advice));
>  		return -4;
>  	}
> @@ -234,7 +235,7 @@ int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_
>  	int res = 0;
>  
>  	if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
> -		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
> +		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);
>  		res = todo_list_check(&backup, todo_list);
>  	}
>  
> diff --git a/sequencer.c b/sequencer.c
> index 91de546b323..cf808c24d20 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2550,8 +2550,37 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>  	return 0;
>  }
>  
> +static int error_merge_commit(enum todo_command command)
> +{
> +	switch(command) {
> +	case TODO_PICK:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -C");

> +	case TODO_EDIT:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s' followed by '%s'"),
> +			     todo_command_info[command].str,
> +			     "merge -C", "break");
> +
> +	case TODO_FIXUP:
> +	case TODO_SQUASH:
> +		return error(_("cannot squash merge commit into another commit"));
> +
> +	default:
> +		BUG("unexpected todo_command");
> +	}
> +}
> +
>  static int parse_insn_line(struct repository *r, struct todo_item *item,
> -			   const char *buf, const char *bol, char *eol)
> +			   const char *buf, const char *bol, char *eol,
> +			   int rebasing)
>  {
>  	struct object_id commit_oid;
>  	char *end_of_object_name;
> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>  		return status;
>  
>  	item->commit = lookup_commit_reference(r, &commit_oid);
> -	return item->commit ? 0 : -1;
> +	if (!item->commit)
> +		return -1;
> +	if (rebasing && item->command != TODO_MERGE &&
> +	    item->commit->parents && item->commit->parents->next)
> +		return error_merge_commit(item->command);
> +	return 0;
>  }
>  
>  int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
>  }
>  
>  int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list)
> +				struct todo_list *todo_list, int rebasing)
>  {
>  	struct todo_item *item;
>  	char *p = buf, *next_p;
> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>  
>  		item = append_new_todo(todo_list);
>  		item->offset_in_buf = p - todo_list->buf.buf;
> -		if (parse_insn_line(r, item, buf, p, eol)) {
> +		if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
>  			res = error(_("invalid line %d: %.*s"),
>  				i, (int)(eol - p), p);
>  			item->command = TODO_COMMENT + 1;
> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
>  	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>  		return -1;
>  
> -	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
> +	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
> +					  is_rebase_i(opts));
>  	if (res) {
>  		if (is_rebase_i(opts))
>  			return error(_("please fix this using "
> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
>  		struct todo_list done = TODO_LIST_INIT;
>  
>  		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
> -		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
> +		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
>  			todo_list->done_nr = count_commands(&done);
>  		else
>  			todo_list->done_nr = 0;
> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  	strbuf_release(&buf2);
>  	/* Nothing is done yet, and we're reparsing, so let's reset the count */
>  	new_todo.total_nr = 0;
> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
>  		BUG("invalid todo list after expanding IDs:\n%s",
>  		    new_todo.buf.buf);
>  
> diff --git a/sequencer.h b/sequencer.h
> index dcef7bb99c0..ed2c4b38514 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -136,7 +136,7 @@ struct todo_list {
>  }
>  
>  int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list);
> +				struct todo_list *todo_list, int rebasing);
>  int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>  			    const char *file, const char *shortrevisions,
>  			    const char *shortonto, int num, unsigned flags);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 64b641002e4..20b8589ad07 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
>  	test_path_is_missing execed
>  '
>  
> +test_expect_success 'non-merge commands reject merge commits' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	git checkout E &&
> +	git merge I &&
> +	oid=$(git rev-parse HEAD) &&
> +	cat >todo <<-EOF &&
> +	pick $oid
> +	reword $oid
> +	edit $oid
> +	fixup $oid
> +	squash $oid
> +	EOF
> +	(
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i HEAD 2>actual
> +	) &&
> +	cat >expect <<-EOF &&
> +	error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
> +	error: invalid line 1: pick $oid
> +	error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
> +	error: invalid line 2: reword $oid
> +	error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
> +	error: invalid line 3: edit $oid
> +	error: cannot squash merge commit into another commit
> +	error: invalid line 4: fixup $oid
> +	error: cannot squash merge commit into another commit
> +	error: invalid line 5: squash $oid
> +	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
> +	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  # This must be the last test in this file
>  test_expect_success '$EDITOR and friends are unchanged' '
>  	test_editor_unchanged
> 
> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
> -- 
> gitgitgadget
> 

> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
> [...]
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 5b086f651a6..a33e41c44da 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>>   	else {
>>   		discard_index(&the_index);
>>   		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
>> -						&todo_list))
>> +						&todo_list, 1))
> 
> I think these booleans are somewhat hard to read. I may be
> overengineering this, so please feel free to push back, but would it
> make more sense to introduce a `unsigned flags` field and then have a
> `PARSE_INSN_IS_REBASING` flag?

>> +static int error_merge_commit(enum todo_command command)
>> +{
>> +	switch(command) {
>> +	case TODO_PICK:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s'"),
>> +			     todo_command_info[command].str, "merge -C");
> 
> I wonder how actionable these commands are. Can we give the full command
> that the user can use instead, including the commit ID?

> That raises another question though: how exactly is the user supposed to
> perform the merge? Should they merge the merge commit, resulting in two
> merge commits? Should they pick one of the sides, and if so, which one?
> I guess the answer is "it depends", which makes it harder for us to come
> up with actionable advice here.

>> +	case TODO_REWORD:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s'"),
>> +			     todo_command_info[command].str, "merge -c");
> 
> I was about to suggest that the above two cases should be merged. But
> then I realized that it's "merge -c" here, but "merge -C" in the first
> case.

> Patrick
> 
>> +	case TODO_EDIT:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s' followed by '%s'"),
>> +			     todo_command_info[command].str,
>> +			     "merge -C", "break");
>> +
>> +	case TODO_FIXUP:
>> +	case TODO_SQUASH:
>> +		return error(_("cannot squash merge commit into another commit"));
>> +
>> +	default:
>> +		BUG("unexpected todo_command");
>> +	}
>> +}
>> +
>>   static int parse_insn_line(struct repository *r, struct todo_item *item,
>> -			   const char *buf, const char *bol, char *eol)
>> +			   const char *buf, const char *bol, char *eol,
>> +			   int rebasing)
>>   {
>>   	struct object_id commit_oid;
>>   	char *end_of_object_name;
>> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   		return status;
>>   
>>   	item->commit = lookup_commit_reference(r, &commit_oid);
>> -	return item->commit ? 0 : -1;
>> +	if (!item->commit)
>> +		return -1;
>> +	if (rebasing && item->command != TODO_MERGE &&
>> +	    item->commit->parents && item->commit->parents->next)
>> +		return error_merge_commit(item->command);
>> +	return 0;
>>   }
>>   
>>   int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
>> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
>>   }
>>   
>>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>> -				struct todo_list *todo_list)
>> +				struct todo_list *todo_list, int rebasing)
>>   {
>>   	struct todo_item *item;
>>   	char *p = buf, *next_p;
>> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>>   
>>   		item = append_new_todo(todo_list);
>>   		item->offset_in_buf = p - todo_list->buf.buf;
>> -		if (parse_insn_line(r, item, buf, p, eol)) {
>> +		if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
>>   			res = error(_("invalid line %d: %.*s"),
>>   				i, (int)(eol - p), p);
>>   			item->command = TODO_COMMENT + 1;
>> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
>>   	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>>   		return -1;
>>   
>> -	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>> +	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
>> +					  is_rebase_i(opts));
>>   	if (res) {
>>   		if (is_rebase_i(opts))
>>   			return error(_("please fix this using "
>> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
>>   		struct todo_list done = TODO_LIST_INIT;
>>   
>>   		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
>> -		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
>> +		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
>>   			todo_list->done_nr = count_commands(&done);
>>   		else
>>   			todo_list->done_nr = 0;
>> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>>   	strbuf_release(&buf2);
>>   	/* Nothing is done yet, and we're reparsing, so let's reset the count */
>>   	new_todo.total_nr = 0;
>> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
>> +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
>>   		BUG("invalid todo list after expanding IDs:\n%s",
>>   		    new_todo.buf.buf);
>>   
>> diff --git a/sequencer.h b/sequencer.h
>> index dcef7bb99c0..ed2c4b38514 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -136,7 +136,7 @@ struct todo_list {
>>   }
>>   
>>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>> -				struct todo_list *todo_list);
>> +				struct todo_list *todo_list, int rebasing);
>>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>>   			    const char *file, const char *shortrevisions,
>>   			    const char *shortonto, int num, unsigned flags);
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 64b641002e4..20b8589ad07 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
>>   	test_path_is_missing execed
>>   '
>>   
>> +test_expect_success 'non-merge commands reject merge commits' '
>> +	test_when_finished "test_might_fail git rebase --abort" &&
>> +	git checkout E &&
>> +	git merge I &&
>> +	oid=$(git rev-parse HEAD) &&
>> +	cat >todo <<-EOF &&
>> +	pick $oid
>> +	reword $oid
>> +	edit $oid
>> +	fixup $oid
>> +	squash $oid
>> +	EOF
>> +	(
>> +		set_replace_editor todo &&
>> +		test_must_fail git rebase -i HEAD 2>actual
>> +	) &&
>> +	cat >expect <<-EOF &&
>> +	error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
>> +	error: invalid line 1: pick $oid
>> +	error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
>> +	error: invalid line 2: reword $oid
>> +	error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
>> +	error: invalid line 3: edit $oid
>> +	error: cannot squash merge commit into another commit
>> +	error: invalid line 4: fixup $oid
>> +	error: cannot squash merge commit into another commit
>> +	error: invalid line 5: squash $oid
>> +	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
>> +	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
>> +	EOF
>> +	test_cmp expect actual
>> +'
>> +
>>   # This must be the last test in this file
>>   test_expect_success '$EDITOR and friends are unchanged' '
>>   	test_editor_unchanged
>>
>> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
>> -- 
>> gitgitgadget
>>

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The only todo commands that accept a merge commit are "merge" and
> "reset". All the other commands like "pick" or "reword" fail when they
> try to pick a a merge commit and print the message
> 
>     error: commit abc123 is a merge but no -m option was given.
> 
> followed by a hint about the command being rescheduled. This message is
> designed to help the user when they cherry-pick a merge and forget to
> pass "-m". For users who are rebasing the message is confusing as there
> is no way for rebase to cherry-pick the merge.
> 
> Improve the user experience by detecting the error when the todo list is
> parsed rather than waiting for the "pick" command to fail and print a
> message recommending the "merge" command instead. We recommend "merge"
> rather than "exec git cherry-pick -m ..." on the assumption that
> cherry-picking merges is relatively rare and it is more likely that the
> user chose "pick" by a mistake.
> 
> It would be possible to support cherry-picking merges by allowing the
> user to pass "-m" to "pick" commands but that adds complexity to do
> something that can already be achieved with
> 
>     exec git cherry-pick -m1 abc123
> 
> The change is relatively straight forward but is complicated slightly as
> we now need to tell the parser if we're rebasing or not.
> 
> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
> The change makes sense, but this is confusing to me:
> 
> With this ...
> 
> 	$ GIT_EDITOR='echo pick 17381ab62a >' ./git rebase -i HEAD
> 	error: 'pick' does not accept merge commits, please use 'merge -C'
> 	error: invalid line 1: pick 17381ab62a
> 	You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
> 	Or you can abort the rebase with 'git rebase --abort'.
> 
> ... I find these repeated messages confusing:
> 
> 	$ GIT_EDITOR=: ./git rebase --edit-todo
> 	error: 'pick' does not accept merge commits, please use 'merge -C'
> 	error: invalid line 1: pick 17381ab62a

> Hi Rubén
> 
> Thanks for trying this out.
> 
> On 04/04/2024 20:44, Rubén Justo wrote:
> > On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
> > The change makes sense, but this is confusing to me:
> > 
> > With this ...
> > 
> > 	$ GIT_EDITOR='echo pick 17381ab62a >' ./git rebase -i HEAD
> > 	error: 'pick' does not accept merge commits, please use 'merge -C'
> > 	error: invalid line 1: pick 17381ab62a
> > 	You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
> > 	Or you can abort the rebase with 'git rebase --abort'.
> > 
> > ... I find these repeated messages confusing:
> > 
> > 	$ GIT_EDITOR=: ./git rebase --edit-todo
> > 	error: 'pick' does not accept merge commits, please use 'merge -C'
> > 	error: invalid line 1: pick 17381ab62a
> 
> The two lines above are printed when "rebase --edit-todo" loads the todo
> list for the user to edit. With a real editor rather than ":" we would then
> print
> 
>     hint: Waiting for your editor to close the file ...
> 
> Then hopefully the user would fix the errors. If not then when the editor
> had finished we'd print the remaining errors  as below.
> 
> > 	error: 'pick' does not accept merge commits, please use 'merge -C'
> > 	error: invalid line 1: pick 17381ab62a
> > 	You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
> > 	Or you can abort the rebase with 'git rebase --abort'.
> 
> I think that printing the error messages when the todo list is parsed before
> it is given to the user to edit is helpful as it reminds the user what the
> problem is. Your test looks confusing because it doesn't really simulate the
> user editing the todo list.

> On Fri, Apr 05, 2024 at 10:30:37AM +0100, phillip.wood123@gmail.com wrote:
>> On 04/04/2024 20:44, Rubén Justo wrote:
>>> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
>>> ... I find these repeated messages confusing:
>>>
>> I think that printing the error messages when the todo list is parsed before
>> it is given to the user to edit is helpful as it reminds the user what the
>> problem is. Your test looks confusing because it doesn't really simulate the
>> user editing the todo list.
> 
> Certainly, the test was not clear to express my confusion.
> 
> The ones that are printed _before_ the editor is run are the ones that
> confuse me, because when the user exits the editor we leave those
> lines there:
> 
> 	$ GIT_EDITOR='sed -i s/pick/merge/' ./git rebase --edit
> 	error: 'pick' does not accept merge commits, please use 'merge -C'
> 	error: invalid line 1: pick 17381ab62a
> 
> But maybe it is my interpretation.  Your reasoning of giving it as a
> help to the user makes sense.

> Thank you for your explanations.
> 
>> Best Wishes
>>
>> Phillip

> As adding the
> parameter requires quite a few changes to plumb it through the call
> chain these are separated into their own commit to avoid cluttering up
> the next commit with incidental changes.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c     | 17 +++++++++++------
>  rebase-interactive.c | 21 +++++++++++++--------
>  rebase-interactive.h |  9 ++++++---
>  sequencer.c          | 22 ++++++++++++----------
>  sequencer.h          |  4 ++--
>  5 files changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 891f28468e8..a33a2ed413a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -194,7 +194,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	return replay;
>  }
>  
> -static int edit_todo_file(unsigned flags)
> +static int edit_todo_file(unsigned flags, struct replay_opts *opts)
>  {
>  	const char *todo_file = rebase_path_todo();
>  	struct todo_list todo_list = TODO_LIST_INIT,
> @@ -205,7 +205,8 @@ static int edit_todo_file(unsigned flags)
>  		return error_errno(_("could not read '%s'."), todo_file);
>  
>  	strbuf_stripspace(&todo_list.buf, comment_line_str);
> -	res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
> +	res = edit_todo_list(the_repository, opts, &todo_list, &new_todo,
> +			     NULL, NULL, flags);
>  	if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
>  					    NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
>  		res = error_errno(_("could not write '%s'"), todo_file);
> @@ -296,8 +297,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>  		error(_("could not generate todo list"));
>  	else {
>  		discard_index(&the_index);
> -		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
> -						&todo_list))
> +		if (todo_list_parse_insn_buffer(the_repository, &replay,
> +						todo_list.buf.buf, &todo_list))
>  			BUG("unusable todo list");
>  
>  		ret = complete_action(the_repository, &replay, flags,
> @@ -352,9 +353,13 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>  		replay_opts_release(&replay_opts);
>  		break;
>  	}
> -	case ACTION_EDIT_TODO:
> -		ret = edit_todo_file(flags);
> +	case ACTION_EDIT_TODO: {
> +		struct replay_opts replay_opts = get_replay_opts(opts);
> +
> +		ret = edit_todo_file(flags, &replay_opts);
> +		replay_opts_release(&replay_opts);
>  		break;
> +	}
>  	case ACTION_SHOW_CURRENT_PATCH: {
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>  
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index c343e16fcdd..56fd7206a95 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -101,9 +101,10 @@ void append_todo_help(int command_count,
>  	strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
>  }
>  
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> -		   struct todo_list *new_todo, const char *shortrevisions,
> -		   const char *shortonto, unsigned flags)
> +int edit_todo_list(struct repository *r, struct replay_opts *opts,
> +		   struct todo_list *todo_list, struct todo_list *new_todo,
> +		   const char *shortrevisions, const char *shortonto,
> +		   unsigned flags)
>  {
>  	const char *todo_file = rebase_path_todo(),
>  		*todo_backup = rebase_path_todo_backup();
> @@ -114,7 +115,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>  	 * it.  If there is an error, we do not return, because the user
>  	 * might want to fix it in the first place. */
>  	if (!initial)
> -		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
> +		incorrect = todo_list_parse_insn_buffer(r, opts,
> +							todo_list->buf.buf,
> +							todo_list) |
>  			file_exists(rebase_path_dropped());
>  
>  	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> @@ -134,13 +137,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
>  	if (initial && new_todo->buf.len == 0)
>  		return -3;
>  
> -	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> +	if (todo_list_parse_insn_buffer(r, opts, new_todo->buf.buf, new_todo)) {
>  		fprintf(stderr, _(edit_todo_list_advice));
>  		return -4;
>  	}
>  
>  	if (incorrect) {
> -		if (todo_list_check_against_backup(r, new_todo)) {
> +		if (todo_list_check_against_backup(r, opts, new_todo)) {
>  			write_file(rebase_path_dropped(), "%s", "");
>  			return -4;
>  		}
> @@ -228,13 +231,15 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo)
>  	return res;
>  }
>  
> -int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_list)
> +int todo_list_check_against_backup(struct repository *r,
> +				   struct replay_opts *opts,
> +				   struct todo_list *todo_list)
>  {
>  	struct todo_list backup = TODO_LIST_INIT;
>  	int res = 0;
>  
>  	if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
> -		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
> +		todo_list_parse_insn_buffer(r, opts, backup.buf.buf, &backup);
>  		res = todo_list_check(&backup, todo_list);
>  	}
>  
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 7239c60f791..8e5b181b334 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -3,17 +3,20 @@
>  
>  struct strbuf;
>  struct repository;
> +struct replay_opts;
>  struct todo_list;
>  
>  void append_todo_help(int command_count,
>  		      const char *shortrevisions, const char *shortonto,
>  		      struct strbuf *buf);
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> -		   struct todo_list *new_todo, const char *shortrevisions,
> -		   const char *shortonto, unsigned flags);
> +int edit_todo_list(struct repository *r, struct replay_opts *opts,
> +		   struct todo_list *todo_list, struct todo_list *new_todo,
> +		   const char *shortrevisions, const char *shortonto,
> +		   unsigned flags);
>  
>  int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
>  int todo_list_check_against_backup(struct repository *r,
> +				   struct replay_opts *opts,
>  				   struct todo_list *todo_list);
>  
>  #endif
> diff --git a/sequencer.c b/sequencer.c
> index 2c19846385b..a3154ba3347 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2573,8 +2573,9 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>  	return 0;
>  }
>  
> -static int parse_insn_line(struct repository *r, struct todo_item *item,
> -			   const char *buf, const char *bol, char *eol)
> +static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,
> +			   struct todo_item *item, const char *buf,
> +			   const char *bol, char *eol)
>  {
>  	struct object_id commit_oid;
>  	char *end_of_object_name;
> @@ -2708,8 +2709,8 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
>  	return ret;
>  }
>  
> -int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list)
> +int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,
> +				char *buf, struct todo_list *todo_list)
>  {
>  	struct todo_item *item;
>  	char *p = buf, *next_p;
> @@ -2727,7 +2728,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>  
>  		item = append_new_todo(todo_list);
>  		item->offset_in_buf = p - todo_list->buf.buf;
> -		if (parse_insn_line(r, item, buf, p, eol)) {
> +		if (parse_insn_line(r, opts, item, buf, p, eol)) {
>  			res = error(_("invalid line %d: %.*s"),
>  				i, (int)(eol - p), p);
>  			item->command = TODO_COMMENT + 1;
> @@ -2875,7 +2876,7 @@ static int read_populate_todo(struct repository *r,
>  	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>  		return -1;
>  
> -	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
> +	res = todo_list_parse_insn_buffer(r, opts, todo_list->buf.buf, todo_list);
>  	if (res) {
>  		if (is_rebase_i(opts))
>  			return error(_("please fix this using "
> @@ -2905,7 +2906,7 @@ static int read_populate_todo(struct repository *r,
>  		struct todo_list done = TODO_LIST_INIT;
>  
>  		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
> -		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
> +		    !todo_list_parse_insn_buffer(r, opts, done.buf.buf, &done))
>  			todo_list->done_nr = count_commands(&done);
>  		else
>  			todo_list->done_nr = 0;
> @@ -5251,7 +5252,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
>  			goto release_todo_list;
>  
>  		if (file_exists(rebase_path_dropped())) {
> -			if ((res = todo_list_check_against_backup(r, &todo_list)))
> +			if ((res = todo_list_check_against_backup(r, opts,
> +								  &todo_list)))
>  				goto release_todo_list;
>  
>  			unlink(rebase_path_dropped());
> @@ -6294,7 +6296,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  		return error(_("nothing to do"));
>  	}
>  
> -	res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> +	res = edit_todo_list(r, opts, todo_list, &new_todo, shortrevisions,
>  			     shortonto, flags);
>  	if (res == -1)
>  		return -1;
> @@ -6322,7 +6324,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  	strbuf_release(&buf2);
>  	/* Nothing is done yet, and we're reparsing, so let's reset the count */
>  	new_todo.total_nr = 0;
> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> +	if (todo_list_parse_insn_buffer(r, opts, new_todo.buf.buf, &new_todo) < 0)
>  		BUG("invalid todo list after expanding IDs:\n%s",
>  		    new_todo.buf.buf);
>  
> diff --git a/sequencer.h b/sequencer.h
> index 437eabd38af..33943e5ed28 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -137,8 +137,8 @@ struct todo_list {
>  	.buf = STRBUF_INIT, \
>  }
>  
> -int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> -				struct todo_list *todo_list);
> +int todo_list_parse_insn_buffer(struct repository *r, struct replay_opts *opts,
> +				char *buf, struct todo_list *todo_list);
>  int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>  			    const char *file, const char *shortrevisions,
>  			    const char *shortonto, int num, unsigned flags);
> -- 
> gitgitgadget
> 

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The only todo commands that accept a merge commit are "merge" and
> "reset". All the other commands like "pick" or "reword" fail when they
> try to pick a a merge commit and print the message
>
>     error: commit abc123 is a merge but no -m option was given.
>
> followed by a hint about the command being rescheduled. This message is
> designed to help the user when they cherry-pick a merge and forget to
> pass "-m". For users who are rebasing the message is confusing as there
> is no way for rebase to cherry-pick the merge.
>
> Improve the user experience by detecting the error when the todo list is
> parsed rather than waiting for the "pick" command to fail and print a
> message recommending the "merge" command instead. We recommend "merge"
> rather than "exec git cherry-pick -m ..." on the assumption that
> cherry-picking merges is relatively rare and it is more likely that the
> user chose "pick" by a mistake.

> It would be possible to support cherry-picking merges by allowing the
> user to pass "-m" to "pick" commands but that adds complexity to do
> something that can already be achieved with
>
>     exec git cherry-pick -m1 abc123

> Reported-by: Stefan Haller <lists@haller-berlin.de>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c                   | 37 +++++++++++++++++++++++++++++++++--
>  t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)

> diff --git a/sequencer.c b/sequencer.c
> index a3154ba3347..4012c6f88d9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
>  	return 0;
>  }
>  
> -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,
> +static int error_merge_commit(enum todo_command command)
> +{
> +	switch(command) {
> +	case TODO_PICK:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -C");
> +
> +	case TODO_REWORD:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s'"),
> +			     todo_command_info[command].str, "merge -c");
> +
> +	case TODO_EDIT:
> +		return error(_("'%s' does not accept merge commits, "
> +			       "please use '%s' followed by '%s'"),
> +			     todo_command_info[command].str,
> +			     "merge -C", "break");

> +	default:
> +		BUG("unexpected todo_command");
> +	}
> +}
> +
> +static int parse_insn_line(struct repository *r, struct replay_opts *opts,
>  			   struct todo_item *item, const char *buf,
>  			   const char *bol, char *eol)
>  {
> @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED
>  		return status;
>  
>  	item->commit = lookup_commit_reference(r, &commit_oid);
> -	return item->commit ? 0 : -1;
> +	if (!item->commit)
> +		return -1;
> +	if (is_rebase_i(opts) && item->command != TODO_MERGE &&
> +	    item->commit->parents && item->commit->parents->next)
> +		return error_merge_commit(item->command);

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > The only todo commands that accept a merge commit are "merge" and
> > "reset". All the other commands like "pick" or "reword" fail when they
> > try to pick a a merge commit and print the message
> >
> >     error: commit abc123 is a merge but no -m option was given.
> >
> > followed by a hint about the command being rescheduled. This message is
> > designed to help the user when they cherry-pick a merge and forget to
> > pass "-m". For users who are rebasing the message is confusing as there
> > is no way for rebase to cherry-pick the merge.
> >
> > Improve the user experience by detecting the error when the todo list is
> > parsed rather than waiting for the "pick" command to fail and print a
> > message recommending the "merge" command instead. We recommend "merge"
> > rather than "exec git cherry-pick -m ..." on the assumption that
> > cherry-picking merges is relatively rare and it is more likely that the
> > user chose "pick" by a mistake.
> 
> Now, the mention of "all the other commands" makes me curious what
> should happen when your "squash" and "fixup" named a merge commit.
> I think it should just error out without any recourse, but it is
> more than likely that I am missing some use cases where it is useful
> to "squash" or "fixup" a merge commit on top of an existing commit?
> 
> > It would be possible to support cherry-picking merges by allowing the
> > user to pass "-m" to "pick" commands but that adds complexity to do
> > something that can already be achieved with
> >
> >     exec git cherry-pick -m1 abc123
> 
> I have no strong opinions between this and "merge" for "pick",
> "edit", and "reword".
> 
> > Reported-by: Stefan Haller <lists@haller-berlin.de>
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> >  sequencer.c                   | 37 +++++++++++++++++++++++++++++++++--
> >  t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++
> >  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> So, having thought about my version of a solution from the problem
> description above without looking at your answers, let's see how you
> solved it.
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index a3154ba3347..4012c6f88d9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2573,7 +2573,35 @@ static int check_label_or_ref_arg(enum todo_command command, const char *arg)
> >  	return 0;
> >  }
> >  
> > -static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,
> > +static int error_merge_commit(enum todo_command command)
> > +{
> > +	switch(command) {
> > +	case TODO_PICK:
> > +		return error(_("'%s' does not accept merge commits, "
> > +			       "please use '%s'"),
> > +			     todo_command_info[command].str, "merge -C");
> > +
> > +	case TODO_REWORD:
> > +		return error(_("'%s' does not accept merge commits, "
> > +			       "please use '%s'"),
> > +			     todo_command_info[command].str, "merge -c");
> > +
> > +	case TODO_EDIT:
> > +		return error(_("'%s' does not accept merge commits, "
> > +			       "please use '%s' followed by '%s'"),
> > +			     todo_command_info[command].str,
> > +			     "merge -C", "break");
> 
> OK.  And when hitting the "break", they know that they are supposed
> to say "git commit --amend" and then "git rebase --continue"?
> 
> > +	case TODO_FIXUP:
> > +	case TODO_SQUASH:
> > +		return error(_("cannot squash merge commit into another commit"));
> 
> OK, this is as I expected.
> 
> > +	default:
> > +		BUG("unexpected todo_command");
> > +	}
> > +}
> > +
> > +static int parse_insn_line(struct repository *r, struct replay_opts *opts,
> >  			   struct todo_item *item, const char *buf,
> >  			   const char *bol, char *eol)
> >  {
> > @@ -2679,7 +2707,12 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED
> >  		return status;
> >  
> >  	item->commit = lookup_commit_reference(r, &commit_oid);
> > -	return item->commit ? 0 : -1;
> > +	if (!item->commit)
> > +		return -1;
> > +	if (is_rebase_i(opts) && item->command != TODO_MERGE &&
> > +	    item->commit->parents && item->commit->parents->next)
> > +		return error_merge_commit(item->command);
> 
> This is good for now, but we may see command other than TODO_MERGE
> learn how to handle a merge commit, and when that happens, I wonder
> what we want to do here.  One thought is to do this:
> 
> 	if (is_rebase_i(opts) && is_merge_commit(item->commit))
>         	return error_merge_commit(item);
> 
> and teach error_merge_commit() to silently return 0 on TODO_MERGE.
> Other commands, when they learn how to deal with a merge commit,
> will then update their entries in error_merge_commit().
> 
> Would we want to customize the message from error_merge_commit() to
> make it closer to cut-and-paste ready?  For that, something like
> 
> 	int error_merge_commit(struct todo_item *item)
> 	{
> 		switch (item->command) {
> 		case TODO_PICK:
> 			return error(_("'%s'" is bad, plase use "
> 				       '%s %s'"),
> 				todo_command_info[item->command].str,
> 				"merge -C",
> 				oid_to_hex(item->commit->oid));
> 		...
> 		}
> 	}
> 
> might go in a more friendly way.

> I wonder how actionable these commands are. Can we give the full command
> that the user can use instead, including the commit ID?
> 
> That raises another question though: how exactly is the user supposed to
> perform the merge? Should they merge the merge commit, resulting in two
> merge commits? Should they pick one of the sides, and if so, which one?
> I guess the answer is "it depends", which makes it harder for us to come
> up with actionable advice here.

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > So I think it's okay to not mention the exact commit here because we
> > cannot reliably second-guess the ultimate extent. My basic assumption is
> > that in many cases the user may not even be aware of them trying to pick
> > a merge commit, and that it may not have been their intent.
> 
> Hmph, if that assumption is really true, would suggesting "merge -C"
> be the right thing in the first place?
> 
> I do not deal with non-linear stuff in "rebase -i" or sequencer, so
> I may not be a good person to judge how good your basic assumption
> is, though.

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> +	case TODO_EDIT:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s' followed by '%s'"),
>> +			     todo_command_info[command].str,
>> +			     "merge -C", "break");
> 
> OK.  And when hitting the "break", they know that they are supposed
> to say "git commit --amend" and then "git rebase --continue"?

>>   	item->commit = lookup_commit_reference(r, &commit_oid);
>> -	return item->commit ? 0 : -1;
>> +	if (!item->commit)
>> +		return -1;
>> +	if (is_rebase_i(opts) && item->command != TODO_MERGE &&
>> +	    item->commit->parents && item->commit->parents->next)
>> +		return error_merge_commit(item->command);
> 
> This is good for now, but we may see command other than TODO_MERGE
> learn how to handle a merge commit, and when that happens, I wonder
> what we want to do here.  One thought is to do this:
> 
> 	if (is_rebase_i(opts) && is_merge_commit(item->commit))
>          	return error_merge_commit(item);
> 
> and teach error_merge_commit() to silently return 0 on TODO_MERGE.
> Other commands, when they learn how to deal with a merge commit,
> will then update their entries in error_merge_commit().

> Would we want to customize the message from error_merge_commit() to
> make it closer to cut-and-paste ready?  For that, something like
> 
> 	int error_merge_commit(struct todo_item *item)
> 	{
> 		switch (item->command) {
> 		case TODO_PICK:
> 			return error(_("'%s'" is bad, plase use "
> 				       '%s %s'"),
> 				todo_command_info[item->command].str,
> 				"merge -C",
> 				oid_to_hex(item->commit->oid));
> 		...
> 		}
> 	}
> 
> might go in a more friendly way.

>>> +		return error(_("'%s' does not accept merge commits, "
>>> +			       "please use '%s' followed by '%s'"),
>>> +			     todo_command_info[command].str,
>>> +			     "merge -C", "break");
>> OK.  And when hitting the "break", they know that they are supposed
>> to say "git commit --amend" and then "git rebase --continue"?
>
> Yes. I guess we could add a hint to that effect if you think its worth it.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>> +		return error(_("'%s' does not accept merge commits, "
>>>> +			       "please use '%s' followed by '%s'"),
>>>> +			     todo_command_info[command].str,
>>>> +			     "merge -C", "break");
>>> OK.  And when hitting the "break", they know that they are supposed
>>> to say "git commit --amend" and then "git rebase --continue"?
>>
>> Yes. I guess we could add a hint to that effect if you think its worth it.
> 
> As I said elsewhere, I do not deal with non-linear stuff using
> sequencer, so I _may_ be missing something obvious to the target
> audience of this message.  But if I were dipping my toes to try
> mucking with sequencer and edit the todo myself by inserting a random
> merge commit there and got this message, I would have probably
> appreciated if the message were a bit more explicit _why_ I would
> want to use the 'break' there.  Otherwise, I probably would be lost
> sitting in front of the shell command prompt.  If it were
> 
> 	'pick' does not take a merge commit.  If you wanted to
> 	replay the merge, use 'merge -C' on the commit, and then
> 	'break' to give the control back to you so that you can
> 	do 'git commit --amend && git rebase --continue'.
> 
> that would have given me 70% of what I needed (the other 30% is why
> I would want to "--amend", instead of just taking the result of
> 'merge -C' as-is, though).
> 
> We can formulate the message in such a way that a succinct first
> part is sufficient for people who know the command, while the latter
> more verbose and prescriptive part can be hidden behind the advice
> mechanism.

>> It feels funny to call error_merge_commit() for a command that we know
>> supports merges but I can see that would make it easier to extend in
>> the future.
> 
> Yes, I think that it is just a sign that the function is misnamed.
> "Gee we have a merge commit, please tell me what you want to do with
> it" is what the caller is asking, and the error messages are side
> effects the caller does not have to care too much about.

> If the user tries to pick a merge commit error out when parsing the todo
> list rather than complaining when trying to pick the commit.
>
> Sorry for the delay in re-rolling, thanks to Junio and Patrick for their
> comments on V2. I've rebased on to master to avoid a conflict with
> 'ps/the-index-is-no-more' and updated patch 2 to
>
>  * Add advice on how rebase a merge commit as suggested by Junio. To avoid
>    duplication between the error messages and the advice I've shortened the
>    error messages.
>
>  * Rework the control flow to make it easier to extend checks on merge
>    commits if new commands are added in the future as suggested by Junio
>
> Phillip Wood (2):
>   rebase -i: pass struct replay_opts to parse_insn_line()
>   rebase -i: improve error message when picking merge
>
>  Documentation/config/advice.txt |  2 +
>  advice.c                        |  1 +
>  advice.h                        |  1 +
>  builtin/rebase.c                | 17 ++++---
>  rebase-interactive.c            | 21 +++++----
>  rebase-interactive.h            |  9 ++--
>  sequencer.c                     | 83 ++++++++++++++++++++++++++++-----
>  sequencer.h                     |  4 +-
>  t/t3404-rebase-interactive.sh   | 45 ++++++++++++++++++
>  9 files changed, 153 insertions(+), 30 deletions(-)
>
>
> base-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1672
>
> Range-diff vs v2:
>
>  1:  1bcf92c6105 ! 1:  91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()
>      @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
>       @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>        		error(_("could not generate todo list"));
>        	else {
>      - 		discard_index(&the_index);
>      + 		discard_index(the_repository->index);
>       -		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
>       -						&todo_list))
>       +		if (todo_list_parse_insn_buffer(the_repository, &replay,

>      + ## Documentation/config/advice.txt ##
>      +@@ Documentation/config/advice.txt: advice.*::
>      + 		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
>      + 		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
>      + 		simultaneously.
>      ++	rebaseTodoError::
>      ++		Shown when there is an error after editing the rebase todo list.

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>   1:  1bcf92c6105 ! 1:  91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line()
>>       @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
>>        @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>>         		error(_("could not generate todo list"));
>>         	else {
>>       - 		discard_index(&the_index);
>>       + 		discard_index(the_repository->index);
>>        -		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
>>        -						&todo_list))
>>        +		if (todo_list_parse_insn_buffer(the_repository, &replay,
> 
> OK.  It would probably have been unnecessary to rebase only for this
> update.

>>       + ## Documentation/config/advice.txt ##
>>       +@@ Documentation/config/advice.txt: advice.*::
>>       + 		`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
>>       + 		`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
>>       + 		simultaneously.
>>       ++	rebaseTodoError::
>>       ++		Shown when there is an error after editing the rebase todo list.
> 
> This thing is new.  It is unclear to me if this description is clear
> enough to readers that we are checking the edited todo list for
> errors.

>  It is clear enough from the actual code change, and the
> readers come to this list after advise_if_enabled() triggers and
> reports that the rebaseTodoError knob allows them to squelch it, so
> it probably is OK.
> 
> Thanks, will replace.  Let's see if we see comments from others and
> then mark it for 'next' soonish.

>> OK.  It would probably have been unnecessary to rebase only for this
>> update.
>
> I agree the conflict is easy to solve but GitGitGadget gets upset and
> refuses to run the ci if it cannot merge the branch into master