Use three-way merge when applying Git patches.

Use three-way merge when applying Git patches.

After switching to a pure git workflow, most of the patchset we upload
to rietveld have git metadata. Apparently, however, the scripts here in
depot tools, which are used by the CQ and trybots, are ignoring that
and applying patches without taking advantage of git metadata.
In practice this causes people to rebase and reupload patches more than
necessary, even in the cases when it could be handled automatically by
means of a three way merge.
This change updates the GitCheckout class of depot_tools to use that.

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

[Primiano Tucci (use gerrit), 2014-09-18 09:04:35]

primiano@chromium.org changed reviewers:
+ iannucci@chromium.org, maruel@chromium.org

[Primiano Tucci (use gerrit), 2014-09-18 09:04:35]

Infra folks, is there any reason why this is not using three way merge already?
Interestingly, the code in "git cl path" (which is independent of this) does
that.
There are a lot of cases in which the patch fail to apply on the CQ in trivial
cases where a three way merge would have suceeded. Most of the times we have to
waste time by doing a local git rebase && git cl upload && re-tick the commit
box.
This step, though, feels to me unnecessary and could be done directly server
side just by passing an extra '-3' to git cl patch.
The only doubt that I have at this point is: is this script used by both trybots
AND the CQ? We definitely don't want to end up in a state where the trybots
suceed and the CQ fails to apply a patch.
The former looks true to me (trybots seem to invoke apply_issue which in turns
use checkout.py). Not sure about the CQ though.

[Torne, 2014-09-18 09:06:45]

On 2014/09/18 09:04:35, Primiano Tucci wrote:
> There are a lot of cases in which the patch fail to apply on the CQ in trivial
> cases where a three way merge would have suceeded.

Well, it might not actually be a lot of cases, but it's more than zero, and the
normal apply method is still tried first even with -3 so this shouldn't be able
to make anything fail that previously succeeded :)

[M-A Ruel, 2014-09-18 17:08:56]

maruel@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, sergeyberezin@chromium.org

[M-A Ruel, 2014-09-18 17:09:26]

On 2014/09/18 09:06:45, Torne wrote:
> On 2014/09/18 09:04:35, Primiano Tucci wrote:
> > There are a lot of cases in which the patch fail to apply on the CQ in
trivial
> > cases where a three way merge would have suceeded.
> 
> Well, it might not actually be a lot of cases, but it's more than zero, and
the
> normal apply method is still tried first even with -3 so this shouldn't be
able
> to make anything fail that previously succeeded :)

This was done on purpose because I wanted to the process to be relatively
deterministic and not too fuzzy. It's easy to get into a state where what was
committed was a tad too fuzzed and wasn't what was initially intended.

It's good to have an healthy level of paranoia here, it's definitely a trade
off. I'd personally advocate against but I do not work on the CQ anymore, Sergey
& Pawel have the last word.

[Primiano Tucci (use gerrit), 2014-09-18 17:42:16]

On 2014/09/18 17:09:26, M-A Ruel wrote:
> On 2014/09/18 09:06:45, Torne wrote:
> > On 2014/09/18 09:04:35, Primiano Tucci wrote:
> > > There are a lot of cases in which the patch fail to apply on the CQ in
> trivial
> > > cases where a three way merge would have suceeded.
> > 
> > Well, it might not actually be a lot of cases, but it's more than zero, and
> the
> > normal apply method is still tried first even with -3 so this shouldn't be
> able
> > to make anything fail that previously succeeded :)
> 
> This was done on purpose because I wanted to the process to be relatively
> deterministic and not too fuzzy. It's easy to get into a state where what was
> committed was a tad too fuzzed and wasn't what was initially intended.
> 
> It's good to have an healthy level of paranoia here, it's definitely a trade
> off. I'd personally advocate against but I do not work on the CQ anymore,
Sergey
> & Pawel have the last word.

Hmm but in that case (things are a bit fuzzed and the merge result will not be
what intended) most people will (am I speculating wrong?) just:
git rebase; git cl upload; retick the box
which has exactly the same effect if the three way merge is successful.
And if it is not successful, both git rebase and the CQ will barf.

[Sergey Berezin, 2014-09-18 17:44:14]

CQ no longer uses depot_tools/checkout.py, it forked its own copy (for many
complicated reasons). You'd need to modify
https://chrome-internal.googlesource.com/infra/infra_internal/+/master/commit...
.

I don't have enough data to judge one way or another about 3-way merge, but I'd
err on a side of caution, and require manual rebase until we have enough stats
to tells us otherwise. The stats I'd like to see are:

