We could also use this feature when pulling in large number of kernel wifi driver patches from external vendors that we need to do about once a quarter.

Worth noting that support for 'git merge' will have to be accompanied to changes to the current rebase/cherry-pick. Once the CL is approved, we may need a 'git rebase -p' on the merge or a 'git cherry-pick -m 1' (at the very least, since the tree will most likely have moved forward since the merge commit). 
I am curious what happens when a merge CL is approved and the tree moves forward with conflicts. Is it worth thinking of buttons for '-X theirs' or '-s ours' for conflict resolutions? Or we do just ask for the CL to be redone? 
(all of the above were scenarios faced in merging the wifi patch trees from vendors). 

Good idea, will keep as an OKR candidate

Re #3, I would be happy if we can support just the basic case of a git merge cl that can apply cleanly first, and maybe support clean rebases. Currently, with non-merge cls if there are conflict you basically need to deal with them manually and re-upload, which is fine. The use case here is to merge CLs from cros/upstream (mirror of aosp/master) on projects that are mostly developed in AOSP.

Is there an estimate for just that?

Hi deymo@, adlr@,
If it is of any use for  bug 624889 , here's what we did to get merges into the tree: 

(1) run trybot and HWtest with the squashed merge that goes Gerrit/pre-cq. 
(2) upload the actual merge branch to a namespace on cros/Gerrit. We've created namespaces with required permissions for the people doing the merges.
(3) upload the merge CL to Gerrit the regular way - since the merge branch is already on the Gerrit server, the CL results in a single commit/patch with only the merge conflicts. 

What really matters in (2) is that the SHAs of the commits in the merge are visible to the server - you can have the same effect by pushing a tag for instance, instead of a namespaced branch.

As stated in the bug description, for the "aosp" repos we have a mirroring from the aosp/master to cros/upstream which should take care of (1) and (2) in #8. We just need to upload the a single CL that merges the SHAs that already exist upstream. This is what I would like to do:

cd src/aosp/system/update_engine/
repo sync .
repo start my_merge .
git merge cros/upstream --no-ff
# Edit the commit message, test it locally, and the upload:
repo upload . --cbr --no-verify

But this is what I get:

Writing objects: 100% (2/2), 295 bytes | 0 bytes/s, done.
Total 2 (delta 1), reused 0 (delta 0)
remote: Resolving deltas: 100% (1/1)
remote: Processing changes: refs: 1, done    
To https://chromium-review.googlesource.com/aosp/platform/system/update_engine
 ! [remote rejected] my_merge -> refs/for/master (you are not allowed to upload merges)
error: failed to push some refs to 'https://chromium-review.googlesource.com/aosp/platform/system/update_engine'



According to what Kirtika said, we should be able to simply change the gerrit permission to allot people to upload merge CLs to the aosp repos, since they did something similar for the kernel. In our case we don't need to do (2) since that's done automatically.

+sosa

He says:
Push Merge Commit is a permission that can be toggled per project by filing a ticket to Infra>Git subcomponent that goes to chrome infra.

Has someone tried this?

Puneet, 

We do have 'Push Merge Commit' permissions enabled for chromeos-kernel-owners. 
Somehow, I was able to merge Intel patches without requiring that permission, but the latest merge from groeck@ needed that switch toggled to on. 
See here: https://chromium-review.googlesource.com/#/admin/projects/chromiumos/third_party/kernel,access

I do not believe that this switch causes anything to change with the Gerrit workflow - which assumes single commit/patch in many places (e.g. the UI, 'cherrypick' doesn't support merge, 'rebase' button doesn't support merge either, and probably for good reason). 
Is there something else we should turn on in Gerrit? 

