Confusing error message in rebase when commit becomes empty

Confusing error message in rebase when commit becomes empty

[Peter Krefting]

Hi!

I am rebasing a branch to combine a couple of commits. One is a revert 
of a previous commit. Since there are commits in-between, I do 
"squash" to make sure I get everything, and then add the actual change 
on top of that. The problem is that rebase stops with a confusing 
error message (from commit, presumably):

   $ git rebase --interactive
   [...]
   You asked to amend the most recent commit, but doing so would make
   it empty. You can repeat your command with --allow-empty, or you can
   remove the commit entirely with "git reset HEAD^".
   rebase in progress; onto 342b22f
   You are currently rebasing branch 'mybranch' on '342b22f'.

   No changes

   Could not apply 4682a1f20f6ac29546536921bc6ea0386441e23e... Revert "something"

OK, so I should retry the command with --allow-empty, then:

   $ git rebase --interactive --allow-empty
   error: unknown option `allow-empty'

Nope, that's not quite right.

Running "git rebase --continue" does work as expected, but perhaps it 
just shouldn't stop in this case?

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: Confusing error message in rebase when commit becomes empty

[Phil Hord]

On Wed, Jun 11, 2014 at 8:49 AM, Peter Krefting <peter@softwolves.pp.se> wrote:
> I am rebasing a branch to combine a couple of commits. One is a revert of a previous commit. Since there are commits in-between, I do "squash" to make sure I get everything, and then add the actual change on top of that. The problem is that rebase stops with a confusing error message (from commit, presumably):
>
>   $ git rebase --interactive
>   [...]
>   You asked to amend the most recent commit, but doing so would make
>   it empty. You can repeat your command with --allow-empty, or you can
>   remove the commit entirely with "git reset HEAD^".
>   rebase in progress; onto 342b22f
>   You are currently rebasing branch 'mybranch' on '342b22f'.
>
>   No changes
>
>   Could not apply 4682a1f20f6ac29546536921bc6ea0386441e23e... Revert "something"
>
> OK, so I should retry the command with --allow-empty, then:
>
>   $ git rebase --interactive --allow-empty
>   error: unknown option `allow-empty'
>
> Nope, that's not quite right.


The correct switch for rebase is --keep-empty, but it is too late to
choose it once the interactive rebase is underway.  I think the
correct advice might be something like this:

  You asked to squash this commit and its parent, but doing so would make
  it empty. You can drop this empty commit with "git reset HEAD^" , or you can
  keep it with "git commit --amend --allow-empty".

But I have not tested this.


> Running "git rebase --continue" does work as expected, but perhaps it just shouldn't stop in this case?


What does it mean when you say it worked as expected?  Did it leave
the empty commit, omit the empty commit, or leave some un-squashed
commit?  It's not clear to me what --continue _should_ do in this
case, but it does seem like the two options here should be

 1. keep the empty commit
 2. drop the empty commit

I would expect "git rebase --skip" to drop the empty commit, but maybe
it will "skip" the squash instead.  I don't know.  Better advice here
is certainly needed.

Re: Confusing error message in rebase when commit becomes empty

[Peter Krefting]

Phil Hord:

> What does it mean when you say it worked as expected?  Did it leave
> the empty commit, omit the empty commit, or leave some un-squashed
> commit?

Actually, it did not work as expected I noted afterward, it just 
dropped the reversion commit, and did not squash the next commit into 
it as I had asked, so from three commits, "change", "revert", 
"new-change" I had two, "change", "new-change" with the end result 
being the same (i.e., instead of squashing all three into one 
"new-change", I had "change" and "revert" + "new-change").

> It's not clear to me what --continue _should_ do in this 
> case, but it does seem like the two options here should be

I sort of expect a squashed commit of "change" + "revert" to be an 
empty commit, and of "change" + "revert" + "new-change" to be a commit 
of "new-change".

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: Confusing error message in rebase when commit becomes empty

[Phil Hord]

On Wed, Jun 11, 2014 at 1:57 PM, Peter Krefting <peter@softwolves.pp.se> wrote:
> Phil Hord:
>
>
>> What does it mean when you say it worked as expected?  Did it leave
>> the empty commit, omit the empty commit, or leave some un-squashed
>> commit?
>
>
> Actually, it did not work as expected I noted afterward, it just dropped the
> reversion commit, and did not squash the next commit into it as I had asked,
> so from three commits, "change", "revert", "new-change" I had two, "change",
> "new-change" with the end result being the same (i.e., instead of squashing
> all three into one "new-change", I had "change" and "revert" +
> "new-change").

Did you have a series of three commits being squashed in your to-do
list?  I mean, did you have a list like this:

   pick ...  do foo
   squash ...  revert "do foo"
   squash ...  What I really meant to do.

I suppose the rebase stopped after the first squash failed due to the
emptiness of the proposed result.  Then rebase --continue proceeded,
having decided that you were finished with the 'revert' commit.  Then
... I would expect the next commit would actually be squashed, but I
can only speculate at the reasons it might have decided not to after
your continue.

