Improve error handling in git-rebase-update

Improve error handling in git-rebase-update

Don't discard stderr from failed rebase operations
I had an issue where stdout of the failed rebase was
empty but stderr contained:
First, rewinding head to replay your work on top of it...
Dirty index: cannot apply patches (dirty: internal_gyp
third_party/html_office).

Also, in my case the second rebase was actually succeeding
for some reason, which is clearly no expected, so assert
in this case.

BUG=410339
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=292444

[Sam Clegg, 2014-10-10 00:09:39]

sbc@chromium.org changed reviewers:
+ iannucci@chromium.org

[Sam Clegg, 2014-10-10 00:15:12]

https://codereview.chromium.org/645763002/diff/1/git_rebase_update.py
File git_rebase_update.py (right):

https://codereview.chromium.org/645763002/diff/1/git_rebase_update.py#newcode152
git_rebase_update.py:152: git.rebase(parent, start_hash, branch)
In my case this final rebase was succeeding!  So we should check for that even
though it is unlikely.

[iannucci, 2014-10-10 02:35:01]

lgtm, thanks :)

https://codereview.chromium.org/645763002/diff/1/git_rebase_update.py
File git_rebase_update.py (right):

https://codereview.chromium.org/645763002/diff/1/git_rebase_update.py#newcode152
git_rebase_update.py:152: git.rebase(parent, start_hash, branch)
On 2014/10/10 00:15:12, Sam Clegg wrote:
> In my case this final rebase was succeeding!  So we should check for that even
> though it is unlikely.

Oh, weird...

[Sam Clegg, 2014-10-13 17:14:26]

OK, I changed this to add an assertion if the second rebase magically succeeds.

I think these is a third case that is not handled in this login: it is possible
that the rebase fails (due to conflicts) but the squashed rebase is OK and
doesn't resolve in an empty rebase.  In this case the messaging might want to be
different.  Something like: "you branch doesn't rebase cleanly, but the quashed
version does: would you like to squash your branch?".  Just a thought.

[iannucci, 2014-10-13 17:40:38]

On 2014/10/13 17:14:26, Sam Clegg wrote:
> OK, I changed this to add an assertion if the second rebase magically
succeeds.
> 
> I think these is a third case that is not handled in this login: it is
possible
> that the rebase fails (due to conflicts) but the squashed rebase is OK and
> doesn't resolve in an empty rebase.  In this case the messaging might want to
be
> different.  Something like: "you branch doesn't rebase cleanly, but the
quashed
> version does: would you like to squash your branch?".  Just a thought.

Yeah that's a possibility. What if we just changed the text to recommend using
`git squash-branch` and trying the rebase-update again?

[iannucci, 2014-10-13 17:41:39]

https://codereview.chromium.org/645763002/diff/80001/git_rebase_update.py
File git_rebase_update.py (right):

https://codereview.chromium.org/645763002/diff/80001/git_rebase_update.py#new...
git_rebase_update.py:156: assert(not second_rebase_ret.success)
I would actually throw an AssertionError with the text that you have in the
comment. Otherwise it just looks like the tool is brokened

[Sam Clegg, 2014-10-13 17:47:39]

On 2014/10/13 17:40:38, iannucci wrote:
> On 2014/10/13 17:14:26, Sam Clegg wrote:
> > OK, I changed this to add an assertion if the second rebase magically
> succeeds.
> > 
> > I think these is a third case that is not handled in this login: it is
> possible
> > that the rebase fails (due to conflicts) but the squashed rebase is OK and
> > doesn't resolve in an empty rebase.  In this case the messaging might want
to
> be
> > different.  Something like: "you branch doesn't rebase cleanly, but the
> quashed
> > version does: would you like to squash your branch?".  Just a thought.
> 
> Yeah that's a possibility. What if we just changed the text to recommend using
> `git squash-branch` and trying the rebase-update again?

If its ok with you I'd like to treat that as a separate issue and land this
as-is.

The particular issue I was seeing was that I was hitting that new assert (still
not sure how/why).

[iannucci, 2014-10-13 18:09:35]

