diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java index f5202f4759..0a24269726 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java @@ -42,7 +42,6 @@ import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; -import com.google.common.collect.Table; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -63,7 +62,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Comparator; -import java.util.HashMap; import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -710,37 +708,10 @@ public class ChangeBundle { } } - @AutoValue - static abstract class ReviewerKey { - private static Map toMap(ReviewerSet reviewers) { - Map result = new HashMap<>(); - for (Table.Cell c : - reviewers.asTable().cellSet()) { - result.put(new AutoValue_ChangeBundle_ReviewerKey( - c.getRowKey(), c.getColumnKey()), c.getValue()); - } - return result; - } - - abstract ReviewerStateInternal state(); - abstract Account.Id account(); - - @Override - public String toString() { - return state() + "," + account(); - } - } - private static void diffReviewers(List diffs, ChangeBundle bundleA, ChangeBundle bundleB) { - Map as = ReviewerKey.toMap(bundleA.reviewers); - Map bs = ReviewerKey.toMap(bundleB.reviewers); - for (ReviewerKey k : diffKeySets(diffs, as, bs)) { - Timestamp a = as.get(k); - Timestamp b = bs.get(k); - String desc = describe(k); - diffTimestamps(diffs, desc, bundleA, a, bundleB, b, "timestamp"); - } + diffSets( + diffs, bundleA.reviewers.all(), bundleB.reviewers.all(), "reviewer"); } private static void diffPatchLineComments(List diffs, @@ -759,19 +730,26 @@ public class ChangeBundle { private static Set diffKeySets(List diffs, Map a, Map b) { - Set as = a.keySet(); - Set bs = b.keySet(); + if (a.isEmpty() && b.isEmpty()) { + return a.keySet(); + } + String clazz = + keyClass((!a.isEmpty() ? a.keySet() : b.keySet()).iterator().next()); + return diffSets(diffs, a.keySet(), b.keySet(), clazz); + } + + private static Set diffSets(List diffs, Set as, + Set bs, String desc) { if (as.isEmpty() && bs.isEmpty()) { return as; } - String clazz = keyClass((!as.isEmpty() ? as : bs).iterator().next()); Set aNotB = Sets.difference(as, bs); Set bNotA = Sets.difference(bs, as); if (aNotB.isEmpty() && bNotA.isEmpty()) { return as; } - diffs.add(clazz + " sets differ: " + aNotB + " only in A; " + diffs.add(desc + " sets differ: " + aNotB + " only in A; " + bNotA + " only in B"); return Sets.intersection(as, bs); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java index 978682b0e9..a6ab96d002 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java @@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.common.TimeUtil.roundToSecond; import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB; import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB; +import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; @@ -1055,7 +1056,7 @@ public class ChangeBundleTest { } @Test - public void diffReviewerKeySets() throws Exception { + public void diffReviewers() throws Exception { Change c = TestChanges.newChange(project, accountId); Timestamp now = TimeUtil.nowTs(); ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), now); @@ -1068,67 +1069,23 @@ public class ChangeBundleTest { assertNoDiffs(b1, b1); assertNoDiffs(b2, b2); assertDiffs(b1, b2, - "ReviewerKey sets differ:" - + " [REVIEWER,1] only in A;" - + " [REVIEWER,2] only in B"); + "reviewer sets differ:" + + " [1] only in A;" + + " [2] only in B"); } @Test - public void diffReviewerTimestamps() throws Exception { + public void diffReviewersIgnoresStateAndTimestamp() throws Exception { Change c = TestChanges.newChange(project, accountId); ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs()); - ReviewerSet r2 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs()); + ReviewerSet r2 = reviewers(CC, new Account.Id(1), TimeUtil.nowTs()); ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1, REVIEW_DB); ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r2, REVIEW_DB); - assertDiffs(b1, b2, - "timestamp differs for ReviewerKey REVIEWER,1:" - + " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:12.0}"); - - b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1, - REVIEW_DB); - b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r2, - NOTE_DB); - assertDiffs(b1, b2, - "timestamp differs for ReviewerKey REVIEWER,1 in NoteDb vs. ReviewDb:" - + " {2009-09-30 17:00:12.0} != {2009-09-30 17:00:06.0}"); - } - - @Test - public void diffReviewerTimestampsAllowsSlop() throws Exception { - subWindowResolution(); - Change c = TestChanges.newChange(project, accountId); - ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs()); - ReviewerSet r2 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs()); - - // Both are ReviewDb, exact timestamp match is required. - ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(), - comments(), r1, REVIEW_DB); - ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(), - comments(), r2, REVIEW_DB); - assertDiffs(b1, b2, - "timestamp differs for ReviewerKey REVIEWER,1:" - + " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}"); - - // One NoteDb, slop is allowed - b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1, - NOTE_DB); - b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r2, - REVIEW_DB); - assertNoDiffs(b1, b2); - - // But not too much slop. - superWindowResolution(); - ReviewerSet r3 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs()); - b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1, - NOTE_DB); - ChangeBundle b3 = new ChangeBundle(c, messages(), latest(c), approvals(), - comments(), r3, REVIEW_DB); - assertDiffs(b1, b3, - "timestamp differs for ReviewerKey REVIEWER,1 in NoteDb vs. ReviewDb:" - + " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}"); + assertNoDiffs(b1, b1); + assertNoDiffs(b2, b2); } @Test