Re: [PATCH v3 0/2] rebase -i: improve error message when picking merge
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 30, 2024, 17:09 UTC
- Message-ID
- <xmqq7cfbp6pb.fsf@gitster.g>
- In-Reply-To
- <pull.1672.v3.git.1717076630.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 47 quoted lines
> 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.
Show 7 quoted lines
> + ## 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.