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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 6422 bytes --]

On Mon, Apr 08, 2024 at 03:29:42PM -0700, Junio C Hamano wrote:
> "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 was asking basically the same thing in [1]. Quoting that part:

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

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.

I might just as well be wrong about that assumption though.

Patrick

[1]: https://lore.kernel.org/git/Zg5D3dXYFM2SONE-@tanuki/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]