On Mon, Apr 08, 2024 at 03:29:42PM -0700, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
