rebase -i: improve error message when picking merge

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

builtin/rebase.c +1 -1

index 5b086f651a6..a33e41c44da 100644
@@ -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,

rebase-interactive.c +4 -3

index d9718409b3d..78d5ed1a41d 100644
@@ -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);
  	}
 

sequencer.c +42 -7

index 91de546b323..cf808c24d20 100644
@@ -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);
 

sequencer.h +1 -1

index dcef7bb99c0..ed2c4b38514 100644
@@ -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);

t/t3404-rebase-interactive.sh +33 -0

index 64b641002e4..20b8589ad07 100755
@@ -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 <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
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(-)

builtin/rebase.c +0 -0

index 5b086f651a6..a33e41c44da 100644

builtin/rebase.c +0 -0

 			BUG("unusable todo list");
 
 		ret = complete_action(the_repository, &replay, flags,

rebase-interactive.c +4 -3

index d9718409b3d..78d5ed1a41d 100644
@@ -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);
 	}
 

sequencer.c +0 -0

index 91de546b323..cf808c24d20 100644

sequencer.c +29 -7

+	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);
 

sequencer.h +1 -1

index dcef7bb99c0..ed2c4b38514 100644
@@ -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);

t/t3404-rebase-interactive.sh +33 -0

index 64b641002e4..20b8589ad07 100755
@@ -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

Signature · 1 line

gitgitgadget

On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
[...]

builtin/rebase.c static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) · line 300

Quoted 10 lines

builtin/rebase.c +1 -1

index 5b086f651a6..a33e41c44da 100644
@@ -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?

builtin/rebase.c static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)

Quoted 7 lines

builtin/rebase.c +7 -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");

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.

builtin/rebase.c

Quoted 4 lines

builtin/rebase.c +4 -0

+	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

Quoted 144 lines

builtin/rebase.c +29 -7

+	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);
 

sequencer.h +1 -1

index dcef7bb99c0..ed2c4b38514 100644
@@ -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);

t/t3404-rebase-interactive.sh +33 -0

index 64b641002e4..20b8589ad07 100755
@@ -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

Signature · 1 line

gitgitgadget

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---

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

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

Hi Rubén

Thanks for trying this out.

Quoted 11 lines · Rubén Justo

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

Quoted 5 lines

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

Quoted 4 lines

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.

Best Wishes

Phillip

Quoted 8 lines · On Fri, Apr 05, 2024 at 10:30:37AM +0100, [email protected] wrote:

Quoted 3 lines · Rubén Justo

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.

Quoted 3 lines

Best Wishes

Phillip

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 sequencer.c                   | 37 +++++++++++++++++++++++++++++++++--
 t/t3404-rebase-interactive.sh | 33 +++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 2 deletions(-)

sequencer.c +8 -1

index a3154ba3347..4012c6f88d9 100644
@@ -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");
+

sequencer.c +6 -0

+	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)
 {

Quoted 13 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 6 lines

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?

Quoted 5 lines

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

Quoted 6 lines

Reported-by Stefan Haller <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

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

Quoted 17 lines

sequencer.c +8 -1

index a3154ba3347..4012c6f88d9 100644
@@ -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");
+

sequencer.c static int check_label_or_ref_arg(enum todo_command command, const char *arg) · line 2593

Quoted 10 lines

sequencer.c +10 -0

+	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"?

sequencer.c static int check_label_or_ref_arg(enum todo_command command, const char *arg) · line 2597

Quoted 3 lines

sequencer.c +3 -0

+	case TODO_FIXUP:
+	case TODO_SQUASH:
+		return error(_("cannot squash merge commit into another commit"));

OK, this is as I expected.

Quoted 10 lines

sequencer.c +6 -0

+	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)
 {

sequencer.c static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED · line 2714

Quoted 9 lines

sequencer.c +5 -1

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

From: Phillip Wood <[email protected]>

This new parameter will be used in the next commit.

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 <[email protected]>

---
 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(-)

builtin/rebase.c +11 -6

index 891f28468e8..a33a2ed413a 100644
@@ -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;
 

rebase-interactive.c +13 -8

index c343e16fcdd..56fd7206a95 100644
@@ -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);
 	}
 

rebase-interactive.h +6 -3

index 7239c60f791..8e5b181b334 100644
@@ -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

sequencer.c +12 -10

index 2c19846385b..a3154ba3347 100644
@@ -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);
 

sequencer.h +2 -2

index 437eabd38af..33943e5ed28 100644
@@ -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);

Signature · 1 line

gitgitgadget

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.

Quoted 4 lines · Patrick Steinhardt

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.

sequencer.c static int check_label_or_ref_arg(enum todo_command command, const char *arg) · line 2593

Quoted 5 lines · Phillip Wood via GitGitGadget

sequencer.c +5 -0

+	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"?

sequencer.c static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED · line 2714

Quoted 7 lines

  	item->commit = lookup_commit_reference(r, &commit_oid);

sequencer.c +5 -1

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

Quoted 6 lines

sequencer.c static int check_label_or_ref_arg(enum todo_command command, const char *arg) · line 2593

Quoted 4 lines

sequencer.c +4 -0

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

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.

Quoted 8 lines · Phillip Wood

Quoted 6 lines

sequencer.c static int check_label_or_ref_arg(enum todo_command command, const char *arg) · line 2593

Quoted 4 lines

sequencer.c +4 -0

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

Quoted 3 lines

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.

Quoted 10 lines · Phillip Wood via GitGitGadget

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.

Quoted 7 lines

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

Quoted 2 lines

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