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

Commit Message

Linus Arver via GitGitGadget Nov. 22, 2019, 7:43 p.m. UTC

From: Phillip Wood <[e-mail]>

This fixes a regression introduced in 356ee4659b ("sequencer: try to
commit without forking 'git commit'", 2017-11-24). When amending a
commit try_to_commit() was using the wrong parent when checking if the
commit would be empty. When amending we need to check against HEAD^ not
HEAD.

t3403 may not seem like the natural home for the new tests but a further
patch series will improve the advice printed by `git commit`. That
series will mutate these tests to check that the advice includes
suggesting `rebase --skip` to skip the fixup that would empty the
commit.

Signed-off-by: Phillip Wood <[e-mail]>
---
 sequencer.c            | 26 +++++++++++++++++++++-----
 t/t3403-rebase-skip.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Nov. 23, 2019, 2:02 a.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> This fixes a regression introduced in 356ee4659b ("sequencer: try to
> commit without forking 'git commit'", 2017-11-24). When amending a
> commit try_to_commit() was using the wrong parent when checking if the
> commit would be empty. When amending we need to check against HEAD^ not
> HEAD.

Thanks.  Will queue.

Junio C Hamano Nov. 23, 2019, 2:03 a.m. UTC | #2

"Phillip Wood via GitGitGadget" <[e-mail]> 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?

> +				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;
> +			}
> +		}

Phillip Wood Nov. 23, 2019, 9:54 a.m. UTC | #3

Hi Junio

On 23/11/2019 02:03, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> 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?

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;
>> +			}
>> +		}

Phillip Wood Nov. 24, 2019, 10:52 a.m. UTC | #4

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" <[e-mail]> 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;
>>> +            }
>>> +        }

Junio C Hamano Nov. 25, 2019, 3 a.m. UTC | #5

Phillip Wood <[e-mail]> 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.

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

Phillip Wood Nov. 25, 2019, 2:23 p.m. UTC | #6

On 25/11/2019 03:00, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> 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.
> 
> 
>

Johannes Schindelin Nov. 25, 2019, 3:53 p.m. UTC | #7

Hi Phillip,

On Mon, 25 Nov 2019, Phillip Wood wrote:

> On 25/11/2019 03:00, Junio C Hamano wrote:
> > Phillip Wood <[e-mail]> 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,
> > > hint: 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

How about `git worktree --orphan does-not-exist unborn`?

Ciao,
Dscho

Eric Sunshine Nov. 25, 2019, 4:10 p.m. UTC | #8

On Mon, Nov 25, 2019 at 10:54 AM Johannes Schindelin
<[e-mail]> wrote:
> On Mon, 25 Nov 2019, Phillip Wood wrote:
> > On 25/11/2019 03:00, Junio C Hamano wrote:
> > > 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
>
> How about `git worktree --orphan does-not-exist unborn`?

git-worktree doesn't presently recognize --orphan, though it would be
nice if it did. In fact, I clearly was thinking of --orphan (along
with -b, -B, and --detach), when I wrote the implementation, as can be
seen from the commentary in one of the original patches[1]. That
--orphan never got added was either due to an oversight or it was one
of those "we'll add it when someone actually needs it" deals.

[1]: https://lore.kernel.org/git/[e-mail]/

Phillip Wood Nov. 25, 2019, 4:42 p.m. UTC | #9

Hi Dscho

On 25/11/2019 15:53, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Mon, 25 Nov 2019, Phillip Wood wrote:
> 
>> On 25/11/2019 03:00, Junio C Hamano wrote:
>>> Phillip Wood <[e-mail]> 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,
>>>> hint: 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
> 
> How about `git worktree --orphan does-not-exist unborn`?

I'm trying to create the unborn branch in the current worktree as that 
is where the rebase is happening

Best Wishes

Phillip

> 
> Ciao,
> Dscho
>

