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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2019, 03:00 UTC
Message-ID: <xmqqr21wy80o.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <94573071-556b-caae-b159-40c168a08f44@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 20 quoted lines

> 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.

Show 14 quoted lines

> 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.

Show 13 quoted lines

> +       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.

Thanks.