ChangeBundle: Be more lenient about reviewers
The computation in ReviewerSet#fromApprovals doesn't really do the
right thing when there are multiple zero and non-zero labels and the
input list isn't sorted. This is a different thing from what
ChangeNotesParser does, which always takes the latest state for a user
regardless of whether there are other states earlier in the series.
We don't distinguish REVIEWER/CC in the UI, so being slightly lossy is
not likely to be noticeable.
Change-Id: I8b8409742a01f53f4d10a363e36bd4d5bb724ed2
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 f5202f4..0a24269 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.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.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 @@
}
}
- @AutoValue
- static abstract class ReviewerKey {
- private static Map<ReviewerKey, Timestamp> toMap(ReviewerSet reviewers) {
- Map<ReviewerKey, Timestamp> result = new HashMap<>();
- for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> 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<String> diffs,
ChangeBundle bundleA, ChangeBundle bundleB) {
- Map<ReviewerKey, Timestamp> as = ReviewerKey.toMap(bundleA.reviewers);
- Map<ReviewerKey, Timestamp> 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<String> diffs,
@@ -759,19 +730,26 @@
private static <T> Set<T> diffKeySets(List<String> diffs, Map<T, ?> a,
Map<T, ?> b) {
- Set<T> as = a.keySet();
- Set<T> 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 <T> Set<T> diffSets(List<String> diffs, Set<T> as,
+ Set<T> bs, String desc) {
if (as.isEmpty() && bs.isEmpty()) {
return as;
}
- String clazz = keyClass((!as.isEmpty() ? as : bs).iterator().next());
Set<T> aNotB = Sets.difference(as, bs);
Set<T> 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 978682b..a6ab96d 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.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 @@
}
@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 @@
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
