DfsPackDescription: Disallow null PackSource

In normal operation, the source of a pack should never be null; the DFS
implementation should always know where a pack came from. Existing
implementations in InMemoryRepository and at Google always have the
source available at construction time.

The problem with null PackSources in the previous implementation was it
made the DfsPackDescription#compareTo method intransitive. Specifically,
it skips comparing the sources at all if *either* operand is null.
Suppose we have three descriptions A, B, and C, where all fields are
equal except the PackSource, and:
 * A's source is INSERT
 * B's source is null
 * C's source is RECEIVE
In this case, A.compareTo(B) == 0, and B.compareTo(C) == 0, since all
fields are equal except the source, which is skipped. But
A.compareTo(C) != 0, since A and B have different sources.

Avoid this problem in compareTo by enforcing that the source is never
null. We could of course assign an arbitrary category number to a null
source in order to make comparison transitive[1], but it's simpler to
implement and reason about if the field is non-nullable, and there is no
real-world use case to make it null.

Although a non-null source is required at construction time, the field
is currently still mutable: DfsPackDecscription#setPackSource is used by
DfsInserterTest to mark packs as garbage. This could probably be
avoided as well, allowing us to convert packSource to a final field, but
doing so is beyond the scope of this change.

[1] The astute reader will notice this is already done by
    DfsObjDatabase#reftableComparator(). In fact, the reason that
    different comparator implementations non-obviously have different
    semantics for this nullable field is another reason why it's clearer
    to avoid null entirely.

Change-Id: I85a2aaf3fd6d4868f241f7972a0349f087830ffa

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

@@ -589,7 +589,7 @@ public abstract class DfsObjDatabase extends ObjectDatabase {
 			DfsPackDescription b = fb.getPackDescription();
 
 			// GC, COMPACT reftables first by higher category.
-			int c = category(b) - category(a);
+			int c = b.getPackSource().category - a.getPackSource().category;
 			if (c != 0) {
 				return c;
 			}
@@ -605,11 +605,6 @@ public abstract class DfsObjDatabase extends ObjectDatabase {
 		};
 	}
 
-	static int category(DfsPackDescription d) {
-		PackSource s = d.getPackSource();
-		return s != null ? s.category : 0;
-	}
-
 	/**
 	 * Clears the cached list of packs, forcing them to be scanned again.
 	 */

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

@@ -48,6 +48,7 @@ import static org.eclipse.jgit.internal.storage.pack.PackExt.REFTABLE;
 
 import java.util.Arrays;
 
+import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
 import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.reftable.ReftableWriter;
@@ -93,11 +94,15 @@ public class DfsPackDescription implements Comparable<DfsPackDescription> {
 	 *            name of the pack file. Must end with ".pack".
 	 * @param repoDesc
 	 *            description of the repo containing the pack file.
+	 * @param packSource
+	 *            the source of the pack.
 	 */
-	public DfsPackDescription(DfsRepositoryDescription repoDesc, String name) {
+	public DfsPackDescription(DfsRepositoryDescription repoDesc, String name,
+			@NonNull PackSource packSource) {
 		this.repoDesc = repoDesc;
 		int dot = name.lastIndexOf('.');
 		this.packName = (dot < 0) ? name : name.substring(0, dot);
+		this.packSource = packSource;
 
 		int extCnt = PackExt.values().length;
 		sizeMap = new long[extCnt];
@@ -162,6 +167,7 @@ public class DfsPackDescription implements Comparable<DfsPackDescription> {
 	 *
 	 * @return the source of the pack.
 	 */
+	@NonNull
 	public PackSource getPackSource() {
 		return packSource;
 	}
@@ -173,7 +179,7 @@ public class DfsPackDescription implements Comparable<DfsPackDescription> {
 	 *            the source of the pack.
 	 * @return {@code this}
 	 */
-	public DfsPackDescription setPackSource(PackSource source) {
+	public DfsPackDescription setPackSource(@NonNull PackSource source) {
 		packSource = source;
 		return this;
 	}
@@ -470,25 +476,24 @@ public class DfsPackDescription implements Comparable<DfsPackDescription> {
 		// Cluster by PackSource, pushing UNREACHABLE_GARBAGE to the end.
 		PackSource as = getPackSource();
 		PackSource bs = b.getPackSource();
-		if (as != null && bs != null) {
+		int cmp = as.category - bs.category;
-			int cmp = as.category - bs.category;
+		if (cmp != 0) {
-			if (cmp != 0)
+			return cmp;
-				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 != null && as == bs && isGC(as)) {
+		if (as == bs && isGC(as)) {
-			int cmp = Long.signum(getFileSize(PACK) - b.getFileSize(PACK));
+			cmp = Long.signum(getFileSize(PACK) - b.getFileSize(PACK));
 			if (cmp != 0) {
 				return cmp;
 			}
 		}
 
 		// Newer packs should sort first.
-		int cmp = Long.signum(b.getLastModified() - getLastModified());
+		cmp = Long.signum(b.getLastModified() - getLastModified());
 		if (cmp != 0)
 			return cmp;
 

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

@@ -619,7 +619,7 @@ public class DfsReader extends ObjectReader implements ObjectReuseAsIs {
 			PackSource as = ad.getPackSource();
 			PackSource bs = bd.getPackSource();
 
-			if (as != null && as == bs && DfsPackDescription.isGC(as)) {
+			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.

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

@@ -10,6 +10,7 @@ import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.eclipse.jgit.annotations.Nullable;
+import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource;
 import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.reftable.ReftableConfig;
 import org.eclipse.jgit.lib.RefDatabase;
@@ -118,10 +119,10 @@ public class InMemoryRepository extends DfsRepository {
 		@Override
 		protected DfsPackDescription newPack(PackSource source) {
 			int id = packId.incrementAndGet();
-			DfsPackDescription desc = new MemPack(
+			return new MemPack(
 					"pack-" + id + "-" + source.name(), //$NON-NLS-1$ //$NON-NLS-2$
-					getRepository().getDescription());
+					getRepository().getDescription(),
-			return desc.setPackSource(source);
+					source);
 		}
 
 		@Override
@@ -169,8 +170,8 @@ public class InMemoryRepository extends DfsRepository {
 	private static class MemPack extends DfsPackDescription {
 		final byte[][] fileMap = new byte[PackExt.values().length][];
 
-		MemPack(String name, DfsRepositoryDescription repoDesc) {
+		MemPack(String name, DfsRepositoryDescription repoDesc, PackSource source) {
-			super(repoDesc, name);
+			super(repoDesc, name, source);
 		}
 
 		void put(PackExt ext, byte[] data) {