Re: Confusing error message in rebase when commit becomes empty

2014-06-13 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Confusing error message in rebase when commit becomes empty

2014-06-13 Thread 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Confusing error message in rebase when commit becomes empty

2014-06-13 Thread 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Confusing error message in rebase when commit becomes empty

2014-06-11 Thread Phil Hord
On Wed, Jun 11, 2014 at 1:57 PM, Peter Krefting  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Confusing error message in rebase when commit becomes empty

2014-06-11 Thread 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Confusing error message in rebase when commit becomes empty

2014-06-11 Thread Phil Hord
On Wed, Jun 11, 2014 at 8:49 AM, Peter Krefting  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Confusing error message in rebase when commit becomes empty

2014-06-11 Thread 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html