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

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java
index ca54ee22ea21f1b1ba8446e7c169baa46bc3b8bd..8e04f66a88bd2f15a38096d90ae0c91dfad8e86e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java
+++ b/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.
         */

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java
index 45eb7b0e1a4c80f7f46d851b03b5685c3fede00d..37bd59a6508c4db96d584c30ca3cc55780ae1125 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java
+++ b/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;
-                       if (cmp != 0)
-                               return cmp;
+               int cmp = as.category - bs.category;
+               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 != null && as == bs && isGC(as)) {
-                       int cmp = Long.signum(getFileSize(PACK) - b.getFileSize(PACK));
+               if (as == bs && isGC(as)) {
+                       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;
 

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
index 197114bd6b4e01594cc6e91f9330fcbb8e4b957e..ec0a778e0748b30d60c6ce6d84397b3e707c7c91 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
+++ b/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.

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
index 662c3fef81611cb6b62cb11950a175779a43666d..5b6894da9c7c2b9eda46cd2d38aecfc4425e3b32 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
+++ b/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());
-                       return desc.setPackSource(source);
+                                       getRepository().getDescription(),
+                                       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) {
-                       super(repoDesc, name);
+               MemPack(String name, DfsRepositoryDescription repoDesc, PackSource source) {
+                       super(repoDesc, name, source);
                }
 
                void put(PackExt ext, byte[] data) {