Support copying of approvals if new patch set has no code changes
It can now be configured that all scores for a label are copied
forward when a new patch set is uploaded that has the same parent
commit as the previous patch set and the same code delta as the
previous patch set. This means only the commit message is different.
This can be used to enable sticky approvals on labels that only
depend on the code, reducing turn-around if only the commit message
is changed prior to submitting a change.
E.g. this configuration could be used for the Verified category if
it is only used by an automated tool to express that build and tests
succeed.
Change-Id: I105f85bd28d0bea39a4e16aaebf2630ef1860fbe
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 0e5c27a..c08d484 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -248,6 +248,18 @@
This can be used to enable sticky approvals, reducing turn-around for
trivial rebases prior to submitting a change. Defaults to false.
+[[label_copyAllScoresIfNoCodeChange]]
+`label.Label-Name.copyAllScoresIfNoCodeChange`
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If true, all scores for the label are copied forward when a new patch
+set is uploaded that has the same parent commit as the previous patch
+set and the same code delta as the previous patch set. This means only
+the commit message is different. This can be used to enable sticky
+approvals on labels that only depend on the code, reducing turn-around
+if only the commit message is changed prior to submitting a change.
+Defaults to false.
+
[[label_canOverride]]
`label.Label-Name.canOverride`
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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 83d3514..7fd8864 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
@@ -98,6 +98,7 @@
protected boolean copyMinScore;
protected boolean copyMaxScore;
protected boolean copyAllScoresOnTrivialRebase;
+ protected boolean copyAllScoresIfNoCodeChange;
protected List<LabelValue> values;
protected short maxNegative;
@@ -214,6 +215,14 @@
this.copyAllScoresOnTrivialRebase = copyAllScoresOnTrivialRebase;
}
+ public boolean isCopyAllScoresIfNoCodeChange() {
+ return copyAllScoresIfNoCodeChange;
+ }
+
+ public void setCopyAllScoresIfNoCodeChange(boolean copyAllScoresIfNoCodeChange) {
+ this.copyAllScoresIfNoCodeChange = copyAllScoresIfNoCodeChange;
+ }
+
public boolean isMaxNegative(PatchSetApproval ca) {
return maxNegative == ca.getValue();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index 7ec26b0..da2f98f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -26,6 +26,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -69,10 +70,10 @@
* @throws OrmException
*/
public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
- PatchSet.Id source, PatchSet dest, boolean trivialRebase) throws OrmException {
+ PatchSet.Id source, PatchSet dest, ChangeKind changeKind) throws OrmException {
Iterable<PatchSetApproval> sourceApprovals =
db.patchSetApprovals().byPatchSet(source);
- copyLabels(db, labelTypes, sourceApprovals, source, dest, trivialRebase);
+ copyLabels(db, labelTypes, sourceApprovals, source, dest, changeKind);
}
/**
@@ -82,7 +83,7 @@
*/
public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
Iterable<PatchSetApproval> sourceApprovals, PatchSet.Id source,
- PatchSet dest, boolean trivialRebase) throws OrmException {
+ PatchSet dest, ChangeKind changeKind) throws OrmException {
List<PatchSetApproval> copied = Lists.newArrayList();
for (PatchSetApproval a : sourceApprovals) {
if (source.equals(a.getPatchSetId())) {
@@ -93,7 +94,11 @@
copied.add(new PatchSetApproval(dest.getId(), a));
} else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
copied.add(new PatchSetApproval(dest.getId(), a));
- } else if (type.isCopyAllScoresOnTrivialRebase() && trivialRebase) {
+ } else if (type.isCopyAllScoresOnTrivialRebase()
+ && ChangeKind.TRIVIAL_REBASE.equals(changeKind)) {
+ copied.add(new PatchSetApproval(dest.getId(), a));
+ } else if (type.isCopyAllScoresIfNoCodeChange()
+ && ChangeKind.NO_CODE_CHANGE.equals(changeKind)) {
copied.add(new PatchSetApproval(dest.getId(), a));
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 2df016f..3d14bab 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -25,7 +25,6 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
@@ -53,7 +52,6 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.FooterLine;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -86,6 +84,10 @@
GERRIT, RECEIVE_COMMITS, NONE;
}
+ public static enum ChangeKind {
+ REWORK, TRIVIAL_REBASE, NO_CODE_CHANGE;
+ }
+
private final ChangeHooks hooks;
private final TrackingFooters trackingFooters;
private final PatchSetInfoFactory patchSetInfoFactory;
@@ -278,11 +280,11 @@
RevCommit priorCommit = revWalk.parseCommit(priorCommitId);
ProjectState projectState =
refControl.getProjectControl().getProjectState();
- boolean trivialRebase =
- isTrivialRebase(mergeUtilFactory, projectState, git, priorCommit, commit);
+ ChangeKind changeKind =
+ getChangeKind(mergeUtilFactory, projectState, git, priorCommit, commit);
ApprovalsUtil.copyLabels(db, refControl.getProjectControl()
- .getLabelTypes(), currentPatchSetId, patchSet, trivialRebase);
+ .getLabelTypes(), currentPatchSetId, patchSet, changeKind);
}
final List<FooterLine> footerLines = commit.getFooterLines();
@@ -362,20 +364,25 @@
}
}
- public static boolean isTrivialRebase(MergeUtil.Factory mergeUtilFactory, ProjectState project,
+ public static ChangeKind getChangeKind(MergeUtil.Factory mergeUtilFactory, ProjectState project,
Repository git, RevCommit prior, RevCommit next) {
if (!next.getFullMessage().equals(prior.getFullMessage())) {
- return false;
+ if (next.getTree() == prior.getTree()
+ && prior.getParent(0).equals(next.getParent(0))) {
+ return ChangeKind.NO_CODE_CHANGE;
+ } else {
+ return ChangeKind.REWORK;
+ }
}
if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
// Trivial rebases done by machine only work well on 1 parent.
- return false;
+ return ChangeKind.REWORK;
}
if (next.getTree() == prior.getTree() &&
prior.getParent(0).equals(next.getParent(0))) {
- return true;
+ return ChangeKind.TRIVIAL_REBASE;
}
// A trivial rebase can be detected by looking for the next commit
@@ -386,12 +393,16 @@
ThreeWayMerger merger =
mergeUtil.newThreeWayMerger(git, mergeUtil.createDryRunInserter());
merger.setBase(prior.getParent(0));
- return merger.merge(next.getParent(0), prior)
- && merger.getResultTreeId().equals(next.getTree());
+ if (merger.merge(next.getParent(0), prior)
+ && merger.getResultTreeId().equals(next.getTree())) {
+ return ChangeKind.TRIVIAL_REBASE;
+ } else {
+ return ChangeKind.REWORK;
+ }
} catch (IOException err) {
log.warn("Cannot check trivial rebase of new patch set " + next.name()
+ " in " + project.getProject().getName(), err);
- return false;
+ return ChangeKind.REWORK;
}
}
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 43cc17a..0c960eb 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
@@ -127,6 +127,7 @@
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_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase";
+ private static final String KEY_COPY_ALL_SCORES_IF_NO_CHANGE = "copyAllScoresIfNoCodeChange";
private static final String KEY_VALUE = "value";
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final String KEY_Branch = "branch";
@@ -654,6 +655,8 @@
rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, false));
label.setCopyAllScoresOnTrivialRebase(
rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, false));
+ label.setCopyAllScoresIfNoCodeChange(
+ rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, false));
label.setCanOverride(
rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true));
label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch));
@@ -986,6 +989,11 @@
} else {
rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE);
}
+ if (label.isCopyAllScoresIfNoCodeChange()) {
+ rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE, true);
+ } else {
+ rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_IF_NO_CHANGE);
+ }
if (!label.canOverride()) {
rc.setBoolean(LABEL, name, KEY_CAN_OVERRIDE, false);
} else {
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 8485db7..a98dbee 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
@@ -66,6 +66,7 @@
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.PatchSetInserter;
+import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.config.AllProjectsName;
@@ -1601,7 +1602,7 @@
ChangeMessage msg;
String mergedIntoRef;
boolean skip;
- boolean trivialRebase;
+ ChangeKind changeKind;
private PatchSet.Id priorPatchSet;
ReplaceRequest(final Change.Id toChange, final RevCommit newCommit,
@@ -1610,7 +1611,7 @@
this.newCommit = newCommit;
this.inputCommand = cmd;
this.checkMergedInto = checkMergedInto;
- this.trivialRebase = false;
+ this.changeKind = ChangeKind.REWORK;
revisions = HashBiMap.create();
for (Ref ref : refs(toChange)) {
@@ -1711,8 +1712,8 @@
}
}
- trivialRebase =
- PatchSetInserter.isTrivialRebase(mergeUtilFactory,
+ changeKind =
+ PatchSetInserter.getChangeKind(mergeUtilFactory,
projectControl.getProjectState(), repo, priorCommit, newCommit);
PatchSet.Id id =
@@ -1793,7 +1794,7 @@
final MailRecipients oldRecipients = getRecipientsFromApprovals(
oldChangeApprovals);
ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals,
- priorPatchSet, newPatchSet, trivialRebase);
+ priorPatchSet, newPatchSet, changeKind);
approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info,
recipients.getReviewers(), oldRecipients.getAll());
recipients.add(oldRecipients);
