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

commit dfc586bf89d91c8a9974432e9e97682cff33699e
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu Nov 30 02:04:51 2017

validation_pool: add logging around push-based submit

BUG= chromium:789619 
TEST=None

Change-Id: Id7894265a57a70a927698e8c58662813a7646ee8
Reviewed-on: https://chromium-review.googlesource.com/797622
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Paul Hobbs <phobbs@google.com>

[modify] https://crrev.com/dfc586bf89d91c8a9974432e9e97682cff33699e/cbuildbot/validation_pool.py
[modify] https://crrev.com/dfc586bf89d91c8a9974432e9e97682cff33699e/lib/git.py

Same happened again with https://chromium-review.googlesource.com/#/c/chromiumos/infra/dummies/merge-sandbox/+/797614/

in https://logs.chromium.org/v/?s=chromeos%2Fbb%2Fchromeos%2Fmaster-paladin%2F17071%2F%2B%2Frecipes%2Fsteps%2FCommitQueueHandleChanges%2F0%2Fstdout


The commit itself has history:

*   27c8011 (refs/published/branchline, branchline) Merge branch 'master' into branchline
|\  
| * 15b6e0f mainline commit
* | c709c94 (cros/branchline) branchline commit
|/  
* 7c9ba66 branchline commit baz
.
.


And in the patch application logs I can see that it was merged in (fastforward) as expected. But when it came to submitting the patch, something strange happened and the history in the tree became:

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
* e97b01d (HEAD, m/master, cros/master) branchline commit
* 59b6c16 (master) mainline commit 2
* 15b6e0f mainline commit
* 7c9ba66 branchline commit baz
.
.

Same happened again with https://chromium-review.googlesource.com/#/c/chromiumos/infra/dummies/merge-sandbox/+/797614/

in https://logs.chromium.org/v/?s=chromeos%2Fbb%2Fchromeos%2Fmaster-paladin%2F17071%2F%2B%2Frecipes%2Fsteps%2FCommitQueueHandleChanges%2F0%2Fstdout


The commit itself has history:

*   27c8011 (refs/published/branchline, branchline) Merge branch 'master' into branchline
|\  
| * 15b6e0f mainline commit
* | c709c94 (cros/branchline) branchline commit
|/  
* 7c9ba66 branchline commit baz
.
.


And in the patch application logs I can see that it was merged in (fastforward) as expected. But when it came to submitting the patch, something strange happened and the history in the tree became:

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
* e97b01d (HEAD, m/master, cros/master) branchline commit
* 59b6c16 (master) mainline commit 2
* 15b6e0f mainline commit
* 7c9ba66 branchline commit baz
.
.

The culprit is in git.py

def SyncPushBranch(git_repo, remote, rebase_target, **kwargs):                     
  """Sync and rebase a local push branch to the latest remote version.             
                                                                                   
  Args:                                                                            
    git_repo: Git repository to rebase in.                                         
    remote: The remote returned by GetTrackingBranch(for_push=True)                
    rebase_target: The branch name returned by GetTrackingBranch().  Must          
      start with refs/remotes/ (specifically must be a proper remote               
      target rather than an ambiguous name).                                       
    kwargs: Arguments passed through to RunGit.                                    
  """                                                                              
  if not rebase_target.startswith('refs/remotes/'):                                
    raise Exception(                                                               
        'Was asked to rebase to a non branch target w/in the push pathways.  '     
        'This is highly indicative of an internal bug.  remote %s, rebase %s'      
        % (remote, rebase_target))                                                 
                                                                                   
  cmd = ['remote', 'update', remote]                                               
  RunGit(git_repo, cmd, **kwargs)                                                  
                                                                                   
  try:                                                                             
    RunGit(git_repo, ['rebase', rebase_target], **kwargs)                          
  except cros_build_lib.RunCommandError:                                           
    # Looks like our change conflicts with upstream. Cleanup our failed            
    # rebase.                                                                      
    RunGit(git_repo, ['rebase', '--abort'], error_code_ok=True, **kwargs)          
    raise                                                                          



We are rebasing the repo just before pushing, and this is destroying the merge commit. (experiment below...)



~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
*   58e6fc7 (HEAD -> branch) Merge branch 'master' into branch
|\  
| * e97b01d (m/master, cros/master, master) branchline commit
* | 1c07f73 branch commit
|/  
* 59b6c16 mainline commit 2
...

