Merge instead of rebasing upstream changes in `gclient sync` when --merge is given.

Merge instead of rebasing upstream changes in `gclient sync` when --merge is given.

Merge ALL the things!

BUG=none

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

[Bernhard Bauer, 2013-12-10 17:49:42]

Please review.

[M-A Ruel, 2013-12-10 17:51:00]

Deflecting to Robbie.

[Bernhard Bauer, 2013-12-14 01:11:39]

Ping :)

[iannucci, 2013-12-16 21:01:15]

I'm hesitant to l-g-t-m this because it adds yet another semi-opaque mode for
managed-mode gclient. What's wrong with using unmanaged mode in order to merge
and/or rebase as you like?

In the long term, I'd like to see:
  * gclient manages DEPS of whatever is currently checked out (i.e. always
unmanaged).
  * We have a merge-flow and a rebase-flow tool which can manage the 'main'
solution as the individual dev likes.

/rant

I think the comment I include below is a bug in this CL though. If you still
want to land this, I'd be OK as long as it were fixed (or you convince me that
it's the desired behavior and not, in fact, a bug :).

https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py#newcode524
gclient_scm.py:524: 'merge' if options.merge else 'fast-forward merge',
Wouldn't we want to still fast-forward, even if merge was selected? I think you
want to parameterize the rebase portion here, not the f-f-m.

[Bernhard Bauer, 2013-12-16 21:39:03]

On 2013/12/16 21:01:15, iannucci wrote:
> I'm hesitant to l-g-t-m this because it adds yet another semi-opaque mode for
> managed-mode gclient. What's wrong with using unmanaged mode in order to merge
> and/or rebase as you like?

This updates the DEPS, regardless of the managed state. If you make a change in
a dependency, otherwise you would need to manually check out the branch with the
old version (with your local changes in it) and merge the DEPS revision in. 

> 
> In the long term, I'd like to see:
>   * gclient manages DEPS of whatever is currently checked out (i.e. always
> unmanaged).
>   * We have a merge-flow and a rebase-flow tool which can manage the 'main'
> solution as the individual dev likes.
> 
> /rant
> 
> I think the comment I include below is a bug in this CL though. If you still
> want to land this, I'd be OK as long as it were fixed (or you convince me that
> it's the desired behavior and not, in fact, a bug :).

https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py#newcode524
gclient_scm.py:524: 'merge' if options.merge else 'fast-forward merge',
On 2013/12/16 21:01:15, iannucci wrote:
> Wouldn't we want to still fast-forward, even if merge was selected? I think
you
> want to parameterize the rebase portion here, not the f-f-m.

With this version (and --merge), we will still try to fast-forward first, but
automatically try to merge if we cannot do that (by virtue of calling `git
merge` without --ff-only), and only offer to rebase if that fails. With your
suggestion, we would never rebase, and ask for confirmation before merging. I'm
not sure if that is really necessary, as merging is easier to undo than
rebasing.

[iannucci, 2014-01-07 22:18:33]

On 2013/12/16 21:39:03, Bernhard Bauer wrote:
> On 2013/12/16 21:01:15, iannucci wrote:
> > I'm hesitant to l-g-t-m this because it adds yet another semi-opaque mode
for
> > managed-mode gclient. What's wrong with using unmanaged mode in order to
merge
> > and/or rebase as you like?
> 
> This updates the DEPS, regardless of the managed state. If you make a change
in
> a dependency, otherwise you would need to manually check out the branch with
the
> old version (with your local changes in it) and merge the DEPS revision in. 
> 
> > 
> > In the long term, I'd like to see:
> >   * gclient manages DEPS of whatever is currently checked out (i.e. always
> > unmanaged).
> >   * We have a merge-flow and a rebase-flow tool which can manage the 'main'
> > solution as the individual dev likes.
> > 
> > /rant
> > 
> > I think the comment I include below is a bug in this CL though. If you still
> > want to land this, I'd be OK as long as it were fixed (or you convince me
that
> > it's the desired behavior and not, in fact, a bug :).
> 
> https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py
> File gclient_scm.py (right):
> 
> https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py#newcode524
> gclient_scm.py:524: 'merge' if options.merge else 'fast-forward merge',
> On 2013/12/16 21:01:15, iannucci wrote:
> > Wouldn't we want to still fast-forward, even if merge was selected? I think
> you
> > want to parameterize the rebase portion here, not the f-f-m.
> 
> With this version (and --merge), we will still try to fast-forward first, but
> automatically try to merge if we cannot do that (by virtue of calling `git
> merge` without --ff-only),

