On Mon, Jul 24, 2023 at 10:40 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Alex Henrie <alexhenrie24@xxxxxxxxx> writes: > > +test_expect_success 'the first command cannot be a fixup' ' > > + rebase_setup_and_clean fixup-first && > > + ( > > + cat >orig <<-EOF && > > + fixup $(git log -1 --format="%h %s" B) > > + pick $(git log -1 --format="%h %s" C) > > + EOF > > + > > + ( > > + 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 > > + ) > > +' > > The structure of this new test piece, including the use of "log -1 > --format", seems to follow existing tests, and very readable. Why > do we have one extra level of subshell, though? There is no "cd" > that may affect the later test pieces, and set_something_editor that > touches environment that may affect the later test pieces is called > in its own subshell already. > > Other than that, looking good (there may be a valid reason why the > test piece needs the subshell around it, but it was just not apparent > to me). The only reason for the outer subshell is that I thought it was required when using rebase_setup_and_clean, but I see now that rebase_setup_and_clean is used in several tests without a subshell. I'll drop it altogether in v6 and use `test_when_finished "git rebase --abort"` instead. Thanks, -Alex