This actually sounds like another case of a bug I reported a few weeks
ago[1] and which Fabian Ruch was kindly investigating[2] and trying to
fix.  I don't think his fix would have helped in this case, but I do
think it is worthy of consideration for that same patch series.

>> It's not clear to me what --continue _should_ do in this case, but it does
>> seem like the two options here should be
>
> I sort of expect a squashed commit of "change" + "revert" to be an empty
> commit, and of "change" + "revert" + "new-change" to be a commit of
> "new-change".

Yes, but empty commits are discouraged on some projects.  If you want
your "change + revert = empty" commit to appear after the squash, I
would expect you would want to use --keep-empty on your inital rebase
command.  But I'm not sure that will do what you expected either; it
may only keep previously-empty commits during the rebase.

[1] http://article.gmane.org/gmane.comp.version-control.git/245688
[2] http://www.mail-archive.com/git%40vger.kernel.org/msg51703.html

Re: Confusing error message in rebase when commit becomes empty

[Peter Krefting]

Phil Hord:

> Did you have a series of three commits being squashed in your to-do
> list?  I mean, did you have a list like this:
>
>   pick ...  do foo
>   squash ...  revert "do foo"
>   squash ...  What I really meant to do.

Yes, that is exactly what I had. Plus an extra commit that I moved to 
the end, which was originally placed between the "do foo" and "revert 
do foo" commits (which is why I wasn't 110% sure the combination of 
the two would produce an empty commit).

> Yes, but empty commits are discouraged on some projects.  If you 
> want your "change + revert = empty" commit to appear after the 
> squash, I would expect you would want to use --keep-empty on your 
> inital rebase command.  But I'm not sure that will do what you 
> expected either; it may only keep previously-empty commits during 
> the rebase.

The thing is that I wasn't expecting it to come out empty, as I had 
another commit to squash into it. That the interim throw-away squashed 
commit was empty should have been an internal matter to rebase, IMHO.

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: Confusing error message in rebase when commit becomes empty

[Jeff King]

On Wed, Jun 11, 2014 at 01:49:04PM +0100, Peter Krefting wrote:

> Hi!
> 
> I am rebasing a branch to combine a couple of commits. One is a revert of a
> previous commit. Since there are commits in-between, I do "squash" to make
> sure I get everything, and then add the actual change on top of that. The
> problem is that rebase stops with a confusing error message (from commit,
> presumably):
> 
>   $ git rebase --interactive
>   [...]
>   You asked to amend the most recent commit, but doing so would make
>   it empty. You can repeat your command with --allow-empty, or you can
>   remove the commit entirely with "git reset HEAD^".
>   rebase in progress; onto 342b22f
>   You are currently rebasing branch 'mybranch' on '342b22f'.
> 
>   No changes
> 
>   Could not apply 4682a1f20f6ac29546536921bc6ea0386441e23e... Revert "something"
> 
> OK, so I should retry the command with --allow-empty, then:
> 
>   $ git rebase --interactive --allow-empty
>   error: unknown option `allow-empty'
> 
> Nope, that's not quite right.

Yeah, that message comes from "commit --amend", which is called by
rebase to handle the squash. The "repeat your command" part is
confusing. The right thing to do here is:

  git commit --amend --allow-empty

if you want to have an empty commit, or:

  git reset HEAD^

if you want to have nothing.

Of course the first one would never occur to you, because it is not
"your command" in the first place. :)

We could change it to say "use git commit --amend --allow-empty", though
that is slightly incomplete for other cases (e.g., you might have
actually said "git commit --amend -a", and the right advice is to
include that "-a".

Commit understands a "whence" flag that could let it customize the
message for the case of rebase. But I think you would have to teach
determine_whence to figure out that we are in a rebase.

> Running "git rebase --continue" does work as expected, but perhaps it just
> shouldn't stop in this case?

As you noticed later in the thread, doing "--continue" omits the revert.
That's because it is telling rebase "OK, I've fixed this up, we can keep
going". But of course it wasn't fixed.

Re: Confusing error message in rebase when commit becomes empty

[Jeff King]

On Fri, Jun 13, 2014 at 08:25:43AM +0100, Peter Krefting wrote:

> >Yes, but empty commits are discouraged on some projects.  If you want your
> >"change + revert = empty" commit to appear after the squash, I would
> >expect you would want to use --keep-empty on your inital rebase command.
> >But I'm not sure that will do what you expected either; it may only keep
> >previously-empty commits during the rebase.
> 
> The thing is that I wasn't expecting it to come out empty, as I had another
> commit to squash into it. That the interim throw-away squashed commit was
> empty should have been an internal matter to rebase, IMHO.

That's a good point that I neglected in my other response. Maybe the
right solution is for "rebase --interactive" to always pass
"--allow-empty" when doing a squash. And then either:

  1. Always keep such empty commits. A user who is surprised by them
     being empty can then revisit them. Or drop them by doing another
     rebase without --keep-empty.

  2. Notice ourselves that the end-result of the whole squash is an
     empty commit, and stop to let the user deal with it.

-Peff