~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
* bc9a288 (HEAD -> branch) branch commit
* e97b01d (m/master, cros/master, master) branchline commit
* 59b6c16 mainline commit 2



Possible resolution: iff we see that a merge commit being handled in the repo being pushed, use `git merge` instead of `git rebase` to bring the repo up to date with any changes that have since landed

(in the experiment above I forgot to paste the command "git rebase master" which is what actually destroyed the merge commit)

Isn't the resolution you proposed in #5 the same as the description of your approach in the design doc, where you have one merge commit in gerrit and another one to sync before pushing? Maybe I misread it, but I definitely remember there being two merge commits.

#7 not quite. By the time we get to SyncPushBranch we already have 2 merge commits (the original merge, and the merge of it that we create in patch.Apply) (we might only have 1 if the 2nd one was a fast-forward).

The suggestion in #5 would create another one.

ok, so if we wanted to avoid creating a 3rd, we could git reset the one we created in patch.Apply and re-create it after fetching?

> ok, so if we wanted to avoid creating a 3rd, we could git reset the one we created in patch.Apply and re-create it after fetching?

yep, that's an option. But we aren't using any of the Apply code in this pre-push-update code, and the Apply code is a bit heavy because it knows about validation pools and dependency trees.

also investigating `git rebase -p` which claims to preserve merges. 

git rebase -p seems promising, at least in the very simply case:

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
*   58e6fc7 (HEAD -> branch) Merge branch 'master' into branch
|\  
| * e97b01d (m/master, cros/master, master) branchline commit
* | 1c07f73 branch commit
|/  
* 59b6c16 mainline commit 2
...
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git rebase -p master
Successfully rebased and updated refs/heads/branch.
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph
*   58e6fc7 (HEAD -> branch) Merge branch 'master' into branch
|\  
| * e97b01d (m/master, cros/master, master) branchline commit
* | 1c07f73 branch commit
|/  
* 59b6c16 mainline commit 2
...


Let's see how it handles both a non-fast-forward in the initial apply, and a non-fast-forward in the update.

Neat!

If the original apply was a fast-forward, then git rebase -p will create a new merge. That's no good because it will destroy any conflict resolutions that the author wrote:

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph --branches
* 6e55478 (master) another mainline commit
| *   58e6fc7 (HEAD -> branch) Merge branch 'master' into branch
| |\  
| |/  
|/|   
* | e97b01d (m/master, cros/master) branchline commit
| * 1c07f73 branch commit
|/  
* 59b6c16 mainline commit 2
...
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git rebase -p master
Successfully rebased and updated refs/heads/branch.
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph --branches
*   73797fd (HEAD -> branch) Merge branch 'master' into branch
|\  
| * 6e55478 (master) another mainline commit
| * e97b01d (m/master, cros/master) branchline commit
* | 1c07f73 branch commit
|/  
* 59b6c16 mainline commit 2


We can possibly avoid this by disallowing fast-forward in that Apply.

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph --branches
* 1324743 (master) commit that landed while CQ was running
| *   a72041d (HEAD -> patchbranch) Apply-created merge, with --no-ff
| |\  
|/ /  
| *   68838c6 (branch) author-generated merge commit
| |\  
| |/  
|/|   
* | 2ba17a8 mainline commit
| * 278b996 branchline commit
|/  
* 186e9cd fresh start
...

akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git rebase -p master
Successfully rebased and updated refs/heads/patchbranch.
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph --branches
akeshet@akeshet:~/chromiumos/infra/dummies/merge-sandbox$ git log --oneline --graph --branches
*   559b31a (HEAD -> afterrebase) Apply-created merge, with --no-ff
|\  
| *   7e9d383 author-generated merge commit
| |\  
| |/  
|/|   
* | 1324743 (master) commit that landed while CQ was running
| | *   a72041d (patchbranch) Apply-created merge, with --no-ff
| | |\  
| |/ /  
|/| |   
| | *   68838c6 (branch) author-generated merge commit
| | |\  
| |/ /  
| | /   
| |/    
|/|     
* | 2ba17a8 mainline commit
| * 278b996 branchline commit
|/  
* 186e9cd fresh start
...



