| Message ID | pull.1672.git.1708945087691.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: improve error message when picking merge | expand |
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 On 26/02/2024 10:58, Phillip Wood via GitGitGadget wrote: > 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. > > 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 <lists@haller-berlin.de> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > 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(-) > > 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)) > BUG("unusable todo list"); > > ret = complete_action(the_repository, &replay, flags, > diff --git a/rebase-interactive.c b/rebase-interactive.c > index d9718409b3d..78d5ed1a41d 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -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); > } > > diff --git a/sequencer.c b/sequencer.c > index 91de546b323..cf808c24d20 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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); > > 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
On Wed, Apr 03, 2024 at 02:42:19PM +0100, phillip.wood123@gmail.com wrote: > 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 I ain't got much of a stake in this, but I've put in my 2c anyway. :) Patrick
On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote: > 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. > > 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 Okay. I think it's reasonable to abort early and give some advice here. It's certainly a lot more user friendly than to abort during the rebase. And if we ever gain the ability to e.g. `pick -m1` in the instruction sheet directly we can adapt accordingly. > 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 <lists@haller-berlin.de> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > 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(-) > > 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? > BUG("unusable todo list"); > > ret = complete_action(the_repository, &replay, flags, > diff --git a/rebase-interactive.c b/rebase-interactive.c > index d9718409b3d..78d5ed1a41d 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -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); > } > > diff --git a/sequencer.c b/sequencer.c > index 91de546b323..cf808c24d20 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -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"); 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. > + 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 > + 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 >
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 >>
On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote: > 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. > > 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 <lists@haller-berlin.de> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- Hi Phillip. 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. On 04/04/2024 20:44, Rubén Justo wrote: > 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 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. > 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
On Fri, Apr 05, 2024 at 10:30:37AM +0100, phillip.wood123@gmail.com wrote: > Hi Rubén > > Thanks for trying this out. > > On 04/04/2024 20:44, Rubén Justo wrote: > > 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 > > 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. > > > 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. 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. > Best Wishes > > Phillip
Hi Rubén On 06/04/2024 15:24, Rubén Justo wrote: > On Fri, Apr 05, 2024 at 10:30:37AM +0100, phillip.wood123@gmail.com wrote: >> On 04/04/2024 20:44, Rubén Justo wrote: >>> 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. Ideally we'd clear the error messages when the user edited the todo list but there isn't an easy way to do that. In principle we could stop printing the error messages when parsing the list before it is edited but I'm not sure how easy that would be. The behavior you are seeing is already present is not changed by this patch - existing error messages behave in the same way. Best Wishes Phillip > Thank you for your explanations. > >> Best Wishes >> >> Phillip
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)) BUG("unusable todo list"); ret = complete_action(the_repository, &replay, flags, diff --git a/rebase-interactive.c b/rebase-interactive.c index d9718409b3d..78d5ed1a41d 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -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); } diff --git a/sequencer.c b/sequencer.c index 91de546b323..cf808c24d20 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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); 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