Ah, doesn't this depend on your git config settings (specifically 'merge.ff')?
Should we pass --ff to the merge command to explicitly intend this behavior?

> and only offer to rebase if that fails. With your
> suggestion, we would never rebase, and ask for confirmation before merging.
I'm
> not sure if that is really necessary, as merging is easier to undo than
> rebasing.

fyi: reflog makes rebases pretty easy to undo, too: `git reset --hard @{1}`

[Bernhard Bauer, 2014-01-08 19:09:02]

On 2014/01/07 22:18:33, iannucci wrote:
> On 2013/12/16 21:39:03, Bernhard Bauer wrote:
> > On 2013/12/16 21:01:15, iannucci wrote:
> > > I'm hesitant to l-g-t-m this because it adds yet another semi-opaque mode
> for
> > > managed-mode gclient. What's wrong with using unmanaged mode in order to
> merge
> > > and/or rebase as you like?
> > 
> > This updates the DEPS, regardless of the managed state. If you make a change
> in
> > a dependency, otherwise you would need to manually check out the branch with
> the
> > old version (with your local changes in it) and merge the DEPS revision in. 
> > 
> > > 
> > > In the long term, I'd like to see:
> > >   * gclient manages DEPS of whatever is currently checked out (i.e. always
> > > unmanaged).
> > >   * We have a merge-flow and a rebase-flow tool which can manage the
'main'
> > > solution as the individual dev likes.
> > > 
> > > /rant
> > > 
> > > I think the comment I include below is a bug in this CL though. If you
still
> > > want to land this, I'd be OK as long as it were fixed (or you convince me
> that
> > > it's the desired behavior and not, in fact, a bug :).
> > 
> > https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py
> > File gclient_scm.py (right):
> > 
> >
https://codereview.chromium.org/61823002/diff/30001/gclient_scm.py#newcode524
> > gclient_scm.py:524: 'merge' if options.merge else 'fast-forward merge',
> > On 2013/12/16 21:01:15, iannucci wrote:
> > > Wouldn't we want to still fast-forward, even if merge was selected? I
think
> > you
> > > want to parameterize the rebase portion here, not the f-f-m.
> > 
> > With this version (and --merge), we will still try to fast-forward first,
but
> > automatically try to merge if we cannot do that (by virtue of calling `git
> > merge` without --ff-only),
> 
> Ah, doesn't this depend on your git config settings (specifically 'merge.ff')?
> Should we pass --ff to the merge command to explicitly intend this behavior?

Good idea! Done.

[iannucci, 2014-01-09 22:52:06]

This l's gtm.... how hard would it be to add a test?  Sorry for the really long
review :(

[Bernhard Bauer, 2014-01-21 16:00:41]

On 2014/01/09 22:52:06, iannucci wrote:
> This l's gtm.... how hard would it be to add a test?  Sorry for the really
long
> review :(

Not a problem, It took me quite a while to get around to writing the test too.
But, done!

[iannucci, 2014-01-21 21:40:47]

Thanks for the test :)

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1000: 'd2e35c10ac24d6c621e14a1fcadceb533155627d')
Isn't this the same as `git rev-parse HEAD:`?

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1016: __builtin__.raw_input = lambda x: 'y'
scary :). Does this automatically get set back to what it was? Can we use mox to
do this?

