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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Alex

On 24/07/2023 16:26, Alex Henrie wrote:
> 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