Gerrit has support for allowing developers to upload Merge Commits (https://gerrit-review.googlesource.com/Documentation/access-control.html#category_push_merge). I'm not actually sure if our current tooling will support it i.e. cq/pre-submit. I agree someone would have to try it out.

I could grant access in a few repos or you guys could request in a few and we could do an experiment and see what explodes if someone is willing to drive it.

Can we start by testing with aosp/external/minijail in the chrome OS tree?

i thought repos under the toplevel aosp/ were meant to be read-only mirrors ?

vapier@ - several repos under aosp/ have now been forked, and ChromeOS works on its copy. We can repo upload and go through CQ the same way as the other repos. See PSA on this by deymo@ (forwarded to you).  

If you flip on the upload & 'Push Merge Commit' permission, you'll be able to upload merge CLs but they will get mangled by the CQ when pushed by the CQ. Your CLs won't work with the Pre-CQ, CQ, trybots, or any cbuildbot-based infrastructure. However, if you push the CLs manually and bypass CQ/Pre-CQ/Trybots it works fine.

It's quite doable to tweak the CQ to support merge commits -- we just need to find someone to do the work.

Hi David,
What you described is what we have been doing so far - effectively the "Push Merge Commit" only allows us to upload a merge commit to Gerrit. 
Like you mentioned, so far we skip the CQ and just chump it. 

I am not sure what you mean by "will get mangled by the CQ when pushed by the CQ". We have not gone the CQ route for merges yet, preferring chumps instead, but would be good to understand this. 
Will the CQ run on every individual patch? In what order does it test the patches? 

If you push the changes manually (chumping) there is no problem. However if you send the changes through the CQ (e.g. by hitting "commit ready"), the CLs will not be pushed correctly and you may end up with corrupted changes (e.g., the change will get pushed but your changes will not go in).

How hard will it be to support this in the CQ, given the Gerrit config changes?

Reasonable starter project for someone with strong git skills?

It's relatively easy. Should be doable in less than a week. It's mostly just updating all the places that use "git cherry-pick" to pass in the appropriate arguments to merge a "merge". (See man git-cherry-pick for details.)

You may also need to take a look at the places we use 'git log' to make sure it doesn't get confused by merges -- we do some trawling of the history so we'll want to make sure we understand the two-parent structure.

Is there any update on this?

The priority on our end dropped as we migrated to use the AOSP copy of minijail directly.

chingcodes@ is working on a design doc

Bump. Where are we on this?  We do merges fairly often in kernel land and not having this ability results in chumping in changes and that is bad for obvious reasons.

I'll try picking this up with a few spare cycles. Plan of attack: Create a dummy repo to play with and see how merges are handled today, assess how much work it would take to support this better.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/7f6c06c516e03b7864c8d6bfb619e898eb217bf2

commit 7f6c06c516e03b7864c8d6bfb619e898eb217bf2
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu Nov 09 06:21:49 2017

add dummies/merge-sandbox

CQ-DEPEND=CL:*499514
BUG= chromium:614164 
TEST=None

Change-Id: Ifb4160a1dc608f820e8d2eebe74845a8423ad997
Reviewed-on: https://chromium-review.googlesource.com/758632
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/7f6c06c516e03b7864c8d6bfb619e898eb217bf2/full.xml

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/4ebb363d69f7e13784f4bf05de34701afabb49ce

commit 4ebb363d69f7e13784f4bf05de34701afabb49ce
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu Nov 09 06:21:49 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/7c6c7f0920cf44ed31a29bd4368614812cbd92fe

commit 7c6c7f0920cf44ed31a29bd4368614812cbd92fe
Author: Aviv Keshet <akeshet@chromium.org>
Date: Wed Nov 22 05:36:15 2017

patch: add _FromSha1 method

BUG= chromium:614164 
TEST=new unit test

Change-Id: I0987f1a4f42f13aa2779dc8581896c52e9817dcf
Reviewed-on: https://chromium-review.googlesource.com/780325
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Aviv Keshet <akeshet@chromium.org>

[modify] https://crrev.com/7c6c7f0920cf44ed31a29bd4368614812cbd92fe/lib/patch_unittest.py
[modify] https://crrev.com/7c6c7f0920cf44ed31a29bd4368614812cbd92fe/lib/patch.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/76779ca1192d94d912be8fab26159e16749d3402

commit 76779ca1192d94d912be8fab26159e16749d3402
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Nov 28 21:34:27 2017

patch: add _ValidateMerge method

BUG= chromium:614164 
TEST=new unit tests

Change-Id: I5fbd995728fb0e2b4b79e69864dd533ee3593e38
Reviewed-on: https://chromium-review.googlesource.com/782650
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Aviv Keshet <akeshet@chromium.org>

[modify] https://crrev.com/76779ca1192d94d912be8fab26159e16749d3402/lib/patch_unittest.py
[modify] https://crrev.com/76779ca1192d94d912be8fab26159e16749d3402/lib/patch.py

This appears to be working.

One possible follow-up / TODO: 
 - merge CLs today aren't being cherrypicked or rebased or anything, and that means we aren't appending any of the usual helpful text to their commit message. It should be possible to add this...

Major support is done, a few cleanups remain.

Some optional follow up bugs exist, but main feature work is done. I'm not working on this actively now. Marking fixed.

For posterity, most of the work to support this feature was accomplished in blocking bug  Issue 786637