Hi Alex
On 24/07/2023 16:26, Alex Henrie wrote:
On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@xxxxxxxxx> 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
