diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java index f958df9bf4..54fe28f09a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java @@ -247,6 +247,70 @@ public class StickyApprovalsIT extends AbstractDaemonTest { } } + @Test + public void stickyAcrossMultiplePatchSets() throws Exception { + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + cfg.getLabelSections().get("Code-Review") + .setCopyMaxScore(true); + cfg.getLabelSections().get("Verified") + .setCopyAllScoresIfNoCodeChange(true); + saveProjectConfig(project, cfg); + + String changeId = createChange(REWORK); + vote(admin, changeId, 2, 1); + + for (int i = 0; i < 5; i++) { + updateChange(changeId, NO_CODE_CHANGE); + ChangeInfo c = detailedChange(changeId); + assertVotes(c, admin, 2, 1, NO_CODE_CHANGE); + } + + updateChange(changeId, REWORK); + ChangeInfo c = detailedChange(changeId); + assertVotes(c, admin, 2, 0, REWORK); + } + + @Test + public void copyMinMaxAcrossMultiplePatchSets() throws Exception { + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + cfg.getLabelSections().get("Code-Review") + .setCopyMaxScore(true); + cfg.getLabelSections().get("Code-Review") + .setCopyMinScore(true); + saveProjectConfig(project, cfg); + + // Vote max score on PS1 + String changeId = createChange(REWORK); + vote(admin, changeId, 2, 1); + + // Have someone else vote min score on PS2 + updateChange(changeId, REWORK); + vote(user, changeId, -2, 0); + ChangeInfo c = detailedChange(changeId); + assertVotes(c, admin, 2, 0, REWORK); + assertVotes(c, user, -2, 0, REWORK); + + // No vote changes on PS3 + updateChange(changeId, REWORK); + c = detailedChange(changeId); + assertVotes(c, admin, 2, 0, REWORK); + assertVotes(c, user, -2, 0, REWORK); + + // Both users revote on PS4 + updateChange(changeId, REWORK); + vote(admin, changeId, 1, 1); + vote(user, changeId, 1, 1); + c = detailedChange(changeId); + assertVotes(c, admin, 1, 1, REWORK); + assertVotes(c, user, 1, 1, REWORK); + + // New approvals shouldn't carry through to PS5 + updateChange(changeId, REWORK); + c = detailedChange(changeId); + assertVotes(c, admin, 0, 0, REWORK); + assertVotes(c, user, 0, 0, REWORK); + } + private ChangeInfo detailedChange(String changeId) throws Exception { return gApi.changes().id(changeId) .get(EnumSet.of(ListChangesOption.DETAILED_LABELS, 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 a3a77346d1..bc2ec0659c 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 @@ -44,9 +44,6 @@ import java.io.IOException; import java.util.Collection; import java.util.Collections; import java.util.List; -import java.util.NavigableSet; -import java.util.Objects; -import java.util.SortedSet; import java.util.TreeMap; /** @@ -104,6 +101,8 @@ public class ApprovalCopier { ListMultimap all = cd.approvals(); checkNotNull(all, "all should not be null"); + Table wontCopy = + HashBasedTable.create(); Table byUser = HashBasedTable.create(); for (PatchSetApproval psa : all.get(ps.getId())) { @@ -111,7 +110,6 @@ public class ApprovalCopier { } TreeMap patchSets = getPatchSets(cd); - NavigableSet allPsIds = patchSets.navigableKeySet(); try (Repository repo = repoManager.openRepository(project.getProject().getNameKey())) { @@ -130,11 +128,18 @@ public class ApprovalCopier { ObjectId.fromString(ps.getRevision().get())); for (PatchSetApproval psa : priorApprovals) { - if (!byUser.contains(psa.getLabel(), psa.getAccountId()) - && canCopy(project, psa, ps.getId(), allPsIds, kind)) { - byUser.put(psa.getLabel(), psa.getAccountId(), - copy(psa, ps.getId())); + if (wontCopy.contains(psa.getLabel(), psa.getAccountId())) { + continue; } + if (byUser.contains(psa.getLabel(), psa.getAccountId())) { + continue; + } + if (!canCopy(project, psa, ps.getId(), kind)) { + wontCopy.put(psa.getLabel(), psa.getAccountId(), psa); + continue; + } + byUser.put(psa.getLabel(), psa.getAccountId(), + copy(psa, ps.getId())); } } return labelNormalizer.normalize(ctl, byUser.values()).getNormalized(); @@ -155,17 +160,15 @@ public class ApprovalCopier { } private static boolean canCopy(ProjectState project, PatchSetApproval psa, - PatchSet.Id psId, NavigableSet allPsIds, ChangeKind kind) { + PatchSet.Id psId, ChangeKind kind) { int n = psa.getKey().getParentKey().get(); checkArgument(n != psId.get()); LabelType type = project.getLabelTypes().byLabel(psa.getLabelId()); if (type == null) { return false; - } else if (Objects.equals(n, previous(allPsIds, psId.get())) && ( - type.isCopyMinScore() && type.isMaxNegative(psa) - || type.isCopyMaxScore() && type.isMaxPositive(psa))) { - // Copy min/max score only from the immediately preceding patch set (which - // may not be psId.get() - 1). + } else if ( + (type.isCopyMinScore() && type.isMaxNegative(psa)) + || (type.isCopyMaxScore() && type.isMaxPositive(psa))) { return true; } switch (kind) { @@ -192,9 +195,4 @@ public class ApprovalCopier { } return new PatchSetApproval(psId, src); } - - private static T previous(NavigableSet s, T v) { - SortedSet head = s.headSet(v); - return !head.isEmpty() ? head.last() : null; - } -} +} \ No newline at end of file