| Message ID | pull.1672.v3.git.1717076630.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase -i: improve error message when picking merge | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > If the user tries to pick a merge commit error out when parsing the todo > list rather than complaining when trying to pick the commit. > > Sorry for the delay in re-rolling, thanks to Junio and Patrick for their > comments on V2. I've rebased on to master to avoid a conflict with > 'ps/the-index-is-no-more' and updated patch 2 to > > * Add advice on how rebase a merge commit as suggested by Junio. To avoid > duplication between the error messages and the advice I've shortened the > error messages. > > * Rework the control flow to make it easier to extend checks on merge > commits if new commands are added in the future as suggested by Junio > > Phillip Wood (2): > rebase -i: pass struct replay_opts to parse_insn_line() > rebase -i: improve error message when picking merge > > Documentation/config/advice.txt | 2 + > advice.c | 1 + > advice.h | 1 + > builtin/rebase.c | 17 ++++--- > rebase-interactive.c | 21 +++++---- > rebase-interactive.h | 9 ++-- > sequencer.c | 83 ++++++++++++++++++++++++++++----- > sequencer.h | 4 +- > t/t3404-rebase-interactive.sh | 45 ++++++++++++++++++ > 9 files changed, 153 insertions(+), 30 deletions(-) > > > base-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1672 > > Range-diff vs v2: > > 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line() > @@ builtin/rebase.c: static int edit_todo_file(unsigned flags) > @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) > error(_("could not generate todo list")); > else { > - discard_index(&the_index); > + discard_index(the_repository->index); > - if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, > - &todo_list)) > + if (todo_list_parse_insn_buffer(the_repository, &replay, OK. It would probably have been unnecessary to rebase only for this update. > + ## Documentation/config/advice.txt ## > +@@ Documentation/config/advice.txt: advice.*:: > + `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`, > + `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate` > + simultaneously. > ++ rebaseTodoError:: > ++ Shown when there is an error after editing the rebase todo list. This thing is new. It is unclear to me if this description is clear enough to readers that we are checking the edited todo list for errors. It is clear enough from the actual code change, and the readers come to this list after advise_if_enabled() triggers and reports that the rebaseTodoError knob allows them to squelch it, so it probably is OK. Thanks, will replace. Let's see if we see comments from others and then mark it for 'next' soonish.
Hi Junio On 30/05/2024 18:09, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line() >> @@ builtin/rebase.c: static int edit_todo_file(unsigned flags) >> @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) >> error(_("could not generate todo list")); >> else { >> - discard_index(&the_index); >> + discard_index(the_repository->index); >> - if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, >> - &todo_list)) >> + if (todo_list_parse_insn_buffer(the_repository, &replay, > > OK. It would probably have been unnecessary to rebase only for this > update. I agree the conflict is easy to solve but GitGitGadget gets upset and refuses to run the ci if it cannot merge the branch into master >> + ## Documentation/config/advice.txt ## >> +@@ Documentation/config/advice.txt: advice.*:: >> + `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`, >> + `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate` >> + simultaneously. >> ++ rebaseTodoError:: >> ++ Shown when there is an error after editing the rebase todo list. > > This thing is new. It is unclear to me if this description is clear > enough to readers that we are checking the edited todo list for > errors. I'm happy to improve it if you have any suggestions - I had hoped "after editing" would be clear enough. > It is clear enough from the actual code change, and the > readers come to this list after advise_if_enabled() triggers and > reports that the rebaseTodoError knob allows them to squelch it, so > it probably is OK. > > Thanks, will replace. Let's see if we see comments from others and > then mark it for 'next' soonish. Thanks Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: >> OK. It would probably have been unnecessary to rebase only for this >> update. > > I agree the conflict is easy to solve but GitGitGadget gets upset and > refuses to run the ci if it cannot merge the branch into master Heh. TIL. That's a tad unfortunate that we have to let the tail wag the dog, but OK.
If the user tries to pick a merge commit error out when parsing the todo list rather than complaining when trying to pick the commit. Sorry for the delay in re-rolling, thanks to Junio and Patrick for their comments on V2. I've rebased on to master to avoid a conflict with 'ps/the-index-is-no-more' and updated patch 2 to * Add advice on how rebase a merge commit as suggested by Junio. To avoid duplication between the error messages and the advice I've shortened the error messages. * Rework the control flow to make it easier to extend checks on merge commits if new commands are added in the future as suggested by Junio Phillip Wood (2): rebase -i: pass struct replay_opts to parse_insn_line() rebase -i: improve error message when picking merge Documentation/config/advice.txt | 2 + advice.c | 1 + advice.h | 1 + builtin/rebase.c | 17 ++++--- rebase-interactive.c | 21 +++++---- rebase-interactive.h | 9 ++-- sequencer.c | 83 ++++++++++++++++++++++++++++----- sequencer.h | 4 +- t/t3404-rebase-interactive.sh | 45 ++++++++++++++++++ 9 files changed, 153 insertions(+), 30 deletions(-) base-commit: 3a57aa566a21e7a510c64881bc6bdff7eb397988 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1672 Range-diff vs v2: 1: 1bcf92c6105 ! 1: 91c6f2f1b45 rebase -i: pass struct replay_opts to parse_insn_line() @@ builtin/rebase.c: static int edit_todo_file(unsigned flags) @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) error(_("could not generate todo list")); else { - discard_index(&the_index); + discard_index(the_repository->index); - if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, - &todo_list)) + if (todo_list_parse_insn_buffer(the_repository, &replay, 2: fbc6746e018 ! 2: 86052416b22 rebase -i: improve error message when picking merge @@ Commit message 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. + Improve the user experience by detecting the error and printing some + advice on how to fix it when the todo list is parsed rather than waiting + for the "pick" command to fail. The advice recommends "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 @@ Commit message Reported-by: Stefan Haller <lists@haller-berlin.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> + ## Documentation/config/advice.txt ## +@@ Documentation/config/advice.txt: advice.*:: + `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`, + `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate` + simultaneously. ++ rebaseTodoError:: ++ Shown when there is an error after editing the rebase todo list. + refSyntax:: + Shown when the user provides an illegal ref name, to + tell the user about the ref syntax documentation. + + ## advice.c ## +@@ advice.c: static struct { + [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" }, + [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" }, + [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */ ++ [ADVICE_REBASE_TODO_ERROR] = { "rebaseTodoError" }, + [ADVICE_REF_SYNTAX] = { "refSyntax" }, + [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" }, + [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" }, + + ## advice.h ## +@@ advice.h: enum advice_type { + ADVICE_PUSH_UNQUALIFIED_REF_NAME, + ADVICE_PUSH_UPDATE_REJECTED, + ADVICE_PUSH_UPDATE_REJECTED_ALIAS, ++ ADVICE_REBASE_TODO_ERROR, + ADVICE_REF_SYNTAX, + ADVICE_RESET_NO_REFRESH_WARNING, + ADVICE_RESOLVE_CONFLICT, + ## sequencer.c ## @@ sequencer.c: 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) ++static int check_merge_commit_insn(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"); ++ error(_("'%s' does not accept merge commits"), ++ todo_command_info[command].str); ++ advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _( ++ /* ++ * TRANSLATORS: 'pick' and 'merge -C' should not be ++ * translated. ++ */ ++ "'pick' does not take a merge commit. If you wanted to\n" ++ "replay the merge, use 'merge -C' on the commit.")); ++ return -1; + + case TODO_REWORD: -+ return error(_("'%s' does not accept merge commits, " -+ "please use '%s'"), -+ todo_command_info[command].str, "merge -c"); ++ error(_("'%s' does not accept merge commits"), ++ todo_command_info[command].str); ++ advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _( ++ /* ++ * TRANSLATORS: 'reword' and 'merge -c' should not be ++ * translated. ++ */ ++ "'reword' does not take a merge commit. If you wanted to\n" ++ "replay the merge and reword the commit message, use\n" ++ "'merge -c' on the commit")); ++ return -1; + + 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"); ++ error(_("'%s' does not accept merge commits"), ++ todo_command_info[command].str); ++ advise_if_enabled(ADVICE_REBASE_TODO_ERROR, _( ++ /* ++ * TRANSLATORS: 'edit', 'merge -C' and 'break' should ++ * not be translated. ++ */ ++ "'edit' does not take a merge commit. If you wanted to\n" ++ "replay the merge, use 'merge -C' on the commit, and then\n" ++ "'break' to give the control back to you so that you can\n" ++ "do 'git commit --amend && git rebase --continue'.")); ++ return -1; + + case TODO_FIXUP: + case TODO_SQUASH: + return error(_("cannot squash merge commit into another commit")); + ++ case TODO_MERGE: ++ return 0; ++ + default: + BUG("unexpected todo_command"); + } @@ sequencer.c: static int parse_insn_line(struct repository *r, struct replay_opts - return item->commit ? 0 : -1; + if (!item->commit) + return -1; -+ if (is_rebase_i(opts) && item->command != TODO_MERGE && ++ if (is_rebase_i(opts) && + item->commit->parents && item->commit->parents->next) -+ return error_merge_commit(item->command); ++ return check_merge_commit_insn(item->command); + return 0; } @@ t/t3404-rebase-interactive.sh: test_expect_success 'bad labels and refs rejected + 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: ${SQ}pick${SQ} does not accept merge commits ++ hint: ${SQ}pick${SQ} does not take a merge commit. If you wanted to ++ hint: replay the merge, use ${SQ}merge -C${SQ} on the commit. ++ hint: Disable this message with "git config advice.rebaseTodoError false" + error: invalid line 1: pick $oid -+ error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ} ++ error: ${SQ}reword${SQ} does not accept merge commits ++ hint: ${SQ}reword${SQ} does not take a merge commit. If you wanted to ++ hint: replay the merge and reword the commit message, use ++ hint: ${SQ}merge -c${SQ} on the commit ++ hint: Disable this message with "git config advice.rebaseTodoError false" + 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: ${SQ}edit${SQ} does not accept merge commits ++ hint: ${SQ}edit${SQ} does not take a merge commit. If you wanted to ++ hint: replay the merge, use ${SQ}merge -C${SQ} on the commit, and then ++ hint: ${SQ}break${SQ} to give the control back to you so that you can ++ hint: do ${SQ}git commit --amend && git rebase --continue${SQ}. ++ hint: Disable this message with "git config advice.rebaseTodoError false" + error: invalid line 3: edit $oid + error: cannot squash merge commit into another commit + error: invalid line 4: fixup $oid