From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 24 Nov 2019 10:52:01 GMT
Subject: Re: [PATCH v2 1/1] sequencer: fix empty commit check when amending
Message-ID: <94573071-556b-caae-b159-40c168a08f44@gmail.com>
In-Reply-To: <340859a7-5cc4-f641-818d-fcedbf29a2a6@gmail.com>

Hi Junio

On 23/11/2019 09:54, Phillip Wood wrote:
> Hi Junio
> 
> On 23/11/2019 02:03, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> +    if (!(flags & ALLOW_EMPTY)) {
>>> +        struct commit *first_parent = current_head;
>>> +
>>> +        if (flags & AMEND_MSG) {
>>> +            if (current_head->parents) {
>>
>> It is not apparent to me that somebody guarantees that this access
>> is safe; would we need to do things differently when !current_head?

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. It's a bit tricky to trigger this (there 
isn't a test at the moment) but something like this does it

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

Best Wishes

Phillip

> That's a good point, I'll fix it, thanks for catching this
> 
> Best Wishes
> 
> Phillip
> 
>>
>>> +                first_parent = current_head->parents->item;
>>> +                if (repo_parse_commit(r, first_parent)) {
>>> +                    res = error(_("could not parse HEAD commit"));
>>> +                    goto out;
>>> +                }
>>> +            } else {
>>> +                first_parent = NULL;
>>> +            }
>>> +        }