On 25/11/2019 03:00, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
We do actually check that there is a valid HEAD before we try to fixup
a commit. Though perhaps we should still change this patch as HEAD may
be changed by another process between that check and re-reading it
here. If you try to fixup a commit without a valid HEAD you get
error: need a HEAD to fixup
hint: Could not execute the todo command
hint:
hint: fixup faef1a5a7637ff91b3611aabd1b96541da5f5536 P
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint: git rebase --edit-todo
hint: git rebase --continue
error: could not copy '.git/rebase-merge/message-squash' to
'.git/rebase-merge/message'
The last error message is unfortunate but we do exit in an orderly
manner rather than segfaulting.
Thanks for thinking about the issue further.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d2f1d5bd23..4f55f0cd1c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -67,6 +67,21 @@ test_expect_success 'setup' '
SHELL=
export SHELL
+test_expect_success 'fixup on orphan branch errors out' '
+
+ test_when_finished "git switch master" &&
+ write_script switch-branch.sh <<-\EOF &&
+ git symbolic-ref HEAD refs/heads/does-not-exist &&
+ git rm -rf .
That "git rm -rf ." scares me, though.
I know I'm not too keen on it my self but we need to empty the worktree
and index if we're going to switch to an unborn branch
+ EOF
+ (
+ set_fake_editor &&
+ FAKE_LINES="exec_./switch-branch.sh \
+ fixup 1" git rebase -i HEAD^
+ ) &&
+ test_pause
+'
+
I think it would be useful to add something like this to the test
suite (changed to check the error message, with a better name for the
script and modified to expect failure) What do you think?
So, we try an interactive rebase, try to apply a fix-up on an unborn
branch and expect it to fail in a controlled way, something like
(
# we are in subshell so freely export
set_fake_editor &&
export FAKE_LINES="exec_./switch-branch.sh fixup 1" &&
test_must_fail git rebase -i HEAD^ 2>error &&
test_i18ngrep "... what we expect ..." error
)
Perhaps. I do not think of a good reason why we should allow
switching to another branch when "rebase -i" gives control back to
the user, so in the longer run, the checked condition may not stay
the same (I suspect you would catch "does-not-exist is unborn and
there is nothing to 'fixup'" right now---I am envisioning that the
condition that is checked and the message we would issue would be
"we gave you a detached HEAD for a good reason---stay there and do
not switch to any other branch") and the message expected by this
test would have to be updated.
I agree there's no good reason for a user to do this. 'git switch' will
refuse to switch branches during a rebase for that reason. At the moment
we check HEAD with get_oid() so that would need changing to check if the
user has switched to another branch (perhaps it could be done when we
check that the index and worktree are clean after running an exec command).
Best Wishes
Phillip
Thanks.
