From cedbbee78ef887aed6391825da1e5959859b7c10 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Fri, 29 Jan 2016 12:06:20 +0100 Subject: [PATCH] Add new copy votes policy for merge commits Adds new 'copyAllScoresOnMergeFirstParentUpdate' vote policy. This policy allows to carry over votes on "merge commit rebase". This meas when only first (left parent) changes and rest of parent(s) stays the same. Such policy is useful for changes that are merge commit of feature branches that tracks destination branch. When it is enabled it reduces the overhead of CI and human reviews on merge commits that only incorporate newer changes from destination branch in next patch set. Change-Id: I02062544608546498d9605c628bc1b7a0386e91a Signed-off-by: Dariusz Luksza --- Documentation/config-labels.txt | 16 ++++++++++ .../google/gerrit/common/data/LabelType.java | 14 +++++++++ .../google/gerrit/server/ApprovalCopier.java | 4 ++- .../gerrit/server/change/ChangeKind.java | 3 ++ .../server/change/ChangeKindCacheImpl.java | 30 +++++++++++++++++-- .../gerrit/server/git/ProjectConfig.java | 4 +++ .../gerrit/server/git/ReceiveCommits.java | 1 + 7 files changed, 69 insertions(+), 3 deletions(-) diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index 16c66bc2af..d36e77f5c6 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -246,6 +246,22 @@ forward when a new patch set is uploaded. This can be used to enable sticky approvals, reducing turn-around for trivial cleanups prior to submitting a change. Defaults to false. +[[lable_copyAllScoresOnMergCommitFirstParentUpdate]] +=== `label.Label-Name.copyAllScoresOnMergeCommitFirstParentUpdate` + +This policy is useful if you don't want to trigger CI or human +verification again if your target branch moved on but the feature +branch being merged into the target branch did not change. It only +applies if the patch set is a merge commit. + +If true, all scores for the label are copied forward when a new +patch set is uploaded that is a new merge commit which only +differs from the previous patch set in its first parent. The +first parent would be the parent of the merge commit that is part +of the change's target branch, whereas the other parent(s) refer to +the feature branch(es) to be merged. +Defaults to false. + [[label_copyAllScoresOnTrivialRebase]] === `label.Label-Name.copyAllScoresOnTrivialRebase` diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java index cf6f7564fb..b1e1243e43 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java @@ -29,6 +29,7 @@ public class LabelType { public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CHANGE = true; public static final boolean DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = false; public static final boolean DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = false; + public static final boolean DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE = false; public static final boolean DEF_COPY_MAX_SCORE = false; public static final boolean DEF_COPY_MIN_SCORE = false; @@ -99,6 +100,7 @@ public class LabelType { protected String functionName; protected boolean copyMinScore; protected boolean copyMaxScore; + protected boolean copyAllScoresOnMergeFirstParentUpdate; protected boolean copyAllScoresOnTrivialRebase; protected boolean copyAllScoresIfNoCodeChange; protected boolean copyAllScoresIfNoChange; @@ -138,6 +140,8 @@ public class LabelType { setCopyAllScoresIfNoChange(DEF_COPY_ALL_SCORES_IF_NO_CHANGE); setCopyAllScoresIfNoCodeChange(DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE); setCopyAllScoresOnTrivialRebase(DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE); + setCopyAllScoresOnMergeFirstParentUpdate( + DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE); setCopyMaxScore(DEF_COPY_MAX_SCORE); setCopyMinScore(DEF_COPY_MIN_SCORE); } @@ -217,6 +221,16 @@ public class LabelType { this.copyMaxScore = copyMaxScore; } + public boolean isCopyAllScoresOnMergeFirstParentUpdate() { + return copyAllScoresOnMergeFirstParentUpdate; + } + + public void setCopyAllScoresOnMergeFirstParentUpdate( + boolean copyAllScoresOnMergeFirstParentUpdate) { + this.copyAllScoresOnMergeFirstParentUpdate = + copyAllScoresOnMergeFirstParentUpdate; + } + public boolean isCopyAllScoresOnTrivialRebase() { return copyAllScoresOnTrivialRebase; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java index c11563a0ad..72823d3576 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java @@ -16,6 +16,7 @@ package com.google.gerrit.server; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.gerrit.server.change.ChangeKind.MERGE_FIRST_PARENT_UPDATE; import static com.google.gerrit.server.change.ChangeKind.NO_CHANGE; import static com.google.gerrit.server.change.ChangeKind.NO_CODE_CHANGE; import static com.google.gerrit.server.change.ChangeKind.TRIVIAL_REBASE; @@ -172,7 +173,8 @@ public class ApprovalCopier { // may not be psId.get() - 1). return true; } - return (type.isCopyAllScoresOnTrivialRebase() && kind == TRIVIAL_REBASE) + return (type.isCopyAllScoresOnMergeFirstParentUpdate() && kind == MERGE_FIRST_PARENT_UPDATE) + || (type.isCopyAllScoresOnTrivialRebase() && kind == TRIVIAL_REBASE) || (type.isCopyAllScoresIfNoCodeChange() && kind == NO_CODE_CHANGE) || (type.isCopyAllScoresIfNoChange() && kind == NO_CHANGE); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java index 6e6f6fa1fe..63f6095ff2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKind.java @@ -22,6 +22,9 @@ public enum ChangeKind { /** Conflict-free merge between the new parent and the prior patch set. */ TRIVIAL_REBASE, + /** Conflict-free change of first (left) parent of a merge commit. */ + MERGE_FIRST_PARENT_UPDATE, + /** Same tree and same parent tree. */ NO_CODE_CHANGE, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index f3d14555f3..6b009a6e77 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java @@ -21,6 +21,7 @@ import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull; import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; import com.google.common.cache.Weigher; +import com.google.common.collect.FluentIterable; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -50,8 +51,10 @@ import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; +import java.util.Arrays; import java.util.Collection; import java.util.Objects; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -209,7 +212,8 @@ public class ChangeKindCacheImpl implements ChangeKindCache { return ChangeKind.NO_CHANGE; } - if (prior.getParentCount() != 1 || next.getParentCount() != 1) { + if ((prior.getParentCount() != 1 || next.getParentCount() != 1) + && !onlyFirstParentChanged(prior, next)) { // Trivial rebases done by machine only work well on 1 parent. return ChangeKind.REWORK; } @@ -223,7 +227,11 @@ public class ChangeKindCacheImpl implements ChangeKindCache { try { if (merger.merge(next.getParent(0), prior) && merger.getResultTreeId().equals(next.getTree())) { - return ChangeKind.TRIVIAL_REBASE; + if (prior.getParentCount() == 1) { + return ChangeKind.TRIVIAL_REBASE; + } else { + return ChangeKind.MERGE_FIRST_PARENT_UPDATE; + } } } catch (LargeObjectException e) { // Some object is too large for the merge attempt to succeed. Assume @@ -233,6 +241,24 @@ public class ChangeKindCacheImpl implements ChangeKindCache { } } + public static boolean onlyFirstParentChanged(RevCommit prior, RevCommit next) { + return !sameFirstParents(prior, next) && sameRestOfParents(prior, next); + } + + private static boolean sameFirstParents(RevCommit prior, RevCommit next) { + return prior.getParent(0).equals(next.getParent(0)); + } + + private static boolean sameRestOfParents(RevCommit prior, RevCommit next) { + Set priorRestParents = allExceptFirstParent(prior.getParents()); + Set nextRestParents = allExceptFirstParent(next.getParents()); + return priorRestParents.equals(nextRestParents); + } + + private static Set allExceptFirstParent(RevCommit[] parents) { + return FluentIterable.from(Arrays.asList(parents)).skip(1).toSet(); + } + private static boolean isSameDeltaAndTree(RevCommit prior, RevCommit next) { if (next.getTree() != prior.getTree()) { return false; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index 246647c3d9..d463c6a3dc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -134,6 +134,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private static final String KEY_DEFAULT_VALUE = "defaultValue"; private static final String KEY_COPY_MIN_SCORE = "copyMinScore"; private static final String KEY_COPY_MAX_SCORE = "copyMaxScore"; + private static final String KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE = "copyAllScoresOnMergeFirstParentUpdate"; private static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase"; private static final String KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE = "copyAllScoresIfNoCodeChange"; private static final String KEY_COPY_ALL_SCORES_IF_NO_CHANGE = "copyAllScoresIfNoChange"; @@ -716,6 +717,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. label.setCopyMaxScore( rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, LabelType.DEF_COPY_MAX_SCORE)); + label.setCopyAllScoresOnMergeFirstParentUpdate( + rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE, + LabelType.DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE)); label.setCopyAllScoresOnTrivialRebase( rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, LabelType.DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index f58313852e..d6b037cf7b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -2198,6 +2198,7 @@ public class ReceiveCommits { private String changeKindMessage(ChangeKind changeKind) { switch (changeKind) { + case MERGE_FIRST_PARENT_UPDATE: case TRIVIAL_REBASE: case NO_CHANGE: return ": Patch Set " + priorPatchSet.get() + " was rebased";