Move DfsPackDescription comparators to common location

There are several ways of comparing DfsPackDescriptions for different
purposes, such as object lookup search order and reftable ordering. Some
of these are later compounded into comparators on other objects, so they
appear in the code as Comparator<DfsReftable>, for example.

Put all the DfsPackDescription comparators in static methods on
DfsPackDescription itself. Stop implementing Comparable, to avoid giving
the impression that there is always one true and correct way of sorting
packs.

Change-Id: Ia5ca65249c13373f7ef5b8a5d1ad50a26577706c

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackDescriptionTest.java

@@ -0,0 +1,273 @@
+/*
+ * Copyright (C) 2018, Google LLC.
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.eclipse.jgit.internal.storage.dfs;
+
+import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.COMPACT;
+import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.GC;
+import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.GC_REST;
+import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.INSERT;
+import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.RECEIVE;
+import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX;
+import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
+import static org.junit.Assert.assertEquals;
+
+import java.util.Comparator;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
+import org.junit.Before;
+import org.junit.Test;
+
+public final class DfsPackDescriptionTest {
+	private AtomicInteger counter;
+
+	@Before
+	public void setUp() {
+		counter = new AtomicInteger();
+	}
+
+	@Test
+	public void objectLookupComparatorEqual() throws Exception {
+		DfsPackDescription a = create(RECEIVE);
+		a.setFileSize(PACK, 1);
+		a.setFileSize(INDEX, 1);
+		a.setLastModified(1);
+		a.setObjectCount(1);
+		a.setMaxUpdateIndex(1);
+
+		DfsPackDescription b = create(INSERT);
+		b.setFileSize(PACK, 1);
+		b.setFileSize(INDEX, 2);
+		b.setLastModified(1);
+		b.setObjectCount(1);
+		b.setMaxUpdateIndex(2);
+
+		assertComparesEqual(DfsPackDescription.objectLookupComparator(), a, b);
+	}
+
+	@Test
+	public void objectLookupComparatorPackSource() throws Exception {
+		DfsPackDescription a = create(COMPACT);
+		a.setFileSize(PACK, 2);
+		a.setLastModified(1);
+		a.setObjectCount(2);
+
+		DfsPackDescription b = create(GC);
+		b.setFileSize(PACK, 1);
+		b.setLastModified(2);
+		b.setObjectCount(1);
+
+		assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b);
+	}
+
+	@Test
+	public void objectLookupComparatorGcFileSize() throws Exception {
+		// a is older and smaller.
+		DfsPackDescription a = create(GC_REST);
+		a.setFileSize(PACK, 100);
+		a.setLastModified(1);
+		a.setObjectCount(2);
+
+		// b is newer and larger.
+		DfsPackDescription b = create(GC_REST);
+		b.setFileSize(PACK, 200);
+		b.setLastModified(2);
+		b.setObjectCount(1);
+
+		// Since they have the same GC type, tiebreaker is size, and a comes first.
+		assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b);
+	}
+
+	@Test
+	public void objectLookupComparatorNonGcLastModified()
+			throws Exception {
+		// a is older and smaller.
+		DfsPackDescription a = create(INSERT);
+		a.setFileSize(PACK, 100);
+		a.setLastModified(1);
+		a.setObjectCount(2);
+
+		// b is newer and larger.
+		DfsPackDescription b = create(INSERT);
+		b.setFileSize(PACK, 200);
+		b.setLastModified(2);
+		b.setObjectCount(1);
+
+		// Since they have the same type but not GC, tiebreaker is last modified,
+		// and b comes first.
+		assertComparesLessThan(DfsPackDescription.objectLookupComparator(), b, a);
+	}
+
+	@Test
+	public void objectLookupComparatorObjectCount() throws Exception {
+		DfsPackDescription a = create(INSERT);
+		a.setObjectCount(1);
+
+		DfsPackDescription b = create(INSERT);
+		b.setObjectCount(2);
+
+		assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b);
+	}
+
+	@Test
+	public void reftableComparatorEqual() throws Exception {
+		DfsPackDescription a = create(INSERT);
+		a.setFileSize(PACK, 100);
+		a.setObjectCount(1);
+
+		DfsPackDescription b = create(INSERT);
+		b.setFileSize(PACK, 200);
+		a.setObjectCount(2);
+
+		assertComparesEqual(DfsPackDescription.reftableComparator(), a, b);
+	}
+
+	@Test
+	public void reftableComparatorPackSource() throws Exception {
+		DfsPackDescription a = create(INSERT);
+		a.setMaxUpdateIndex(1);
+		a.setLastModified(1);
+
+		DfsPackDescription b = create(GC);
+		b.setMaxUpdateIndex(2);
+		b.setLastModified(2);
+
+		assertComparesLessThan(DfsPackDescription.reftableComparator(), b, a);
+	}
+
+	@Test
+	public void reftableComparatorMaxUpdateIndex() throws Exception {
+		DfsPackDescription a = create(INSERT);
+		a.setMaxUpdateIndex(1);
+		a.setLastModified(2);
+
+		DfsPackDescription b = create(INSERT);
+		b.setMaxUpdateIndex(2);
+		b.setLastModified(1);
+
+		assertComparesLessThan(DfsPackDescription.reftableComparator(), a, b);
+	}
+
+	@Test
+	public void reftableComparatorLastModified() throws Exception {
+		DfsPackDescription a = create(INSERT);
+		a.setLastModified(1);
+
+		DfsPackDescription b = create(INSERT);
+		b.setLastModified(2);
+
+		assertComparesLessThan(DfsPackDescription.reftableComparator(), a, b);
+	}
+
+	@Test
+	public void reuseComparatorEqual() throws Exception {
+		DfsPackDescription a = create(RECEIVE);
+		a.setFileSize(PACK, 1);
+		a.setFileSize(INDEX, 1);
+		a.setLastModified(1);
+		a.setObjectCount(1);
+		a.setMaxUpdateIndex(1);
+
+		DfsPackDescription b = create(INSERT);
+		b.setFileSize(PACK, 2);
+		b.setFileSize(INDEX, 2);
+		b.setLastModified(2);
+		b.setObjectCount(2);
+		b.setMaxUpdateIndex(2);
+
+		assertComparesEqual(DfsPackDescription.reuseComparator(), a, b);
+	}
+
+	@Test
+	public void reuseComparatorGcPackSize() throws Exception {
+		DfsPackDescription a = create(GC_REST);
+		a.setFileSize(PACK, 1);
+		a.setFileSize(INDEX, 1);
+		a.setLastModified(2);
+		a.setObjectCount(1);
+		a.setMaxUpdateIndex(1);
+
+		DfsPackDescription b = create(GC_REST);
+		b.setFileSize(PACK, 2);
+		b.setFileSize(INDEX, 2);
+		b.setLastModified(1);
+		b.setObjectCount(2);
+		b.setMaxUpdateIndex(2);
+
+		assertComparesLessThan(DfsPackDescription.reuseComparator(), b, a);
+	}
+
+	private DfsPackDescription create(PackSource source) {
+		return new DfsPackDescription(
+				new DfsRepositoryDescription("repo"),
+				"pack_" + counter.incrementAndGet(),
+				source);
+	}
+
+	private static <T> void assertComparesEqual(
+			Comparator<T> comparator, T o1, T o2) {
+		assertEquals(
+				"first object must compare equal to itself",
+				0, comparator.compare(o1, o1));
+		assertEquals(
+				"second object must compare equal to itself",
+				0, comparator.compare(o2, o2));
+		assertEquals(
+				"first object must compare equal to second object",
+				0, comparator.compare(o1, o2));
+	}
+
+	private static <T> void assertComparesLessThan(
+			Comparator<T> comparator, T o1, T o2) {
+		assertEquals(
+				"first object must compare equal to itself",
+				0, comparator.compare(o1, o1));
+		assertEquals(
+				"second object must compare equal to itself",
+				0, comparator.compare(o2, o2));
+		assertEquals(
+				"first object must compare less than second object",
+				-1, comparator.compare(o1, o2));
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java

@@ -607,7 +607,7 @@ public abstract class DfsObjDatabase extends ObjectDatabase {
 		Map<DfsPackDescription, DfsReftable> reftables = reftableMap(old);
 
 		List<DfsPackDescription> scanned = listPacks();
-		Collections.sort(scanned);
+		Collections.sort(scanned, DfsPackDescription.objectLookupComparator());
 
 		List<DfsPackFile> newPacks = new ArrayList<>(scanned.size());
 		List<DfsReftable> newReftables = new ArrayList<>(scanned.size());
@@ -668,26 +668,9 @@ public abstract class DfsObjDatabase extends ObjectDatabase {
 	 * @return comparator to sort {@link DfsReftable} by priority.
 	 */
 	protected Comparator<DfsReftable> reftableComparator() {
-		return (fa, fb) -> {
-			DfsPackDescription a = fa.getPackDescription();
-			DfsPackDescription b = fb.getPackDescription();
-
-			// GC, COMPACT reftables first by reversing default order.
-			int c = PackSource.DEFAULT_COMPARATOR.reversed()
-					.compare(a.getPackSource(), b.getPackSource());
-			if (c != 0) {
-				return c;
-			}
-
-			// Lower maxUpdateIndex first.
-			c = Long.signum(a.getMaxUpdateIndex() - b.getMaxUpdateIndex());
-			if (c != 0) {
-				return c;
-			}
-
-			// Older reftable first.
-			return Long.signum(a.getLastModified() - b.getLastModified());
-		};
+		return Comparator.comparing(
+				DfsReftable::getPackDescription,
+				DfsPackDescription.reftableComparator());
 	}
 
 	/**

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java

@@ -404,12 +404,11 @@ public class DfsPackCompactor {
 		// Sort packs by description ordering, this places newer packs before
 		// older packs, allowing the PackWriter to be handed newer objects
 		// first and older objects last.
-		Collections.sort(srcPacks, new Comparator<DfsPackFile>() {
-			@Override
-			public int compare(DfsPackFile a, DfsPackFile b) {
-				return a.getPackDescription().compareTo(b.getPackDescription());
-			}
-		});
+		Collections.sort(
+				srcPacks,
+				Comparator.comparing(
+						DfsPackFile::getPackDescription,
+						DfsPackDescription.objectLookupComparator()));
 
 		rw = new RevWalk(ctx);
 		added = rw.newFlag("ADDED"); //$NON-NLS-1$

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java

@@ -47,6 +47,7 @@ import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.REFTABLE;
 
 import java.util.Arrays;
+import java.util.Comparator;
 
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
@@ -62,7 +63,88 @@ import org.eclipse.jgit.storage.pack.PackStatistics;
  * Instances of this class are cached with the DfsPackFile, and should not be
  * modified once initialized and presented to the JGit DFS library.
  */
