| Message ID | fbc6746e0188ed7b69c238935ec85b69112ddd79.1712585788.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rebase -i: improve error message when picking merge | expand |
"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.
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/
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. Thanks.
On Mon, Apr 08, 2024 at 10:08:24PM -0700, Junio C Hamano wrote: > 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. Yeah, neither do I. The assumption really only comes from my own gut feeling, which can certainly be quite wrong. Patrick
Hi Junio On 08/04/2024 23:29, Junio C Hamano wrote: > "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"? Yes. I guess we could add a hint to that effect if you think its worth it. >> 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(). f 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. > 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. If we want something the user can cut and paste then we would need to suggest a second parent for the merge. We cannot just use the existing parent as it may be rebased which means tracking all the commits we see while parsing the list. If the second parent is rebased we need to make sure the user labels it so we can use that label here. For that reason (and the fact that I don't think can we really tell what the user was hoping to achieve) I think the best we can do is point the user in the direction of the "merge" command even though that leaves them to figure out what to do with that command. Best Wishes Phillip
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.
Hi Junio On 09/04/2024 20:56, Junio C Hamano wrote: > 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. I'll re-roll with a short error message plus some advice. I'll have a think about adding advice for the other commands as well explaining how the "merge" command should be used as well. >> 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. Sure Thanks Phillip
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"); + + 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 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); + return 0; } int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index d1bead61fad..8565fd4b5ae 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -2215,6 +2215,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