Implement sticky approvals with copyMaxScore
Labels that have copyMaxScore set true are automatically copied to the
new patch set. This permits projects to enable Code-Review+2 to always
move forward, avoiding the need to re-review trivial fixups.
Change-Id: Ib5f1acb69937cb61897d79cb9a966744fa57b9c1
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 40c380c..3d0f827 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -219,6 +219,14 @@
If true, the lowest possible negative value for the label is copied
forward when a new patch set is uploaded.
+[[label_copyMaxScore]]
+`label.Label-Name.copyMaxScore`
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If true, the highest possible positive value for the label is copied
+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.
[[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 2049887..8a9a508 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
@@ -96,6 +96,7 @@
protected String abbreviatedName;
protected String functionName;
protected boolean copyMinScore;
+ protected boolean copyMaxScore;
protected List<LabelValue> values;
protected short maxNegative;
@@ -187,6 +188,14 @@
this.copyMinScore = copyMinScore;
}
+ public boolean isCopyMaxScore() {
+ return copyMaxScore;
+ }
+
+ public void setCopyMaxScore(boolean copyMaxScore) {
+ this.copyMaxScore = copyMaxScore;
+ }
+
public boolean isMaxNegative(PatchSetApproval ca) {
return maxNegative == ca.getValue();
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java
index 5b064c8..ba50417 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java
@@ -22,15 +22,14 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.mail.CommitMessageEditedSender;
import com.google.gerrit.server.git.validators.CommitValidators;
+import com.google.gerrit.server.mail.CommitMessageEditedSender;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
@@ -76,7 +75,6 @@
private final PatchSetInfoFactory patchSetInfoFactory;
private final PersonIdent myIdent;
- private final ApprovalsUtil approvalsUtil;
private final TrackingFooters trackingFooters;
@Inject
@@ -91,7 +89,7 @@
final PatchSetInfoFactory patchSetInfoFactory,
final GitReferenceUpdated gitRefUpdated,
@GerritPersonIdent final PersonIdent myIdent,
- final ApprovalsUtil approvalsUtil, TrackingFooters trackingFooters) {
+ TrackingFooters trackingFooters) {
this.changeControlFactory = changeControlFactory;
this.db = db;
this.currentUser = currentUser;
@@ -107,7 +105,6 @@
this.patchSetInfoFactory = patchSetInfoFactory;
this.gitRefUpdated = gitRefUpdated;
this.myIdent = myIdent;
- this.approvalsUtil = approvalsUtil;
this.trackingFooters = trackingFooters;
}
@@ -136,7 +133,7 @@
ChangeUtil.editCommitMessage(patchSetId, control.getRefControl(), commitValidators, currentUser, message, db,
commitMessageEditedSenderFactory, hooks, git, patchSetInfoFactory, gitRefUpdated, myIdent,
- approvalsUtil, trackingFooters);
+ trackingFooters);
return changeDetailFactory.create(changeId).call();
} finally {
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 0ce6892..76c33ab 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
@@ -20,7 +20,6 @@
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Account.Id;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
@@ -30,7 +29,6 @@
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
-import java.util.Collections;
import java.util.List;
import java.util.Set;
@@ -69,36 +67,30 @@
* Moves the PatchSetApprovals to the specified PatchSet on the change from
* the prior PatchSet, while keeping the vetos.
*
- * @param db database connection to use for updates.
- * @param dest PatchSet to copy to
* @throws OrmException
* @return List<PatchSetApproval> The previous approvals
*/
- public List<PatchSetApproval> copyVetosToPatchSet(ReviewDb db,
- LabelTypes labelTypes, PatchSet.Id dest) throws OrmException {
- PatchSet.Id source;
- if (dest.get() > 1) {
- source = new PatchSet.Id(dest.getParentKey(), dest.get() - 1);
- } else {
- throw new OrmException("Previous patch set could not be found");
- }
-
- List<PatchSetApproval> patchSetApprovals =
- db.patchSetApprovals().byChange(dest.getParentKey()).toList();
- for (PatchSetApproval a : patchSetApprovals) {
+ public static List<PatchSetApproval> copyLabels(ReviewDb db,
+ LabelTypes labelTypes,
+ PatchSet.Id source,
+ PatchSet.Id dest) throws OrmException {
+ List<PatchSetApproval> copied = Lists.newArrayList();
+ for (PatchSetApproval a : db.patchSetApprovals().byPatchSet(source)) {
LabelType type = labelTypes.byLabel(a.getLabelId());
- if (type != null && a.getPatchSetId().equals(source) &&
- type.isCopyMinScore() &&
- type.isMaxNegative(a)) {
- db.patchSetApprovals().insert(
- Collections.singleton(new PatchSetApproval(dest, a)));
+ if (type == null) {
+ continue;
+ } else if (type.isCopyMinScore() && type.isMaxNegative(a)) {
+ copied.add(new PatchSetApproval(dest, a));
+ } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
+ copied.add(new PatchSetApproval(dest, a));
}
}
- return patchSetApprovals;
+ db.patchSetApprovals().insert(copied);
+ return copied;
}
public void addReviewers(ReviewDb db, LabelTypes labelTypes, Change change,
- PatchSet ps, PatchSetInfo info, Set<Id> wantReviewers,
+ PatchSet ps, PatchSetInfo info, Set<Account.Id> wantReviewers,
Set<Account.Id> existingReviewers) throws OrmException {
List<LabelType> allTypes = labelTypes.getLabelTypes();
if (allTypes.isEmpty()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
index 41a97d2..a1dbd54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@@ -330,7 +330,7 @@
final ChangeHooks hooks, Repository git,
final PatchSetInfoFactory patchSetInfoFactory,
final GitReferenceUpdated gitRefUpdated, PersonIdent myIdent,
- final ApprovalsUtil approvalsUtil, final TrackingFooters trackingFooters)
+ final TrackingFooters trackingFooters)
throws NoSuchChangeException, EmailException, OrmException,
MissingObjectException, IncorrectObjectTypeException, IOException,
InvalidChangeOperationException, PatchSetInfoNotAvailableException {
@@ -445,8 +445,9 @@
"Change %s was modified", change.getId()));
}
- approvalsUtil.copyVetosToPatchSet(db,
+ ApprovalsUtil.copyLabels(db,
refControl.getProjectControl().getLabelTypes(),
+ originalPS.getId(),
change.currentPatchSetId());
final List<FooterLine> footerLines = newCommit.getFooterLines();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
index 6ae8c96..e5f6b8f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java
@@ -16,7 +16,6 @@
import com.google.common.collect.Sets;
import com.google.gerrit.common.ChangeHookRunner;
-import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
@@ -72,7 +71,6 @@
private final GitReferenceUpdated gitRefUpdated;
private final RebasedPatchSetSender.Factory rebasedPatchSetSenderFactory;
private final ChangeHookRunner hooks;
- private final ApprovalsUtil approvalsUtil;
private final MergeUtil.Factory mergeUtilFactory;
@Inject
@@ -82,7 +80,7 @@
final GitRepositoryManager gitManager,
final GitReferenceUpdated gitRefUpdated,
final RebasedPatchSetSender.Factory rebasedPatchSetSenderFactory,
- final ChangeHookRunner hooks, final ApprovalsUtil approvalsUtil,
+ final ChangeHookRunner hooks,
final MergeUtil.Factory mergeUtilFactory) {
this.changeControlFactory = changeControlFactory;
this.patchSetInfoFactory = patchSetInfoFactory;
@@ -92,7 +90,6 @@
this.gitRefUpdated = gitRefUpdated;
this.rebasedPatchSetSenderFactory = rebasedPatchSetSenderFactory;
this.hooks = hooks;
- this.approvalsUtil = approvalsUtil;
this.mergeUtilFactory = mergeUtilFactory;
}
@@ -377,9 +374,9 @@
"Change %s was modified", change.getId()));
}
- final LabelTypes labelTypes = changeControlFactory.controlFor(change)
- .getLabelTypes();
- approvalsUtil.copyVetosToPatchSet(db, labelTypes,
+ ApprovalsUtil.copyLabels(db,
+ changeControlFactory.controlFor(change).getLabelTypes(),
+ patchSetId,
change.currentPatchSetId());
final ChangeMessage cmsg =
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 2a260e4d..852b5e7 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
@@ -112,6 +112,7 @@
private static final String KEY_ABBREVIATION = "abbreviation";
private static final String KEY_FUNCTION = "function";
private static final String KEY_COPY_MIN_SCORE = "copyMinScore";
+ private static final String KEY_COPY_MAX_SCORE = "copyMaxScore";
private static final String KEY_VALUE = "value";
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final Set<String> LABEL_FUNCTIONS = ImmutableSet.of(
@@ -581,6 +582,8 @@
}
label.setCopyMinScore(
rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false));
+ label.setCopyMaxScore(
+ rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, false));
label.setCanOverride(
rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true));
labelSections.put(name, label);
@@ -848,6 +851,11 @@
} else {
rc.unset(LABEL, name, KEY_COPY_MIN_SCORE);
}
+ if (label.isCopyMaxScore()) {
+ rc.setBoolean(LABEL, name, KEY_COPY_MAX_SCORE, true);
+ } else {
+ rc.unset(LABEL, name, KEY_COPY_MAX_SCORE);
+ }
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 308f5db..9dd5a4f 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
@@ -18,7 +18,6 @@
import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
-
import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.OK;
@@ -50,7 +49,6 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
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;
@@ -1767,10 +1765,9 @@
mergedIntoRef = mergedInto != null ? mergedInto.getName() : null;
}
- List<PatchSetApproval> patchSetApprovals =
- approvalsUtil.copyVetosToPatchSet(db, labelTypes, newPatchSet.getId());
final MailRecipients oldRecipients =
- getRecipientsFromApprovals(patchSetApprovals);
+ getRecipientsFromApprovals(ApprovalsUtil.copyLabels(
+ db, labelTypes, priorPatchSet, newPatchSet.getId()));
approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info,
recipients.getReviewers(), oldRecipients.getAll());
recipients.add(oldRecipients);
