Reduce extra merges by cherry-picking parallel builds to branch

Fixes: QTQAINFRA-4345
Change-Id: I81893d21c561aceac38d452c7382781be6cb7f8a
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

4 files changed, 300 insertions, 38 deletions

diff --git a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApprove.java b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApprove.java
index fc6eca9..a679c9a 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApprove.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApprove.java
@@ -183,17 +183,6 @@ class QtCommandBuildApprove extends SshCommand {
       if (git.resolve(destBranchKey.branch()) == null) throw die("branch not found");
       if (git.resolve(buildBranchKey.branch()) == null) throw die("build not found");
 
-      // Initialize and populate open changes list.
-      affectedChanges = qtUtil.listChangesNotMerged(git, buildBranchKey, destBranchKey);
-
-      // Notify user that build did not have any open changes. The build has already been approved.
-      if (affectedChanges.isEmpty()) {
-        logger.atInfo().log(
-            "qtcodereview: staging-approve build %s already in project %s branch %s",
-            buildBranch, projectKey, destBranchKey);
-        throw die("No open changes in the build branch");
-      }
-
       if (result.toLowerCase().equals(PASS)) {
         approveBuildChanges();
       } else if (result.toLowerCase().equals(FAIL)) {
@@ -212,8 +201,6 @@ class QtCommandBuildApprove extends SshCommand {
       throw die(e.getMessage());
     } catch (QtUtil.BranchNotFoundException e) {
       throw die("invalid branch " + e.getMessage());
-    } catch (NoSuchRefException e) {
-      throw die("invalid reference " + e.getMessage());
     } catch (UpdateException | RestApiException | ConfigInvalidException e) {
       logger.atSevere().log("qtcodereview: staging-napprove failed to update change status %s", e);
       throw die("Failed to update change status");
@@ -228,20 +215,25 @@ class QtCommandBuildApprove extends SshCommand {
   }
 
   private void approveBuildChanges()
-      throws QtUtil.MergeConflictException, NoSuchRefException, IOException, UpdateException,
-          RestApiException, ConfigInvalidException {
+      throws QtUtil.MergeConflictException, IOException, UpdateException, UnloggedFailure,
+          RestApiException, ConfigInvalidException, QtUtil.BranchNotFoundException {
     if (message == null) message = String.format("Change merged into branch %s", destBranchKey);
 
     ObjectId oldId = git.resolve(destBranchKey.branch());
 
-    Result result = QtUtil.mergeBranches(user.asIdentifiedUser(), git, buildBranchKey,
-        destBranchKey, "Merge integration " + buildBranch);
-
-    if (result != Result.FAST_FORWARD) {
+    try {
+      affectedChanges = qtUtil.mergeIntegrationToBranch(user.asIdentifiedUser(), git, projectKey,
+          buildBranchKey, destBranchKey, "Merge integration " + buildBranch);
+    } catch (NoSuchRefException e) {
+      message = "Gerrit plugin internal error. Please contact Gerrit Admin.";
+      logger.atInfo().log(e.getMessage());
+      rejectBuildChanges();
+      return;
+    } catch (QtUtil.MergeConflictException e) {
       message = "Unable to merge this integration because another integration parallel to this one "
               + "successfully merged first and created a conflict in one of the tested changes.\n"
               + "Please review, resolve conflicts if necessary, and restage.";
-      logger.atInfo().log(message);
+      logger.atInfo().log(e.getMessage());
       rejectBuildChanges();
       return;
     }
@@ -266,9 +258,19 @@ class QtCommandBuildApprove extends SshCommand {
 
   private void rejectBuildChanges()
       throws QtUtil.MergeConflictException, UpdateException, RestApiException, IOException,
-          ConfigInvalidException {
+          ConfigInvalidException, QtUtil.BranchNotFoundException, UnloggedFailure {
     if (message == null) message = String.format("Change rejected for branch %s", destBranchKey);
 
+    affectedChanges = qtUtil.listChangesNotMerged(git, buildBranchKey, destBranchKey);
+
+    // Notify user that build did not have any open changes. The build has already been approved.
+    if (affectedChanges.isEmpty()) {
+      logger.atInfo().log(
+          "staging-approve build %s already in project %s branch %s",
+          buildBranch, projectKey, destBranchKey);
+      throw die("No open changes in the build branch");
+    }
+
     updateChanges(
         affectedChanges,
         Change.Status.NEW,
@@ -360,9 +362,6 @@ class QtCommandBuildApprove extends SshCommand {
     PatchSet ps = changeData.currentPatchSet();
     changeMerged.fire(
         changeData.change(), ps, user.asIdentifiedUser().state(), ps.commitId().name(), ts);
-
-    // logger.atInfo().log("qtcodereview: staging-approve sending merge event failed for %s",
-    //                     changeData.change());
   }
 
   private void readMessageParameter() throws UnloggedFailure {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java
index 82ec2ce..a487fed 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java
@@ -34,9 +34,13 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.events.ChangeEvent;
 import com.google.gerrit.server.events.EventDispatcher;
 import com.google.gerrit.server.events.EventFactory;
+import com.google.gerrit.server.git.CodeReviewCommit;
+import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.NoSuchRefException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.submit.IntegrationException;
@@ -50,6 +54,7 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.AbstractMap;
 import java.util.ArrayList;
+import java.util.Date;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -60,6 +65,7 @@ import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
@@ -92,6 +98,8 @@ public class QtUtil {
   private final ChangeNotes.Factory changeNotesFactory;
   private final DynamicItem<EventDispatcher> eventDispatcher;
   private final EventFactory eventFactory;
+  private final MergeUtil.Factory mergeUtilFactory;
+  private final ProjectCache projectCache;
   private final QtCherryPickPatch qtCherryPickPatch;
   private final QtChangeUpdateOp.Factory qtUpdateFactory;
   private final QtEmailSender qtEmailSender;
@@ -104,6 +112,8 @@ public class QtUtil {
       ChangeNotes.Factory changeNotesFactory,
       EventFactory eventFactory,
       DynamicItem<EventDispatcher> eventDispatcher,
+      MergeUtil.Factory mergeUtilFactory,
+      ProjectCache projectCache,
       QtCherryPickPatch qtCherryPickPatch,
       QtChangeUpdateOp.Factory qtUpdateFactory,
       QtEmailSender qtEmailSender) {
@@ -113,6 +123,8 @@ public class QtUtil {
     this.changeNotesFactory = changeNotesFactory;
     this.eventDispatcher = eventDispatcher;
     this.eventFactory = eventFactory;
+    this.mergeUtilFactory = mergeUtilFactory;
+    this.projectCache = projectCache;
     this.qtCherryPickPatch = qtCherryPickPatch;
     this.qtUpdateFactory = qtUpdateFactory;
     this.qtEmailSender = qtEmailSender;
@@ -604,11 +616,10 @@ public class QtUtil {
     String message = customCommitMessage;
     if (message == null) {
       try {
-          message = revWalk.parseCommit(toMerge).getShortMessage();
+          message = "Merge \"" + revWalk.parseCommit(toMerge).getShortMessage() + "\"";
         } catch (Exception e) {
-          message = toMerge.toString();
+          message = "Merge";
         }
-      message = "Merge \"" + toMerge.toString() + "\"";
     }
 
     final CommitBuilder mergeCommit = new CommitBuilder();
@@ -675,6 +686,213 @@ public class QtUtil {
     }
   }
 
+  private RefUpdate.Result fastForwardBranch(Repository git,
+      String branchName, ObjectId toObjectId) {
+
+    RefUpdate.Result result = null;
+
+    try {
+      RefUpdate refUpdate = git.updateRef(branchName);
+      refUpdate.setNewObjectId(toObjectId);
+      refUpdate.setForceUpdate(false);
+      result = refUpdate.update();
+      logger.atInfo().log("fastforward branch %s to %s, result: %s", branchName,
+          toObjectId.name(), result);
+    } catch (Exception e) {
+      result = null;
+      logger.atWarning().log("fastforward failed for %s: %s", branchName, e);
+    }
+
+    return result;
+  }
+
+  private List<RevCommit> listCommitsInIntegrationBranch(Repository git,
+      ObjectId integrationHeadId, ObjectId targetBranchHeadId) {
+
+    List<RevCommit> commits = new ArrayList<RevCommit>();
+    int count = 0;
+
+    try {
+      RevWalk revWalk = new RevWalk(git);
+      RevCommit commit = revWalk.parseCommit(integrationHeadId);
+      RevCommit targetHeadCommit = revWalk.parseCommit(targetBranchHeadId);
+
+      do {
+        if (revWalk.isMergedInto(commit, targetHeadCommit)) {
+          commit = null;
+        } else {
+          commits.add(0, commit);
+          if (commit.getParentCount() > 0) {
+            // Qt Gerrit plugins's merges always have the branch as parent 0
+            commit = revWalk.parseCommit(commit.getParent(0));
+          } else commit = null;
+        }
+        count++;
+      } while (commit != null && count < 100);
+    } catch (Exception e) {
+      commits = null;
+      logger.atWarning().log("listing commits in a branch failed: %s", e);
+    }
+
+    if (count >= 100) {
+      logger.atWarning().log("listing commits in a branch failed: too many commmits");
+      return null;
+    }
+
+    return commits;
+  }
+
+  private Boolean isCherryPickingAllowed(List<RevCommit> commits) {
+    // Cherry-picking merge commits is not allowed, because it would squash
+    // the merge into one single commit
+    for (RevCommit commit : commits) {
+      if (commit.getParentCount() > 1) return false;
+    }
+    return true;
+  }
+
+  private List<RevCommit> cherryPickCommitsToBranch(Repository git,
+      Project.NameKey project, String branchName, List<RevCommit> commits) {
+
+    try {
+      ProjectState projectState = projectCache.checkedGet(project);
+      MergeUtil mergeUtil = mergeUtilFactory.create(projectState, true);
+      ObjectInserter objInserter = git.newObjectInserter();
+      ObjectReader reader = objInserter.newReader();
+      CodeReviewCommit.CodeReviewRevWalk revWalk = CodeReviewCommit.newRevWalk(reader);
+      RevCommit newBranchHead = revWalk.parseCommit(git.resolve(branchName));
+
+      List<RevCommit> cherryPicked = new ArrayList<RevCommit>();
+
+      for (RevCommit commit : commits) {
+        CodeReviewCommit cherryPickCommit =
+            mergeUtil.createCherryPickFromCommit(
+                objInserter,
+                git.getConfig(),
+                newBranchHead,
+                commit,
+                new PersonIdent(commit.getCommitterIdent(), new Date()),
+                commit.getFullMessage(),
+                revWalk,
+                0,
+                true, // ignoreIdenticalTree
+                false); // allowConflicts
+        objInserter.flush();
+        logger.atInfo().log("created cherrypick commit %s from %s", cherryPickCommit.name(),
+            commit.name());
+        newBranchHead = cherryPickCommit;
+        cherryPicked.add(cherryPickCommit);
+      }
+
+      RefUpdate.Result result = fastForwardBranch(git, branchName, newBranchHead);
+      if (result != RefUpdate.Result.FAST_FORWARD) return null;
+
+      return cherryPicked;
+    } catch (Exception e) {
+      logger.atWarning().log("cherrypicking commits to branch failed: %s", e);
+      return null;
+    }
+  }
+
+  private List<Map.Entry<ChangeData, RevCommit>> listChanges(Repository git,
+      BranchNameKey destination, List<RevCommit> commits) throws Exception {
+
+    Map<Change.Id, Map.Entry<ChangeData, RevCommit>> map = new HashMap<>();
+
+    for (RevCommit commit : commits) {
+      String changeId = getChangeId(commit);
+      List<ChangeData> changes = null;
+
+      if (changeId == null && commit.getParentCount() > 1) {
+        // Merge commit without Change-Id, so done by plugin: changes merged in will be parent 1
+        changeId = getChangeId(commit.getParent(1));
+      }
+
+      if (changeId != null) {
+        Change.Key key = Change.key(changeId);
+        changes = queryProvider.get().byBranchKey(destination, key);
+      }
+
+      if (changes != null && !changes.isEmpty()) {
+        if (changes.size() > 1) {
+          String msg = String.format("Same Change-Id in several changes on same branch: %s",
+              commit.name());
+          throw new Exception(msg);
+        }
+        ChangeData cd = changes.get(0);
+        map.put(cd.getId(), new AbstractMap.SimpleEntry<ChangeData, RevCommit>(cd, commit));
+      }
+    }
+    return new ArrayList<Map.Entry<ChangeData, RevCommit>>(map.values());
+  }
+
+  public List<Map.Entry<ChangeData, RevCommit>> mergeIntegrationToBranch(
+      IdentifiedUser user,
+      Repository git,
+      Project.NameKey project,
+      final BranchNameKey integrationBranch,
+      final BranchNameKey targetBranch,
+      String customCommitMessage)
+      throws MergeConflictException, NoSuchRefException {
+
+    List<RevCommit> commitsInBranch = null;
+    RevCommit newBranchHead = null;
+    RefUpdate.Result result  = null;
+    ObjectId sourceId = null;
+    ObjectId targetId = null;
+
+    logger.atInfo().log("start merging integration %s to %s", integrationBranch.branch(),
+        targetBranch.branch());
+
+    try {
+      sourceId = git.resolve(integrationBranch.branch());
+      if (sourceId == null) throw new NoSuchRefException("Invalid Revision: " + integrationBranch);
+
+      Ref targetRef = git.getRefDatabase().getRef(targetBranch.branch());
+      if (targetRef == null) throw new NoSuchRefException("No such branch: " + targetBranch);
+
+      targetId = git.resolve(targetBranch.branch());
+      if (targetId == null) throw new NoSuchRefException("Invalid Revision: " + targetBranch);
+
+      if (sourceId.equals(targetId)) throw new NoSuchRefException("Nothing to merge");
+
+      commitsInBranch = listCommitsInIntegrationBranch(git, sourceId, targetId);
+      if (commitsInBranch == null)
+          throw new NoSuchRefException("Failed to list commits in " + integrationBranch);
+      else if (commitsInBranch.isEmpty())
+          throw new NoSuchRefException("No commits in " + integrationBranch);
+    } catch (Exception e) {
+      logger.atWarning().log("preconditions of merging integration failed: %s", e);
+      throw new NoSuchRefException(e.getMessage());
+    }
+
+    try {
+      logger.atInfo().log("Trying to fast forward...");
+      result = fastForwardBranch(git, targetBranch.branch(), sourceId);
+      if (result == RefUpdate.Result.FAST_FORWARD)
+        return listChanges(git, targetBranch, commitsInBranch);
+
+      if (isCherryPickingAllowed(commitsInBranch)) {
+        logger.atInfo().log("Trying to rebase integration onto the target branch...");
+        List<RevCommit> cherrypicks = cherryPickCommitsToBranch(git, project, targetBranch.branch(), commitsInBranch);
+        if (cherrypicks != null) return listChanges(git, targetBranch, cherrypicks);
+      }
+
+      logger.atInfo().log("Trying to create merge commit...");
+      List<Map.Entry<ChangeData, RevCommit>> mergedCommits =
+          listChangesNotMerged(git, integrationBranch, targetBranch);
+      result = mergeBranches(user, git, integrationBranch, targetBranch, customCommitMessage);
+      if (result != RefUpdate.Result.FAST_FORWARD) throw new Exception("Merge conflict");
+      return mergedCommits;
+    } catch (Exception e) {
+      result = null;
+      logger.atWarning().log("Merging integration %s to %s failed: %s",
+          integrationBranch.branch(), targetBranch.branch(), e.getMessage());
+      throw new MergeConflictException("Merge conflict:" + integrationBranch.branch() +
+                                       " to " + targetBranch.branch());
+    }
+  }
+
   private Supplier<ChangeAttribute> changeAttributeSupplier(Change change, ChangeNotes notes) {
     return Suppliers.memoize(
         () -> {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCodeReviewIT.java b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCodeReviewIT.java
index 9af06ed..98b03f4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCodeReviewIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCodeReviewIT.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2019 The Qt Company
+// Copyright (C) 2021 The Qt Company
 
 package com.googlesource.gerrit.plugins.qtcodereview;
 
@@ -380,4 +380,11 @@ public class QtCodeReviewIT extends LightweightPluginDaemonTest {
   protected RevCommit getRemoteHead() throws Exception {
     return getRemoteHead(project, "master");
   }
+
+  protected RevCommit loadCommit(RevCommit commit) throws Exception {
+      Repository repo = repoManager.openRepository(project);
+      RevWalk revWalk = new RevWalk(repo);
+      return revWalk.parseCommit(commit);
+  }
+
 }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApproveIT.java b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApproveIT.java
index c95fc6f..7f4c5e2 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApproveIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtCommandBuildApproveIT.java
@@ -151,7 +151,7 @@ public class QtCommandBuildApproveIT extends QtCodeReviewIT {
   }
 
   @Test
-  public void parallelBuilds_MergeCommitVerify() throws Exception {
+  public void parallelBuilds_CherryPicksVerify() throws Exception {
     // created 3 parallel builds
     RevCommit initialHead = getRemoteHead();
     PushOneCommit.Result c1 = pushCommit("master", "commitmsg1", "file1", "content1");
@@ -178,16 +178,18 @@ public class QtCommandBuildApproveIT extends QtCodeReviewIT {
     QtNewBuild("master", "test-build-parallel-3");
 
     RevCommit updatedHead = qtApproveBuild("master", "test-build-parallel-1", c1, false);
-    String commitMessage = updatedHead.getFullMessage();
-    assertThat(commitMessage).doesNotContain("Merge");
+    assertThat(updatedHead.getFullMessage()).doesNotContain("Merge");
+    assertThat(updatedHead.getShortMessage()).isEqualTo("commitmsg1");
 
-    updatedHead = qtApproveBuild("master", "test-build-parallel-2", c2, true);
-    commitMessage = updatedHead.getFullMessage();
-    assertThat(commitMessage).contains("Merge integration test-build-parallel-2");
+    updatedHead = qtApproveBuild("master", "test-build-parallel-2", c2, false);
+    assertThat(updatedHead.getFullMessage()).doesNotContain("Merge");
+    assertThat(updatedHead.getShortMessage()).isEqualTo("commitmsg2");
 
-    updatedHead = qtApproveBuild("master", "test-build-parallel-3", c5, true);
-    commitMessage = updatedHead.getFullMessage();
-    assertThat(commitMessage).contains("Merge integration test-build-parallel-3");
+    updatedHead = qtApproveBuild("master", "test-build-parallel-3", c5, false);
+    assertThat(updatedHead.getFullMessage()).doesNotContain("Merge");
+    assertThat(updatedHead.getShortMessage()).isEqualTo("commitmsg5");
+    assertCherryPick(updatedHead.getParent(0), c4.getCommit(), null);
+    assertCherryPick(loadCommit(updatedHead.getParent(0)).getParent(0), c3.getCommit(), null);
 
     assertStatusMerged(c1.getChange().change());
     assertStatusMerged(c2.getChange().change());
@@ -197,6 +199,41 @@ public class QtCommandBuildApproveIT extends QtCodeReviewIT {
   }
 
   @Test
+  public void parallelBuilds_MergeCommitIntegrationVerify() throws Exception {
+
+    // make a change on feature branch
+    final PushOneCommit.Result f1 = pushCommit("feature", "f1-commitmsg", "f1-file", "f1-content");
+    approve(f1.getChangeId());
+    gApi.changes().id(f1.getCommit().getName()).current().submit();
+
+    // make a change on master branch
+    final PushOneCommit.Result m1 = pushCommit("master", "m1-commitmsg", "m1-file", "m1-content");
+    approve(m1.getChangeId());
+    gApi.changes().id(m1.getCommit().getName()).current().submit();
+
+    // Start a build
+    RevCommit initialHead = getRemoteHead();
+    PushOneCommit.Result c1 = pushCommit("master", "commitmsg1", "file1", "content1");
+    approve(c1.getChangeId());
+    QtStage(c1);
+    QtNewBuild("master", "test-build-parallel");
+
+    // Start parallel build with a merge commit
+    final PushOneCommit mm = pushFactory.create(admin.newIdent(), testRepo);
+    mm.setParents(ImmutableList.of(f1.getCommit(), m1.getCommit()));
+    final PushOneCommit.Result m = mm.to("refs/for/master");
+    m.assertOkStatus();
+    approve(m.getChangeId());
+    QtStage(m);
+    QtNewBuild("master", "test-build-parallel-with-merge");
+
+    RevCommit updatedHead = qtApproveBuild("master", "test-build-parallel", c1, false);
+
+    updatedHead = qtApproveBuild("master", "test-build-parallel-with-merge", m, true);
+    assertThat(updatedHead.getFullMessage()).contains("Merge integration test-build-parallel-with-merge");
+  }
+
+  @Test
   public void cherryPicked_Stays_Intact_After_Merge_And_Build() throws Exception {
     // make a change on feature branch
     final PushOneCommit.Result f1 = pushCommit("feature", "f1-commitmsg", "f1-file", "f1-content");
@@ -474,7 +511,8 @@ public class QtCommandBuildApproveIT extends QtCodeReviewIT {
     RevCommit updatedHead = getRemoteHead(project, branchRef);
     if (expectMerge) {
       assertThat(updatedHead.getParentCount()).isEqualTo(2);
-      assertCherryPick(updatedHead.getParent(1), expectedContent.getCommit(), null);
+      assertThat(updatedHead.getName()).isNotEqualTo(expectedContent.getCommit().getName());
+      assertThat(updatedHead.getShortMessage()).contains("Merge");
       assertThat(updatedHead.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email());
       assertThat(updatedHead.getCommitterIdent().getEmailAddress()).isEqualTo(admin.email());
     } else {