-public class DfsPackDescription implements Comparable<DfsPackDescription> {
+public class DfsPackDescription {
+	/**
+	 * Comparator for packs when looking up objects in indexes.
+	 * <p>
+	 * This comparator tries to position packs in the order readers should examine
+	 * them when looking for objects by SHA-1. The default tries to sort packs
+	 * with more recent modification dates before older packs, and packs with
+	 * fewer objects before packs with more objects.
+	 * <p>
+	 * Uses {@link PackSource#DEFAULT_COMPARATOR} for sorting sources.
+	 *
+	 * @return comparator.
+	 */
+	public static Comparator<DfsPackDescription> objectLookupComparator() {
+		return Comparator.comparing(
+					DfsPackDescription::getPackSource, PackSource.DEFAULT_COMPARATOR)
+			.thenComparing((a, b) -> {
+				PackSource as = a.getPackSource();
+				PackSource bs = b.getPackSource();
+
+				// Tie break GC type packs by smallest first. There should be at most
+				// one of each source, but when multiple exist concurrent GCs may have
+				// run. Preferring the smaller file selects higher quality delta
+				// compression, placing less demand on the DfsBlockCache.
+				if (as == bs && isGC(as)) {
+					int cmp = Long.signum(a.getFileSize(PACK) - b.getFileSize(PACK));
+					if (cmp != 0) {
+						return cmp;
+					}
+				}
+
+				// Newer packs should sort first.
+				int cmp = Long.signum(b.getLastModified() - a.getLastModified());
+				if (cmp != 0) {
+					return cmp;
+				}
+
+				// Break ties on smaller index. Readers may get lucky and find
+				// the object they care about in the smaller index. This also pushes
+				// big historical packs to the end of the list, due to more objects.
+				return Long.signum(a.getObjectCount() - b.getObjectCount());
+			});
+	}
+
+	static Comparator<DfsPackDescription> reftableComparator() {
+		return (a, b) -> {
+				// GC, COMPACT reftables first by reversing default order.
+				int c = PackSource.DEFAULT_COMPARATOR.reversed()
+						.compare(a.getPackSource(), b.getPackSource());
+				if (c != 0) {
+					return c;
+				}
+
+				// Lower maxUpdateIndex first.
+				c = Long.signum(a.getMaxUpdateIndex() - b.getMaxUpdateIndex());
+				if (c != 0) {
+					return c;
+				}
+
+				// Older reftable first.
+				return Long.signum(a.getLastModified() - b.getLastModified());
+			};
+	}
+
+	static Comparator<DfsPackDescription> reuseComparator() {
+		return (a, b) -> {
+			PackSource as = a.getPackSource();
+			PackSource bs = b.getPackSource();
+
+			if (as == bs && DfsPackDescription.isGC(as)) {
+				// Push smaller GC files last; these likely have higher quality
+				// delta compression and the contained representation should be
+				// favored over other files.
+				return Long.signum(b.getFileSize(PACK) - a.getFileSize(PACK));
+			}
+
+			// DfsPackDescription.compareTo already did a reasonable sort.
+			// Rely on Arrays.sort being stable, leaving equal elements.
+			return 0;
+		};
+	}
+
 	private final DfsRepositoryDescription repoDesc;
 	private final String packName;
 	private PackSource packSource;
@@ -461,48 +543,6 @@ public class DfsPackDescription implements Comparable<DfsPackDescription> {
 		return false;
 	}
 
-	/**
-	 * {@inheritDoc}
-	 * <p>
-	 * Sort packs according to the optimal lookup ordering.
-	 * <p>
-	 * This method tries to position packs in the order readers should examine
-	 * them when looking for objects by SHA-1. The default tries to sort packs
-	 * with more recent modification dates before older packs, and packs with
-	 * fewer objects before packs with more objects.
-	 */
-	@Override
-	public int compareTo(DfsPackDescription b) {
-		// Cluster by PackSource, pushing UNREACHABLE_GARBAGE to the end.
-		PackSource as = getPackSource();
-		PackSource bs = b.getPackSource();
-		int cmp = PackSource.DEFAULT_COMPARATOR.compare(as, bs);
-		if (cmp != 0) {
-			return cmp;
-		}
-
-		// Tie break GC type packs by smallest first. There should be at most
-		// one of each source, but when multiple exist concurrent GCs may have
-		// run. Preferring the smaller file selects higher quality delta
-		// compression, placing less demand on the DfsBlockCache.
-		if (as == bs && isGC(as)) {
-			cmp = Long.signum(getFileSize(PACK) - b.getFileSize(PACK));
-			if (cmp != 0) {
-				return cmp;
-			}
-		}
-
-		// Newer packs should sort first.
-		cmp = Long.signum(b.getLastModified() - getLastModified());
-		if (cmp != 0)
-			return cmp;
-
-		// Break ties on smaller index. Readers may get lucky and find
-		// the object they care about in the smaller index. This also pushes
-		// big historical packs to the end of the list, due to more objects.
-		return Long.signum(getObjectCount() - b.getObjectCount());
-	}
-
 	static boolean isGC(PackSource s) {
 		switch (s) {
 		case GC:

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java

@@ -45,7 +45,6 @@
 package org.eclipse.jgit.internal.storage.dfs;
 
 import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.UNREACHABLE_GARBAGE;
-import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
 import static org.eclipse.jgit.lib.Constants.OBJECT_ID_LENGTH;
 
 import java.io.IOException;
@@ -66,7 +65,6 @@ import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.StoredObjectRepresentationNotAvailableException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackList;
-import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
 import org.eclipse.jgit.internal.storage.file.BitmapIndexImpl;
 import org.eclipse.jgit.internal.storage.file.PackBitmapIndex;
 import org.eclipse.jgit.internal.storage.file.PackIndex;
@@ -611,26 +609,9 @@ public class DfsReader extends ObjectReader implements ObjectReuseAsIs {
 		}
 	}
 
-	private static final Comparator<DfsPackFile> PACK_SORT_FOR_REUSE = new Comparator<DfsPackFile>() {
-		@Override
-		public int compare(DfsPackFile af, DfsPackFile bf) {
-			DfsPackDescription ad = af.getPackDescription();
-			DfsPackDescription bd = bf.getPackDescription();
-			PackSource as = ad.getPackSource();
-			PackSource bs = bd.getPackSource();
-
-			if (as == bs && DfsPackDescription.isGC(as)) {
-				// Push smaller GC files last; these likely have higher quality
-				// delta compression and the contained representation should be
-				// favored over other files.
-				return Long.signum(bd.getFileSize(PACK) - ad.getFileSize(PACK));
-			}
-
-			// DfsPackDescription.compareTo already did a reasonable sort.
-			// Rely on Arrays.sort being stable, leaving equal elements.
-			return 0;
-		}
-	};
+	private static final Comparator<DfsPackFile> PACK_SORT_FOR_REUSE =
+		Comparator.comparing(
+				DfsPackFile::getPackDescription, DfsPackDescription.reuseComparator());
 
 	private List<DfsPackFile> sortPacksForSelectRepresentation()
 			throws IOException {