Remove ReviewDbUtil#intKeyOrdering()

None of the callers are subject to the null safety issue described in
the Javadoc. Although it's true that some callers require more code to
use the equivalent stream API, it's still plenty readable. On balance,
it doesn't justify having this gwtorm-specific helper.

Change-Id: Ic5b9dc07566ec9bf20da9a0ac10b9cf95d78386b

java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java

@@ -16,10 +16,8 @@ package com.google.gerrit.reviewdb.server;
 
 import static com.google.common.base.Preconditions.checkState;
 
-import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.gwtorm.client.Column;
-import com.google.gwtorm.client.IntKey;
 import java.lang.reflect.Field;
 import java.util.Arrays;
 import java.util.Set;
@@ -27,28 +25,6 @@ import java.util.TreeSet;
 
 /** Static utilities for ReviewDb types. */
 public class ReviewDbUtil {
-  private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING =
-      Ordering.natural().nullsFirst().<IntKey<?>>onResultOf(IntKey::get).nullsFirst();
-
-  /**
-   * Null-safe ordering over arbitrary subclass of {@code IntKey}.
-   *
-   * <p>In some cases, {@code Comparator.comparing(Change.Id::get)} may be shorter and cleaner.
-   * However, this method may be preferable in some cases:
-   *
-   * <ul>
-   *   <li>This ordering is null-safe over both input and the result of {@link IntKey#get()}; {@code
-   *       comparing} is only a good idea if all inputs are obviously non-null.
-   *   <li>{@code intKeyOrdering().sortedCopy(iterable)} is shorter than the stream equivalent.
-   *   <li>Creating derived comparators may be more readable with {@link Ordering} method chaining
-   *       rather than static {@code Comparator} methods.
-   * </ul>
-   */
-  @SuppressWarnings("unchecked")
-  public static <K extends IntKey<?>> Ordering<K> intKeyOrdering() {
-    return (Ordering<K>) INT_KEY_ORDERING;
-  }
-
   public static ReviewDb unwrapDb(ReviewDb db) {
     if (db instanceof DisallowedReviewDb) {
       return unwrapDb(((DisallowedReviewDb) db).unsafeGetDelegate());

java/com/google/gerrit/server/change/ConsistencyChecker.java

@@ -15,9 +15,10 @@
 package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
-import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
 import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER;
+import static java.util.Comparator.comparing;
 import static java.util.Objects.requireNonNull;
 
 import com.google.auto.value.AutoValue;
@@ -37,7 +38,6 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
@@ -67,6 +67,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeSet;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
@@ -457,7 +458,11 @@ public class ConsistencyChecker {
           problem(
               String.format(
                   "Multiple patch sets for expected merged commit %s: %s",
-                  commit.name(), intKeyOrdering().sortedCopy(thisCommitPsIds)));
+                  commit.name(),
+                  thisCommitPsIds
+                      .stream()
+                      .sorted(comparing(PatchSet.Id::get))
+                      .collect(toImmutableList())));
           break;
       }
     } catch (IOException e) {
@@ -695,7 +700,7 @@ public class ConsistencyChecker {
       if (!toDelete.contains(ctx.getChange().currentPatchSetId())) {
         return false;
       }
-      Set<PatchSet.Id> all = new HashSet<>();
+      TreeSet<PatchSet.Id> all = new TreeSet<>(comparing(PatchSet.Id::get));
       // Doesn't make any assumptions about the order in which deletes happen
       // and whether they are seen by this op; we are already given the full set
       // of patch sets that will eventually be deleted in this update.
@@ -707,8 +712,7 @@ public class ConsistencyChecker {
       if (all.isEmpty()) {
         throw new NoPatchSetsWouldRemainException();
       }
-      PatchSet.Id latest = ReviewDbUtil.intKeyOrdering().max(all);
-      ctx.getChange().setCurrentPatchSet(patchSetInfoFactory.get(ctx.getNotes(), latest));
+      ctx.getChange().setCurrentPatchSet(patchSetInfoFactory.get(ctx.getNotes(), all.last()));
       return true;
     }
   }

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -37,6 +37,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange;
 import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
+import static java.util.Comparator.comparing;
 import static java.util.stream.Collectors.joining;
 
 import com.google.auto.value.AutoValue;
@@ -67,7 +68,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
@@ -1004,7 +1004,7 @@ class ChangeNotesParser {
   }
 
   private void updatePatchSetStates() {
-    Set<PatchSet.Id> missing = new TreeSet<>(ReviewDbUtil.intKeyOrdering());
+    Set<PatchSet.Id> missing = new TreeSet<>(comparing(PatchSet.Id::get));
     for (Iterator<PatchSet> it = patchSets.values().iterator(); it.hasNext(); ) {
       PatchSet ps = it.next();
       if (ps.getRevision().equals(PARTIAL_PATCH_SET)) {

java/com/google/gerrit/server/submit/SubmitStrategyOp.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.submit;
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
+import static java.util.Comparator.comparing;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.base.Function;
@@ -32,7 +33,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.IdentifiedUser;
@@ -182,8 +182,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
         continue; // Bogus ref, can't be merged into tip so we don't care.
       }
     }
-    commits.sort(
-        ReviewDbUtil.intKeyOrdering().reverse().onResultOf(CodeReviewCommit::getPatchsetId));
+    commits.sort(comparing((CodeReviewCommit c) -> c.getPatchsetId().get()).reversed());
     CodeReviewCommit result = MergeUtil.findAnyMergedInto(rw, commits, tip);
     if (result == null) {
       return null;

javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
 import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
@@ -21,6 +22,7 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REMOVED;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Comparator.comparing;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 import static org.junit.Assert.fail;
 
@@ -44,7 +46,6 @@ import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.ReviewerSet;
@@ -405,10 +406,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     update.commit();
 
     ChangeNotes notes = newNotes(c);
-    List<PatchSetApproval> approvals =
-        ReviewDbUtil.intKeyOrdering()
-            .onResultOf(PatchSetApproval::getAccountId)
-            .sortedCopy(notes.getApprovals().get(c.currentPatchSetId()));
+    ImmutableList<PatchSetApproval> approvals =
+        notes
+            .getApprovals()
+            .get(c.currentPatchSetId())
+            .stream()
+            .sorted(comparing(a -> a.getAccountId().get()))
+            .collect(toImmutableList());
     assertThat(approvals).hasSize(2);
 
     assertThat(approvals.get(0).getAccountId()).isEqualTo(changeOwner.getAccountId());