1. percentage of CLs rejected due to needing rebase, and in particular, after
all tryjobs pass (it's a bummer to rerun them again)
2. percentage of CLs that could be safely committed with a 3-way merge out of
those rejected above. (or correctly rejected if the merge fails.)

If #2 is very high (>99%), the change is definitely safe; otherwise I would
think twice before attempting it.
If #1 is >1% of all rejected CLs, then it falls under the Code Yellow false
rejection criteria and becomes high priority. Otherwise it's a nice to have.

[Paweł Hajdan Jr., 2014-09-18 23:50:59]

This is still used by apply_issue on the tryserver, right?

In that case I'm inclined to approve this.

I'm also in favor on enabling -3 on CQ.

AFAIK Gerrit allows "trivial merges" like this.

When calling for data or something otherwise burdensome to the author (is there
an obvious way to get such data?), it's good to know the actual risks - is there
something specific we're concerned about here?

[Sergey Berezin, 2014-09-19 00:15:53]

> Hmm but in that case (things are a bit fuzzed and the merge result will not be
> what intended) most people will (am I speculating wrong?) just:
> git rebase; git cl upload; retick the box
> which has exactly the same effect if the three way merge is successful.
> And if it is not successful, both git rebase and the CQ will barf.

I'm re-reading this again, and now it makes more sense to me. If 3-way merge is
basically identical to git rebase / reupload, then yes, I don't see a reason not
to do it in CQ. This is provided we indeed have enough information for a 3-way
merge in CQ (I haven't checked that yet). 

+agable@ for git-specific insight (he knows CQ side of git magic the best).

[agable, 2014-09-19 08:36:56]

On 2014/09/19 00:15:53, Sergey Berezin wrote:
> > Hmm but in that case (things are a bit fuzzed and the merge result will not
be
> > what intended) most people will (am I speculating wrong?) just:
> > git rebase; git cl upload; retick the box
> > which has exactly the same effect if the three way merge is successful.
> > And if it is not successful, both git rebase and the CQ will barf.
> 
> I'm re-reading this again, and now it makes more sense to me. If 3-way merge
is
> basically identical to git rebase / reupload, then yes, I don't see a reason
not
> to do it in CQ. This is provided we indeed have enough information for a 3-way
> merge in CQ (I haven't checked that yet). 
> 
> +agable@ for git-specific insight (he knows CQ side of git magic the best).

It appears that 'git apply -3' returns 1 when it leaves merge conflict markers
in the working tree, so I'm okay with this. If it didn't, then this would be a
horrifically breaking change.

LGTM from my side. Make sure that the same change lands for the CQ itself.

Frankly, both the cq and the trybots should be using some flavor of 'git cl
patch', not having any logic of their own.

[Primiano Tucci (use gerrit), 2014-09-19 10:26:57]

Thanks everybody for all the discussion and for pointing out that this is NOT
the same code used by the CQ (sigh).
Just to clarify, I am not going to land this until I get the same change
reviewed and ready for the CQ.
Definitely we don't want to be in a state where the trybot succeed and the CQ
fails to apply the patch. That would be irritating and definitely against the
goals of the CY.

> If 3-way merge is basically identical to git rebase / reupload, then yes.
Just to be more clear, I uploaded a concrete and realistic example in
https://codereview.chromium.org/587453007/
I wrote that demo-patch today but I pretended that I din't sync for a couple of
days, and in the meanwhile let somebody else change lines nearby.
As you can see, trybots failed to apply the patch. git apply -3 (as well as git
rebase and reupload) would have avoided this.

[Primiano Tucci (use gerrit), 2014-09-19 15:34:27]

Trying to come up with some data (see inline):


> 1. percentage of CLs rejected due to needing rebase, and in particular, after
> all tryjobs pass (it's a bummer to rerun them again)
At the moment neither tryjobs nor CQ use 3-way apply. It is IMHO unlikely that a
CL passes tryjobs and fails to apply on CQ (unless a conflicting change is
landed in the time windows between tryjobs and CQ).
What more frequently happens is that the cl fails both on all the tryjobs (and
is not even submitted to the cq) because it didn't apply cleanly (i.e. the
author didn't sync in the last N days and didn't rebase before uploading)

> 2. percentage of CLs that could be safely committed with a 3-way merge out of
> those rejected above. (or correctly rejected if the merge fails.)
I have some numbers which might give an answer probably similar to what you are
looking for.
I wrote a script (http://pastebin.com/0ZhYBCvW) which runs over all the
codereviews issues for the main project.
For each issue that has > 1 patchsets it calculates the diff between the diff
lines of the last patchset and the diff of the 2nd-last patchsets (Excluding any
context).
Rationale: if the last patchset is identical to the previous one (% context
lines) that is definitely a rebase without any same-line conflicts. It is
extremely likley that the author did such a rebase because the previous PS
failed to try/commit, and it is very likely that a three way merge would have
avoided the burden of rebasing and reuploading to the author.
Does it make sense?

If if does, this is the number: out of the last 10k commits 7.48% of them have
the aforementioned property (last patchset smells like a rebase)

[Sergey Berezin, 2014-09-19 16:31:18]

Thanks for the script! I filed a bug http://crbug.com/416003 and added it to
this issue.

[Paweł Hajdan Jr., 2014-09-23 00:58:52]

LGTM

[Primiano Tucci (use gerrit), 2014-09-23 07:50:46]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2014-09-23 07:51:13]

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

[commit-bot: I haz the power, 2014-09-23 07:53:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 07:53:24]

Presubmit check for 577203002-1 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 **
tests/checkout_test.py (9.73s) failed
Switched to branch 'master'
Already on 'master'
.F.....................
======================================================================
FAIL: testException (__main__.GitCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/checkout_test.py", line 460, in testException
    'While running git apply --index -p1;\n  fatal: corrupt patch at line '
  File "tests/checkout_test.py", line 193, in _check_exception
    self.assertEquals(e.status, err_msg)
AssertionError: 'While running git apply --index -3 -p1;\n  fatal: corrupt patch
at line 12\n' != 'While running git apply --index -p1;\n  fatal: corrupt patch
at line 12\n'

----------------------------------------------------------------------
Ran 23 tests in 9.413s

FAILED (failures=1)


Presubmit checks took 125.7s 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.

[Primiano Tucci (use gerrit), 2014-09-23 08:11:40]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2014-09-23 08:12:36]

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

[commit-bot: I haz the power, 2014-09-23 08:14:41]

Committed patchset #2 (id:20001) as 292087