No good. `git rebase -p` insists on rewriting the author-generated merge, and my reading of the docs is that it will not preserve the author's fixes.

       -p, --preserve-merges
           Recreate merge commits instead of flattening the history by replaying commits a merge commit introduces. Merge conflict resolutions or manual amendments to merge commits are not
           preserved.

           This uses the --interactive machinery internally, but combining it with the --interactive option explicitly is generally not a good idea unless you know what you are doing (see BUGS
           below).

Therefore, I suggest going back to the approach suggested in #5:

1) During Apply, create a merge of the merge. This is allowed to be a fast-forward.
2) During pre-push update, if there are any merge commits for that repo being handled, create a merge. This is also allowed to be a fast forward.

That will result in a final product with 1-3 merges depending on how many intervening CLs there were.

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

commit 350e0c10953586ff5472eba1e1958b78dc045cc3
Author: Aviv Keshet <akeshet@chromium.org>
Date: Sat Dec 02 04:32:41 2017

validation_pool: add yet more logging to git push

BUG= chromium:789619 
TEST=None

Change-Id: Ie69c0f79d797792cd5e97ceacd81e33659d53d9f
Reviewed-on: https://chromium-review.googlesource.com/798603
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Paul Hobbs <phobbs@google.com>
Reviewed-by: Aviv Keshet <akeshet@chromium.org>

[modify] https://crrev.com/350e0c10953586ff5472eba1e1958b78dc045cc3/cbuildbot/validation_pool.py

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

commit 6f9c3c26ae2548f8d7f8bf053e7bed08d0bb28ca
Author: Aviv Keshet <akeshet@chromium.org>
Date: Sat Dec 02 04:32:46 2017

git: add use_merge argument to SyncPushBranch

BUG= chromium:789619 
TEST=Manual local test of `git merge` and `git merge --abort`

Change-Id: I6add480bb16dc69fa1bb4daa0c054a7308b3a380
Reviewed-on: https://chromium-review.googlesource.com/804402
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Paul Hobbs <phobbs@google.com>

[modify] https://crrev.com/6f9c3c26ae2548f8d7f8bf053e7bed08d0bb28ca/lib/git.py

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

commit 6802e180d5479e5539fe2cb70423afd9802c6374
Author: Aviv Keshet <akeshet@chromium.org>
Date: Sat Dec 02 06:45:31 2017

validation_pool: set use_merge if there are merges being landed

BUG= chromium:789619 
TEST=None

Change-Id: Ifa4e2b0315818050ca1f6a8c3debb1b5a34b840b
Reviewed-on: https://chromium-review.googlesource.com/804403
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/6802e180d5479e5539fe2cb70423afd9802c6374/cbuildbot/validation_pool.py
[modify] https://crrev.com/6802e180d5479e5539fe2cb70423afd9802c6374/cbuildbot/validation_pool_unittest.py
[modify] https://crrev.com/6802e180d5479e5539fe2cb70423afd9802c6374/lib/patch.py

Fixed. This CL is a merge commit that was handled by the CQ https://chromium-review.googlesource.com/#/c/chromiumos/infra/dummies/merge-sandbox/+/807246/-1..1

Resulting in this history (which preserves the merge history, and demonstrates the maximum case of the initial merge being non-fast-forward, and a change being chumped in the midst of a run.

*   1137bb0 (HEAD, m/master, cros/master) Merge commit '7d181ba6ae8cae59b748bc9adfb919a6f3aec473' into patch_branch
|\  
| *   7d181ba (refs/published/branch, branch) non -ff able Merge commit '59b6c16' into branch
| |\  
| * | 5676a08 (cros/branchline2) branchline commit
* | | 46246d8 a change that is chumped during the CQ run
* | | 3adb1a1 Revert "a change that is chumped during the CQ run"
* | | cd7f2a6 a change that is chumped during the CQ run
* | | b5546db (precq) add a pre-cq config for dummy repo
* | | e97b01d (cros/release-R64-10176.B, master) branchline commit
| |/  
|/|   
* | 59b6c16 mainline commit 2
|/  
* 15b6e0f mainline commit

Actually, the above doesn't demonstrate the chump-during-cq case. Oops. However, I'm pretty confident that it works too. Will prepare a separate test of it.