Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending

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

On 25/11/2019 03:00, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> 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.
> 
> 
>