From: Phillip Wood Date: Mon, 24 Jul 2023 16:00:34 GMT Subject: Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line Message-ID: In-Reply-To: Hi Alex On 24/07/2023 16:26, Alex Henrie wrote: > On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood wrote: > >> On 22/07/2023 22:28, Alex Henrie wrote: >>> Before the todo list is edited it is rewritten to shorten the OIDs of >>> the commits being picked and to append advice about editing the list. >>> The exact advice depends on whether the todo list is being edited for >>> the first time or not. After the todo list has been edited it is >>> rewritten to lengthen the OIDs of the commits being picked and to remove >>> the advice. If the edited list cannot be parsed then this last step is >>> skipped. >>> >>> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file() >>> in edit_todo_list(), 2019-03-05) if the existing todo list could not be >>> parsed then the initial rewrite was skipped as well. This had the >>> unfortunate consequence that if the list could not be parsed after the >>> initial edit the advice given to the user was wrong when they re-edited >>> the list. This change relied on todo_list_parse_insn_buffer() returning >>> the whole todo list even when it cannot be parsed. Unfortunately if the >>> list starts with a "fixup" command then it will be truncated and the >>> remaining lines are lost. Fix this by continuing to parse after an >>> initial "fixup" commit as we do when we see any other invalid line. >> >> This version looks great apart from the test being run in an unnecessary >> subshell which looks like it got left in from the last version. Junio >> might be able to correct that when he applies the patch. > > I think I see what you mean now: Because this test never performs a > successful rebase, rebase_setup_and_clean is overkill. I can send a v6 > tonight that uses 'test_when_finished "git rebase --abort"' instead. I don't mind either way on that particular issue though I agree we could just use 'test_when_finished "git rebase --abort"' instead. What I was referring to was the subshell that comes after 'rebase_setup_and_clean'. The change I was looking for was removing the '(' after "rebase_setup_and_clean" at the beginning of the test, removing ')' at the end of the test and adjusting the indentation. +test_expect_success 'the first command cannot be a fixup' ' + rebase_setup_and_clean fixup-first && + ( This subshell is unnecessary as we're not changing directory or exporting any environment variables + cat >orig <<-EOF && + fixup $(git log -1 --format="%h %s" B) + pick $(git log -1 --format="%h %s" C) + EOF + + ( This subshell is required as we're setting GIT_SEQUENCE_EDITOR in the environment. We only want that set for the initial rebase. In particular we do not want it set when we run "git rebase --edit-todo" below which is why we exit the subshell as soon as the initial rebase exits. Best Wishes Phillip + set_replace_editor orig && + test_must_fail git rebase -i A 2>actual + ) && + grep "cannot .fixup. without a previous commit" actual && + grep "You can fix this with .git rebase --edit-todo.." actual && + # verify that the todo list has not been truncated + grep -v "^#" .git/rebase-merge/git-rebase-todo >actual && + test_cmp orig actual && + + test_must_fail git rebase --edit-todo 2>actual && + grep "cannot .fixup. without a previous commit" actual && + grep "You can fix this with .git rebase --edit-todo.." actual && + # verify that the todo list has not been truncated + grep -v "^#" .git/rebase-merge/git-rebase-todo >actual && + test_cmp orig actual + ) +' > Thanks, > > -Alex