On 2014/10/13 17:47:39, Sam Clegg wrote:
> On 2014/10/13 17:40:38, iannucci wrote:
> > On 2014/10/13 17:14:26, Sam Clegg wrote:
> > > OK, I changed this to add an assertion if the second rebase magically
> > succeeds.
> > > 
> > > I think these is a third case that is not handled in this login: it is
> > possible
> > > that the rebase fails (due to conflicts) but the squashed rebase is OK and
> > > doesn't resolve in an empty rebase.  In this case the messaging might want
> to
> > be
> > > different.  Something like: "you branch doesn't rebase cleanly, but the
> > quashed
> > > version does: would you like to squash your branch?".  Just a thought.
> > 
> > Yeah that's a possibility. What if we just changed the text to recommend
using
> > `git squash-branch` and trying the rebase-update again?
> 
> If its ok with you I'd like to treat that as a separate issue and land this
> as-is.
> 
> The particular issue I was seeing was that I was hitting that new assert
(still
> not sure how/why).

So, that can happen if you have uncommitted changes on your branch, I think.
Before rebase-update does its thing, it does a `git freeze`, which creates new
commits on your branch. If you then squash and rebase it, you'll end up with
not-a-clean rebase.

I think the right solution here is to make `git squash-branch` exclude
git-freeze commits, squash the rest, and then rebase the freeze commits onto the
newly squashed branch. This would allow the regular rebase to succeed, but we'd
need to detect the situation where unfreezing the branch at the end of the
rebase-update lands you on the upstream again (otherwise you'd have a leftover
CL number, empty branch, etc).

[iannucci, 2014-10-13 18:10:05]

lgtm though I would definitely throw a more informative error

[Sam Clegg, 2014-10-13 18:45:00]

On 2014/10/13 18:09:35, iannucci wrote:
> On 2014/10/13 17:47:39, Sam Clegg wrote:
> > On 2014/10/13 17:40:38, iannucci wrote:
> > > On 2014/10/13 17:14:26, Sam Clegg wrote:
> > > > OK, I changed this to add an assertion if the second rebase magically
> > > succeeds.
> > > > 
> > > > I think these is a third case that is not handled in this login: it is
> > > possible
> > > > that the rebase fails (due to conflicts) but the squashed rebase is OK
and
> > > > doesn't resolve in an empty rebase.  In this case the messaging might
want
> > to
> > > be
> > > > different.  Something like: "you branch doesn't rebase cleanly, but the
> > > quashed
> > > > version does: would you like to squash your branch?".  Just a thought.
> > > 
> > > Yeah that's a possibility. What if we just changed the text to recommend
> using
> > > `git squash-branch` and trying the rebase-update again?
> > 
> > If its ok with you I'd like to treat that as a separate issue and land this
> > as-is.
> > 
> > The particular issue I was seeing was that I was hitting that new assert
> (still
> > not sure how/why).
> 
> So, that can happen if you have uncommitted changes on your branch, I think.
> Before rebase-update does its thing, it does a `git freeze`, which creates new
> commits on your branch. If you then squash and rebase it, you'll end up with
> not-a-clean rebase.

I don't think that is what happened in my case.  I didn't have any outstanding
work when I ran rebase-update.

rebase-update looks like it does 3 different rebases:

1. attempt normal rebase (but abort on fail)
2. if that fails then attempt squashing rebase 
3. if that fails then redo (1) to leave user in the assumed conflicted state.

My problem is that (1) fails and then (3) succeeds!  Even though they should be
the same operation and should both fail in the same way.  Also, although (1)
fails it is not due to a merge conflict but something else (see CL description).



> 
> I think the right solution here is to make `git squash-branch` exclude
> git-freeze commits, squash the rest, and then rebase the freeze commits onto
the
> newly squashed branch. This would allow the regular rebase to succeed, but
we'd
> need to detect the situation where unfreezing the branch at the end of the
> rebase-update lands you on the upstream again (otherwise you'd have a leftover
> CL number, empty branch, etc).

[Sam Clegg, 2014-10-13 20:58:33]

The CQ bit was checked by sbc@chromium.org

[commit-bot: I haz the power, 2014-10-13 20:59:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645763002/80001

[commit-bot: I haz the power, 2014-10-13 21:01:03]

Committed patchset #2 (id:80001) as 292444