Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 24, 2023, 16:00 UTC
Message-ID: <ecabaf7d-d5a3-d2d9-2610-87c6a7b2570e@gmail.com>
In-Reply-To: <CAMMLpeTBBS7FExevcvCWut8wFbcDSDBhUUq+tCaXfOPiY+3GXA@mail.gmail.com>

Hi Alex

On 24/07/2023 16:26, Alex Henrie wrote:

Show 29 quoted lines

> On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@gmail.com> 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