diff options
author | Jukka Jokiniva <jukka.jokiniva@qt.io> | 2019-07-04 12:12:38 +0300 |
---|---|---|
committer | Jukka Jokiniva <jukka.jokiniva@qt.io> | 2019-09-13 06:14:01 +0000 |
commit | c8c22916065438a4468258b22143648396a5f460 (patch) | |
tree | d80e0784fba8c4f492966c4925c784adfa2a33f3 | |
parent | 37bc077ec92549d314227a981dafdcf3ebe58b96 (diff) |
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; |