Johannes Schindelin Nov. 25, 2019, 10:52 p.m. UTC | #10

Hi Eric,


On Mon, 25 Nov 2019, Eric Sunshine wrote:

> On Mon, Nov 25, 2019 at 10:54 AM Johannes Schindelin
> <[e-mail]> wrote:
> > On Mon, 25 Nov 2019, Phillip Wood wrote:
> > > On 25/11/2019 03:00, Junio C Hamano wrote:
> > > > 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
> >
> > How about `git worktree --orphan does-not-exist unborn`?
>
> git-worktree doesn't presently recognize --orphan, though it would be
> nice if it did. In fact, I clearly was thinking of --orphan (along
> with -b, -B, and --detach), when I wrote the implementation, as can be
> seen from the commentary in one of the original patches[1]. That
> --orphan never got added was either due to an oversight or it was one
> of those "we'll add it when someone actually needs it" deals.
>
> [1]: https://lore.kernel.org/git/[e-mail]/

You're absolutely correct, of course. I actually had looked at the output
of `git checkout -h` instead of `git worktree -h`... And `checkout` does
have that `--orphan` option.

But from the documentation at
https://git-scm.com/docs/git-checkout#Documentation/git-checkout.txt---orphanltnewbranchgt
I see that the command I had in mind does not work as I expected it to:
`git checkout --orphan new-branch $EMPTY_TREE` will fail with
fatal: Cannot switch branch to a non-commit '4b825dc642cb6eb9a060e54bf8d69288fbee4904'
(and the documentation of the `--orphan` option also suggests to use `git
rm -rf` for the use case under discussion, so there...)

Sorry for the noise,
Dscho

Junio C Hamano Nov. 26, 2019, 1:11 a.m. UTC | #11

Phillip Wood <[e-mail]> writes:

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

"checkout --orphan" takes you to an unborn branch.  If you need to
also be with no contents in the index and nothing in the working
tree, then "git rm -r ." may be needed, but applying a fixup without
the target content does sound like asking for a conflict already.

Patch

diff --git a/sequencer.c b/sequencer.c
index da2decbd3a..f4f81cbddc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1351,11 +1351,27 @@  static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (!(flags & ALLOW_EMPTY) && oideq(current_head ?
-					    get_commit_tree_oid(current_head) :
-					    the_hash_algo->empty_tree, &tree)) {
-		res = 1; /* run 'git commit' to display error message */
-		goto out;
+	if (!(flags & ALLOW_EMPTY)) {
+		struct commit *first_parent = current_head;
+
+		if (flags & AMEND_MSG) {
+			if (current_head->parents) {
+				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;
+			}
+		}
+		if (oideq(first_parent
+			  ? get_commit_tree_oid(first_parent)
+			  : the_hash_algo->empty_tree,
+			  &tree)) {
+			res = 1; /* run 'git commit' to display error message */
+			goto out;
+		}
 	}
 
 	if (find_hook("prepare-commit-msg")) {
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 1f5122b632..ee8a8dba52 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -7,6 +7,8 @@  test_description='git rebase --merge --skip tests'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 # we assume the default git am -3 --skip strategy is tested independently
 # and always works :)
 
@@ -20,6 +22,13 @@  test_expect_success setup '
 	git commit -a -m "hello world" &&
 	echo goodbye >> hello &&
 	git commit -a -m "goodbye" &&
+	git tag goodbye &&
+
+	git checkout --detach &&
+	git checkout HEAD^ . &&
+	test_tick &&
+	git commit -m reverted-goodbye &&
+	git tag reverted-goodbye &&
 
 	git checkout -f skip-reference &&
 	echo moo > hello &&
@@ -76,4 +85,27 @@  test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'fixup that empties commit fails' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \
+			goodbye^ reverted-goodbye
+	)
+'
+
+test_expect_success 'squash that empties commit fails' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \
+			goodbye^ reverted-goodbye
+	)
+'
+
+# Must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done