Re: [PATCH] rebase -i: improve error message when picking merge

[phillip.wood123@xxxxxxxxx]

Hi Patrick

On 04/04/2024 07:08, Patrick Steinhardt wrote:
> 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?

I agree the boolean is a bit opaque, I don't think they are that unusual 
in the code base but that doesn't mean they're readable. I had hoped to 
pass an instance of 'struct replay_opts' to parse_insn_line() and then 
call is_rebase_i() which there which would have been nicer. 
Unfortunately "git rebase --edit-todo" does not instantiate an instance 
of that struct as it is not needed for editing the todo list so I went 
with a boolean instead. There are one or two places where we need to use 
is_rebase_i() for this when calling parse_insn_line() which makes using 
an unsigned flags argument a bit messy. I'll have a look and see how 
hard it would be to ensure we always have a valid 'struct replay_opts' 
when calling parse_insn_line().

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

The calling function also prints the offending line so the user can see 
the commit details there.

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

I agree it would be nice to be more precise but I'm not sure we can tell 
what the user is actually trying to do so I think the best we can do is 
point them in the direction of the 'merge' command.

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

Yes, it is a subtle difference.

Thanks very much for taking a look at this

Phillip

> 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