From 7fbff35887a4179d33b17fce96191c82b397ebd7 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Wed, 10 Feb 2021 23:26:17 -0700 Subject: [PATCH] Pack: Replace extensions bitset with bitmapIdx PackFile The only extension that was ever consulted from the bitmap was the bitmap index. We can simplify the Pack code as well as the code of all the callers if we focus on just that usage. Change-Id: I799ddfdee93142af67ce5081d14a430d36aa4c15 Signed-off-by: Nasser Grainawi --- .../jgit/internal/storage/file/PackTest.java | 2 +- .../storage/file/ObjectDirectory.java | 39 +++++++++---------- .../jgit/internal/storage/file/Pack.java | 38 ++++++++---------- .../internal/storage/file/PackDirectory.java | 5 +-- 4 files changed, 38 insertions(+), 46 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackTest.java index 85bd31d71..a3596541f 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackTest.java @@ -261,7 +261,7 @@ public void testDelta_FailsOver2GiB() throws Exception { new PackIndexWriterV1(f).write(list, footer); } - Pack pack = new Pack(packName, PackExt.INDEX.getBit()); + Pack pack = new Pack(packName, null); try { pack.get(wc, b); fail("expected LargeObjectException.ExceedsByteArrayLimit"); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index e71a96060..170df5780 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -11,8 +11,8 @@ package org.eclipse.jgit.internal.storage.file; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX; import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; +import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; import java.io.BufferedReader; import java.io.File; @@ -31,7 +31,6 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.ObjectToPack; -import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.internal.storage.pack.PackWriter; import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; @@ -216,26 +215,26 @@ public Collection getPacks() { * Add a single existing pack to the list of available pack files. */ @Override - public Pack openPack(File pack) - throws IOException { - final String p = pack.getName(); - if (p.length() != 50 || !p.startsWith("pack-") || !p.endsWith(".pack")) //$NON-NLS-1$ //$NON-NLS-2$ - throw new IOException(MessageFormat.format(JGitText.get().notAValidPack, pack)); - - // The pack and index are assumed to exist. The existence of other - // extensions needs to be explicitly checked. - // - int extensions = PACK.getBit() | INDEX.getBit(); - final String base = p.substring(0, p.length() - 4); - for (PackExt ext : PackExt.values()) { - if ((extensions & ext.getBit()) == 0) { - final String name = base + ext.getExtension(); - if (new File(pack.getParentFile(), name).exists()) - extensions |= ext.getBit(); - } + public Pack openPack(File pack) throws IOException { + PackFile pf; + try { + pf = new PackFile(pack); + } catch (IllegalArgumentException e) { + throw new IOException( + MessageFormat.format(JGitText.get().notAValidPack, pack), + e); } - Pack res = new Pack(pack, extensions); + String p = pf.getName(); + // TODO(nasserg): See if PackFile can do these checks instead + if (p.length() != 50 || !p.startsWith("pack-") //$NON-NLS-1$ + || !pf.getPackExt().equals(PACK)) { + throw new IOException( + MessageFormat.format(JGitText.get().notAValidPack, pack)); + } + + PackFile bitmapIdx = pf.create(BITMAP_INDEX); + Pack res = new Pack(pack, bitmapIdx.exists() ? bitmapIdx : null); packed.insert(res); return res; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index fa938b311..5efd4c5bf 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -12,7 +12,6 @@ package org.eclipse.jgit.internal.storage.file; -import static org.eclipse.jgit.internal.storage.pack.PackExt.BITMAP_INDEX; import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX; import static org.eclipse.jgit.internal.storage.pack.PackExt.KEEP; @@ -38,6 +37,7 @@ import java.util.zip.DataFormatException; import java.util.zip.Inflater; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.LargeObjectException; import org.eclipse.jgit.errors.MissingObjectException; @@ -51,7 +51,6 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.BinaryDelta; import org.eclipse.jgit.internal.storage.pack.ObjectToPack; -import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.internal.storage.pack.PackOutputStream; import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; @@ -80,8 +79,6 @@ public class Pack implements Iterable { private final PackFile packFile; - private final int extensions; - private PackFile keepFile; final int hash; @@ -105,7 +102,8 @@ public class Pack implements Iterable { private volatile Exception invalidatingCause; - private boolean invalidBitmap; + @Nullable + private PackFile bitmapIdxFile; private AtomicInteger transientErrorCount = new AtomicInteger(); @@ -131,14 +129,14 @@ public class Pack implements Iterable { * * @param packFile * path of the .pack file holding the data. - * @param extensions - * additional pack file extensions with the same base as the pack + * @param bitmapIdxFile + * existing bitmap index file with the same base as the pack */ - public Pack(File packFile, int extensions) { + public Pack(File packFile, @Nullable PackFile bitmapIdxFile) { this.packFile = new PackFile(packFile); this.fileSnapshot = PackFileSnapshot.save(packFile); this.packLastModified = fileSnapshot.lastModifiedInstant(); - this.extensions = extensions; + this.bitmapIdxFile = bitmapIdxFile; // Multiply by 31 here so we can more directly combine with another // value in WindowCache.hash(), without doing the multiply there. @@ -1124,26 +1122,28 @@ private long findEndOffset(long startOffset) } synchronized PackBitmapIndex getBitmapIndex() throws IOException { - if (invalid || invalidBitmap) + if (invalid || bitmapIdxFile == null) { return null; - if (bitmapIdx == null && hasExt(BITMAP_INDEX)) { + } + if (bitmapIdx == null) { final PackBitmapIndex idx; try { - idx = PackBitmapIndex.open(packFile.create(BITMAP_INDEX), idx(), + idx = PackBitmapIndex.open(bitmapIdxFile, idx(), getReverseIdx()); } catch (FileNotFoundException e) { // Once upon a time this bitmap file existed. Now it // has been removed. Most likely an external gc has // removed this packfile and the bitmap - invalidBitmap = true; - return null; + bitmapIdxFile = null; + return null; } // At this point, idx() will have set packChecksum. - if (Arrays.equals(packChecksum, idx.packChecksum)) + if (Arrays.equals(packChecksum, idx.packChecksum)) { bitmapIdx = idx; - else - invalidBitmap = true; + } else { + bitmapIdxFile = null; + } } return bitmapIdx; } @@ -1179,10 +1179,6 @@ private void setCorrupt(long offset) { } } - private boolean hasExt(PackExt ext) { - return (extensions & ext.getBit()) != 0; - } - @SuppressWarnings("nls") @Override public String toString() { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java index 2e68d4624..007205e55 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java @@ -421,10 +421,7 @@ private PackList scanPacksImpl(PackList old) { continue; } - list.add(new Pack(packFile, - packFilesByExt.containsKey(BITMAP_INDEX) - ? BITMAP_INDEX.getBit() - : 0)); + list.add(new Pack(packFile, packFilesByExt.get(BITMAP_INDEX))); foundNew = true; }