From 43ec590d0e7c6b294bcbb1f7e4c2490b6b984c20 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 29 May 2018 17:53:26 -0700 Subject: [PATCH] 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 --- .../internal/storage/dfs/DfsObjDatabase.java | 7 +----- .../storage/dfs/DfsPackDescription.java | 23 +++++++++++-------- .../jgit/internal/storage/dfs/DfsReader.java | 2 +- .../storage/dfs/InMemoryRepository.java | 11 +++++---- 4 files changed, 22 insertions(+), 21 deletions(-) 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 ca54ee22e..8e04f66a8 100644 --- 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 45eb7b0e1..37bd59a65 100644 --- 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 { * 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 { * * @return the source of the pack. */ + @NonNull public PackSource getPackSource() { return packSource; } @@ -173,7 +179,7 @@ public class DfsPackDescription implements Comparable { * 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 { // 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 197114bd6..ec0a778e0 100644 --- 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 662c3fef8..5b6894da9 100644 --- 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) {