[Bernhard Bauer, 2014-01-21 23:03:54]

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1000: 'd2e35c10ac24d6c621e14a1fcadceb533155627d')
On 2014/01/21 21:40:47, iannucci wrote:
> Isn't this the same as `git rev-parse HEAD:`?

No, this is the tree (i.e. without the commit metadata).

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1016: __builtin__.raw_input = lambda x: 'y'
On 2014/01/21 21:40:47, iannucci wrote:
> scary :). Does this automatically get set back to what it was? Can we use mox
to
> do this?

I have to admit, I stole this from updateConflict below... :) I'll try to make
it a bit less scary tomorrow.

[iannucci, 2014-01-21 23:23:29]

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1000: 'd2e35c10ac24d6c621e14a1fcadceb533155627d')
On 2014/01/21 23:03:54, Bernhard Bauer wrote:
> On 2014/01/21 21:40:47, iannucci wrote:
> > Isn't this the same as `git rev-parse HEAD:`?
> 
> No, this is the tree (i.e. without the commit metadata).

Right, hence the trailing colon :)

HEAD:path refers to tree object in a given commit, and : without the path is the
root tree object.

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1016: __builtin__.raw_input = lambda x: 'y'
On 2014/01/21 23:03:54, Bernhard Bauer wrote:
> On 2014/01/21 21:40:47, iannucci wrote:
> > scary :). Does this automatically get set back to what it was? Can we use
mox
> to
> > do this?
> 
> I have to admit, I stole this from updateConflict below... :) I'll try to make
> it a bit less scary tomorrow.

Heh, fair enough... as long as they're both the same hack... This is just not
threadsafe as-is (not that unittests parallelizes tests by default).

[Bernhard Bauer, 2014-01-22 15:59:50]

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1000: 'd2e35c10ac24d6c621e14a1fcadceb533155627d')
On 2014/01/21 23:23:29, iannucci wrote:
> On 2014/01/21 23:03:54, Bernhard Bauer wrote:
> > On 2014/01/21 21:40:47, iannucci wrote:
> > > Isn't this the same as `git rev-parse HEAD:`?
> > 
> > No, this is the tree (i.e. without the commit metadata).
> 
> Right, hence the trailing colon :)
> 
> HEAD:path refers to tree object in a given commit, and : without the path is
the
> root tree object.

Ooh, that's neat! Done.

https://codereview.chromium.org/61823002/diff/200001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1016: __builtin__.raw_input = lambda x: 'y'
On 2014/01/21 23:23:29, iannucci wrote:
> On 2014/01/21 23:03:54, Bernhard Bauer wrote:
> > On 2014/01/21 21:40:47, iannucci wrote:
> > > scary :). Does this automatically get set back to what it was? Can we use
> mox
> > to
> > > do this?
> > 
> > I have to admit, I stole this from updateConflict below... :) I'll try to
make
> > it a bit less scary tomorrow.
> 
> Heh, fair enough... as long as they're both the same hack... This is just not
> threadsafe as-is (not that unittests parallelizes tests by default).

OK, I made it a method on |scm| which I can override.

[iannucci, 2014-01-22 23:33:07]

lgtm, thanks :)

When you land, please coordinate with the trooper [and probably sheriff] so they
know to keep an eye out. It also probably wouldn't hurt to shoot an email to
chrome-troopers as an FYI as well.

[commit-bot: I haz the power, 2014-01-23 11:22:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/61823002/280001

[commit-bot: I haz the power, 2014-01-23 11:24:29]

Presubmit check for 61823002-280001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Pylint (81 files) (27.60s) failed
************* Module gclient_scm
R0201:881,2:GitWrapper._AskForData: Method could be a function
************* Module gclient_scm_test
W0612:1019,4:ManagedGitWrapperTestCase.testUpdateRebase: Unused variable 'rev'
W0611: 19,0: Unused import __builtin__


Presubmit checks took 103.9s to calculate.

Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.

[commit-bot: I haz the power, 2014-01-23 12:10:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/61823002/360001

[commit-bot: I haz the power, 2014-01-23 12:11:57]

Change committed as 246575