Reduce unnecessary merge of merge commits

Allow fast forward for merge commits if parent index 0 is correct.

Task-number: QTQAINFRA-3078
Change-Id: I217f8e6c6688aa4179e2b013c75b0910418c84b9
Reviewed-by: Paul Wicking <paul.wicking@qt.io>
Reviewed-by: Kari Oikarinen <kari.oikarinen@qt.io>

4 files changed, 127 insertions, 24 deletions

diff --git a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCherryPickPatch.java b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCherryPickPatch.java
index 061d341..8295e39 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCherryPickPatch.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtCherryPickPatch.java
@@ -128,13 +128,21 @@ public class QtCherryPickPatch {
                 // Merge commit cannot be cherrypicked
                 logger.atInfo().log("qtcodereview: merge commit detected %s", commitToCherryPick);
                 mergeCommit = true;
-                RevCommit commit = QtUtil.merge(committerIdent,
-                                                git, oi,
-                                                revWalk,
-                                                commitToCherryPick,
-                                                baseCommit,
-                                                true /* merge always */);
-                cherryPickCommit = revWalk.parseCommit(commit);
+
+                if (commitToCherryPick.getParent(0).equals(baseCommit)) {
+                    // allow fast forward, when parent index 0 is correct
+                    logger.atInfo().log("qtcodereview: merge commit fast forwarded");
+                    cherryPickCommit = commitToCherryPick;
+                } else {
+                    logger.atInfo().log("qtcodereview: merge of merge created");
+                    RevCommit commit = QtUtil.merge(committerIdent,
+                                                    git, oi,
+                                                    revWalk,
+                                                    commitToCherryPick,
+                                                    baseCommit,
+                                                    true);
+                    cherryPickCommit = revWalk.parseCommit(commit);
+                }
             } else {
                 String commitMessage = mergeUtil.createCommitMessageOnSubmit(commitToCherryPick, baseCommit);
                 cherryPickCommit = mergeUtil.createCherryPickFromCommit(oi,
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 c72f8c3..cfabba8 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/qtcodereview/QtUtil.java
@@ -318,6 +318,7 @@ public class QtUtil {
                 ChangeData change = findChangeFromList(changeId, changeList);
                 if (change != null) results.add(0, change);
 
+                // It can always be trusted that parent in index 0 is the correct one
                 commit = revWalk.parseCommit(commit.getParent(0));
             }
         } while (commit != null && !commit.equals(tipObj) && count < 100);
@@ -536,7 +537,7 @@ public class QtUtil {
 
         final CommitBuilder mergeCommit = new CommitBuilder();
         mergeCommit.setTreeId(merger.getResultTreeId());
-        mergeCommit.setParentIds(mergeTip, toMerge);
+        mergeCommit.setParentIds(mergeTip, toMerge); // important: mergeTip must be parent index 0
         mergeCommit.setAuthor(toMerge.getAuthorIdent());
         mergeCommit.setCommitter(committerIdent);
         mergeCommit.setMessage(message);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtStageIT.java b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtStageIT.java
index 343de8d..101694e 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtStageIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtStageIT.java
@@ -101,7 +101,42 @@ public class QtStageIT extends QtCodeReviewIT {
         PushOneCommit.Result m = mm.to("refs/for/master");
         m.assertOkStatus();
         approve(m.getChangeId());
-        RevCommit stagingHead = qtStageExpectMerge(m);
+        RevCommit stagingHead = qtStageExpectMergeFastForward(m);
+
+        // check that all commits are in staging ref
+        String gitLog = getRemoteLog("refs/staging/master").toString();
+        assertThat(gitLog).contains(initialHead.getId().name());
+        assertThat(gitLog).contains(c1.getCommit().getId().name());
+        assertThat(gitLog).contains(f1.getCommit().getId().name());
+        assertThat(gitLog).contains(f2.getCommit().getId().name());
+        assertThat(gitLog).contains(m.getCommit().getId().name());
+    }
+
+    @Test
+    public void mergeCommit_Stage_ExpectMergeOfMerge() throws Exception {
+        RevCommit initialHead = getRemoteHead();
+
+        // make changes on feature branch
+        PushOneCommit.Result f1 = pushCommit("feature", "commitmsg1", "file1", "content1");
+        PushOneCommit.Result f2 = pushCommit("feature", "commitmsg2", "file2", "content2");
+        approve(f1.getChangeId());
+        gApi.changes().id(f1.getChangeId()).current().submit();
+        approve(f2.getChangeId());
+        gApi.changes().id(f2.getChangeId()).current().submit();
+
+        // make a change on master branch
+        testRepo.reset(initialHead);
+        PushOneCommit.Result c1 = pushCommit("master", "commitmsg3", "file3", "content3");
+        approve(c1.getChangeId());
+        gApi.changes().id(c1.getChangeId()).current().submit();
+
+        // merge feature branch into master
+        PushOneCommit mm = pushFactory.create(db, admin.getIdent(), testRepo);
+        mm.setParents(ImmutableList.of(f2.getCommit(), c1.getCommit()));
+        PushOneCommit.Result m = mm.to("refs/for/master");
+        m.assertOkStatus();
+        approve(m.getChangeId());
+        RevCommit stagingHead = qtStageExpectMergeOfMerge(m);
 
         // check that all commits are in staging ref
         String gitLog = getRemoteLog("refs/staging/master").toString();
@@ -224,14 +259,18 @@ public class QtStageIT extends QtCodeReviewIT {
     }
 
     private RevCommit qtStage(PushOneCommit.Result c) throws Exception {
-        return qtStage(c, false);
+        return qtStage(c, false, false);
+    }
+
+    private RevCommit qtStageExpectMergeFastForward(PushOneCommit.Result c) throws Exception {
+        return qtStage(c, true, true);
     }
 
-    private RevCommit qtStageExpectMerge(PushOneCommit.Result c) throws Exception {
-        return qtStage(c, true);
+    private RevCommit qtStageExpectMergeOfMerge(PushOneCommit.Result c) throws Exception {
+        return qtStage(c, true, false);
     }
 
-    private RevCommit qtStage(PushOneCommit.Result c, boolean merge) throws Exception {
+    private RevCommit qtStage(PushOneCommit.Result c, boolean merge, boolean fastForward) throws Exception {
         String branch = getBranchNameFromRef(c.getChange().change().getDest().get());
         String stagingRef = R_STAGING + branch;
         String branchRef = R_HEADS + branch;
@@ -248,7 +287,9 @@ public class QtStageIT extends QtCodeReviewIT {
 
         RevCommit stagingHead = getRemoteHead(project, stagingRef);
 
-        if (merge) {
+        if (fastForward) {
+            assertThat(stagingHead).isEqualTo(originalCommit);
+        } else if (merge) {
             assertThat(stagingHead.getParentCount()).isEqualTo(2);
             assertThat(stagingHead.getParent(1)).isEqualTo(originalCommit);
         } else {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtUnStageIT.java b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtUnStageIT.java
index 06b249e..5ff3f15 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtUnStageIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/qtcodereview/QtUnStageIT.java
@@ -169,7 +169,7 @@ public class QtUnStageIT extends QtCodeReviewIT {
     }
 
     @Test
-    public void multiChange_UnStage_Before_MergeCommit() throws Exception {
+    public void multiChange_UnStage_Before_MergeCommit_ExpectFastForward() throws Exception {
         RevCommit initialHead = getRemoteHead();
 
         // make changes on feature branch
@@ -197,10 +197,54 @@ public class QtUnStageIT extends QtCodeReviewIT {
         mm.setParents(ImmutableList.of(c1.getCommit(), f2.getCommit()));
         PushOneCommit.Result m = mm.to("refs/for/master");
         m.assertOkStatus();
+        RevCommit originalMergeCommit = m.getCommit();
         approve(m.getChangeId());
         QtStage(m);
 
-        RevCommit stagingHead = qtUnStageExpectMerge(c2, m);
+        RevCommit stagingHead = qtUnStageExpectMergeFastForward(c2, originalMergeCommit);
+        String gitLog = getRemoteLog("refs/staging/master").toString();
+        assertThat(gitLog).contains(initialHead.getId().name());
+        assertThat(gitLog).contains(c1.getCommit().getId().name());
+        assertThat(gitLog).contains(stagingHead.getId().name());
+        assertThat(gitLog).contains(f1.getCommit().getId().name());
+        assertThat(gitLog).contains(f2.getCommit().getId().name());
+        assertThat(gitLog).contains(m.getCommit().getId().name());
+        assertThat(gitLog).doesNotContain(c2.getCommit().getId().name());
+    }
+
+    @Test
+    public void multiChange_UnStage_Before_MergeCommit_ExpectMergeOfMerge() throws Exception {
+        RevCommit initialHead = getRemoteHead();
+
+        // make changes on feature branch
+        PushOneCommit.Result f1 = pushCommit("feature", "commitmsg1", "file1", "content1");
+        PushOneCommit.Result f2 = pushCommit("feature", "commitmsg2", "file2", "content2");
+        approve(f1.getChangeId());
+        gApi.changes().id(f1.getChangeId()).current().submit();
+        approve(f2.getChangeId());
+        gApi.changes().id(f2.getChangeId()).current().submit();
+
+        // make a change on master branch
+        testRepo.reset(initialHead);
+        PushOneCommit.Result c1 = pushCommit("master", "commitmsg3", "file3", "content3");
+        approve(c1.getChangeId());
+        gApi.changes().id(c1.getChangeId()).current().submit();
+
+        // Stage a change
+        testRepo.reset(initialHead);
+        PushOneCommit.Result c2 = pushCommit("master", "commitmsg4", "file4", "content4");
+        approve(c2.getChangeId());
+        QtStage(c2);
+
+        // merge feature branch and stage it on top
+        PushOneCommit mm = pushFactory.create(db, admin.getIdent(), testRepo);
+        mm.setParents(ImmutableList.of(f2.getCommit(),c1.getCommit()));
+        PushOneCommit.Result m = mm.to("refs/for/master");
+        m.assertOkStatus();
+        approve(m.getChangeId());
+        QtStage(m);
+
+        RevCommit stagingHead = qtUnStageExpectMergeOfMerge(c2, m);
         String gitLog = getRemoteLog("refs/staging/master").toString();
         assertThat(gitLog).contains(initialHead.getId().name());
         assertThat(gitLog).contains(c1.getCommit().getId().name());
@@ -295,25 +339,32 @@ public class QtUnStageIT extends QtCodeReviewIT {
     private RevCommit qtUnStageExpectCherryPick(PushOneCommit.Result c,
                                                 PushOneCommit.Result expectedContent)
                                                 throws Exception {
-        return qtUnStage(c, null, expectedContent, false);
+        return qtUnStage(c, null, expectedContent, false, false);
     }
 
     private RevCommit qtUnStageExpectCommit(PushOneCommit.Result c,
                                            RevCommit expectedStagingHead)
                                            throws Exception {
-        return qtUnStage(c, expectedStagingHead, null, false);
+        return qtUnStage(c, expectedStagingHead, null, false, false);
     }
 
-    private RevCommit qtUnStageExpectMerge(PushOneCommit.Result c,
-                                           PushOneCommit.Result expectedContent)
-                                           throws Exception {
-        return qtUnStage(c, null, expectedContent, true);
+    private RevCommit qtUnStageExpectMergeFastForward(PushOneCommit.Result c,
+                                                      RevCommit expectedStagingHead)
+                                                      throws Exception {
+        return qtUnStage(c, expectedStagingHead, null, true, true);
+    }
+
+    private RevCommit qtUnStageExpectMergeOfMerge(PushOneCommit.Result c,
+                                                  PushOneCommit.Result expectedContent)
+                                                  throws Exception {
+        return qtUnStage(c, null, expectedContent, true, false);
     }
 
     private RevCommit qtUnStage(PushOneCommit.Result c,
                                 RevCommit expectedStagingHead,
                                 PushOneCommit.Result expectedContent,
-                                boolean merge)
+                                boolean merge,
+                                boolean fastForward)
                                 throws Exception {
         String branch = getBranchNameFromRef(c.getChange().change().getDest().get());
         String stagingRef = R_STAGING + branch;
@@ -332,7 +383,9 @@ public class QtUnStageIT extends QtCodeReviewIT {
         RevCommit stagingHead = getRemoteHead(project, stagingRef);
         assertThat(stagingHead).isNotEqualTo(oldStagingHead);
 
-        if (merge) {
+        if (fastForward) {
+            assertThat(stagingHead).isEqualTo(expectedStagingHead);
+        } else if (merge) {
             assertThat(stagingHead.getParentCount()).isEqualTo(2);
             assertThat(stagingHead.getParent(1)).isEqualTo(expectedContent.getCommit());
             expectedStagingHead = stagingHead;