From 2a6b2eddcfac17ef5ff3b6b28dfaadd83e34956a Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Thu, 4 Mar 2021 14:14:43 -0700 Subject: [PATCH] PackFile: Add id + ext based constructors Add new constructors to PackFile to improve a common use case where callers know the directory, id, and extension, but previously needed to construct a valid file name (with prefix, '.', etc) to create a PackFile. Most callers can use the variant that has id as an ObjectId, but provide an id as String variant too. Change-Id: I39e4466abe8c9509f5916d5bfe675066570b8585 Signed-off-by: Nasser Grainawi --- .../eclipse/jgit/junit/TestRepository.java | 11 +----- .../storage/file/AbbreviationTest.java | 6 +-- .../storage/file/ConcurrentRepackTest.java | 12 ++---- .../internal/storage/file/PackFileTest.java | 31 ++++++++++++--- .../internal/storage/file/PackWriterTest.java | 4 +- .../jgit/internal/storage/file/GC.java | 12 ++---- .../storage/file/LocalCachedPack.java | 3 +- .../file/ObjectDirectoryPackParser.java | 9 +++-- .../jgit/internal/storage/file/PackFile.java | 33 ++++++++++++++++ .../internal/storage/file/PackInserter.java | 9 +++-- .../jgit/transport/WalkPushConnection.java | 38 ++++++++++--------- 11 files changed, 106 insertions(+), 62 deletions(-) diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java index 24f7741f1..0232156a4 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java @@ -915,9 +915,8 @@ public void packAndPrune() throws Exception { all.add(r.getObjectId()); pw.preparePack(m, all, PackWriter.NONE); - final ObjectId name = pw.computeName(); - - pack = nameFor(odb, name, ".pack"); + pack = new PackFile(odb.getPackDirectory(), pw.computeName(), + PackExt.PACK); try (OutputStream out = new BufferedOutputStream(new FileOutputStream(pack))) { pw.writePack(m, m, out); @@ -962,12 +961,6 @@ private static void prunePacked(ObjectDirectory odb) throws IOException { } } - private static PackFile nameFor(ObjectDirectory odb, ObjectId name, - String t) { - File packdir = odb.getPackDirectory(); - return new PackFile(packdir, "pack-" + name.name() + t); - } - private void writeFile(File p, byte[] bin) throws IOException, ObjectWritingException { final LockFile lck = new LockFile(p); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AbbreviationTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AbbreviationTest.java index 45d864d45..bd36337f3 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AbbreviationTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/AbbreviationTest.java @@ -28,6 +28,7 @@ import java.util.List; import org.eclipse.jgit.errors.AmbiguousObjectException; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.AbbreviatedObjectId; @@ -144,10 +145,9 @@ public void testAbbreviateIsActuallyUnique() throws Exception { objects.add(new PackedObjectInfo(ObjectId.fromRaw(idBuf))); } - String packName = "pack-" + id.name(); File packDir = db.getObjectDatabase().getPackDirectory(); - File idxFile = new File(packDir, packName + ".idx"); - File packFile = new File(packDir, packName + ".pack"); + PackFile idxFile = new PackFile(packDir, id, PackExt.INDEX); + PackFile packFile = idxFile.create(PackExt.PACK); FileUtils.mkdir(packDir, true); try (OutputStream dst = new BufferedOutputStream( new FileOutputStream(idxFile))) { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ConcurrentRepackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ConcurrentRepackTest.java index ca6350735..df5d952ee 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ConcurrentRepackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ConcurrentRepackTest.java @@ -194,9 +194,10 @@ private File[] pack(Repository src, RevObject... list) pw.addObject(o); } - final ObjectId name = pw.computeName(); - final PackFile packFile = fullPackFileName(name, ".pack"); - final PackFile idxFile = packFile.create(PackExt.INDEX); + PackFile packFile = new PackFile( + db.getObjectDatabase().getPackDirectory(), pw.computeName(), + PackExt.PACK); + PackFile idxFile = packFile.create(PackExt.INDEX); final File[] files = new File[] { packFile, idxFile }; write(files, pw); return files; @@ -243,11 +244,6 @@ private static void touch(Instant begin, File dir) throws IOException { } } - private PackFile fullPackFileName(ObjectId name, String suffix) { - final File packdir = db.getObjectDatabase().getPackDirectory(); - return new PackFile(packdir, "pack-" + name.name() + suffix); - } - private RevObject writeBlob(Repository repo, String data) throws IOException { final byte[] bytes = Constants.encode(data); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackFileTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackFileTest.java index 88d25b73d..619cfcac3 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackFileTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackFileTest.java @@ -17,10 +17,14 @@ import java.io.File; import org.eclipse.jgit.internal.storage.pack.PackExt; +import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; public class PackFileTest { - private static final String TEST_ID = "0123456789012345678901234567890123456789"; + private static final ObjectId TEST_OID = ObjectId + .fromString("0123456789012345678901234567890123456789"); + + private static final String TEST_ID = TEST_OID.name(); private static final String PREFIX = "pack-"; @@ -39,12 +43,20 @@ public class PackFileTest { new File(TEST_PACK_DIR, PREFIX + TEST_ID)); @Test - public void idIsSameFromFileOrDirAndName() throws Exception { - File pack = new File(TEST_PACK_DIR, PREFIX + TEST_ID); + public void objectsAreSameFromAnyConstructor() throws Exception { + String name = PREFIX + TEST_ID + "." + PackExt.PACK.getExtension(); + File pack = new File(TEST_PACK_DIR, name); PackFile pf = new PackFile(pack); - PackFile pfFromDirAndName = new PackFile(TEST_PACK_DIR, - PREFIX + TEST_ID); - assertEquals(pf.getId(), pfFromDirAndName.getId()); + PackFile pfFromDirAndName = new PackFile(TEST_PACK_DIR, name); + assertPackFilesEqual(pf, pfFromDirAndName); + + PackFile pfFromOIdAndExt = new PackFile(TEST_PACK_DIR, TEST_OID, + PackExt.PACK); + assertPackFilesEqual(pf, pfFromOIdAndExt); + + PackFile pfFromIdAndExt = new PackFile(TEST_PACK_DIR, TEST_ID, + PackExt.PACK); + assertPackFilesEqual(pf, pfFromIdAndExt); } @Test @@ -147,4 +159,11 @@ public void canCreateNonPreservedFromAnyPreservedExt() throws Exception { preservedWithExt.getPackExt()); } } + + private void assertPackFilesEqual(PackFile p1, PackFile p2) { + // for test purposes, considered equal if id, name, and ext are equal + assertEquals(p1.getId(), p2.getId()); + assertEquals(p1.getPackExt(), p2.getPackExt()); + assertEquals(p1.getName(), p2.getName()); + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java index 6e8584a9c..e422ab9db 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java @@ -684,9 +684,9 @@ private static PackIndex writePack(FileRepository repo, RevWalk walk, ObjectWalk ow = walk.toObjectWalkWithSameObjects(); pw.preparePack(NullProgressMonitor.INSTANCE, ow, want, have, NONE); - String id = pw.computeName().getName(); File packdir = repo.getObjectDatabase().getPackDirectory(); - PackFile packFile = new PackFile(packdir, "pack-" + id + ".pack"); + PackFile packFile = new PackFile(packdir, pw.computeName(), + PackExt.PACK); try (FileOutputStream packOS = new FileOutputStream(packFile)) { pw.writePack(NullProgressMonitor.INSTANCE, NullProgressMonitor.INSTANCE, packOS); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index 9366404ba..9ffff9f66 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -1163,7 +1163,7 @@ private Pack writePack(@NonNull Set want, checkCancelled(); // create temporary files - String id = pw.computeName().getName(); + ObjectId id = pw.computeName(); File packdir = repo.getObjectDatabase().getPackDirectory(); packdir.mkdirs(); tmpPack = File.createTempFile("gc_", ".pack_tmp", packdir); //$NON-NLS-1$ //$NON-NLS-2$ @@ -1213,7 +1213,8 @@ private Pack writePack(@NonNull Set want, } // rename the temporary files to real files - File realPack = nameFor(id, PackExt.PACK); + File packDir = repo.getObjectDatabase().getPackDirectory(); + PackFile realPack = new PackFile(packDir, id, PackExt.PACK); repo.getObjectDatabase().closeAllPackHandles(realPack); tmpPack.setReadOnly(); @@ -1223,7 +1224,7 @@ private Pack writePack(@NonNull Set want, File tmpExt = tmpEntry.getValue(); tmpExt.setReadOnly(); - File realExt = nameFor(id, tmpEntry.getKey()); + PackFile realExt = new PackFile(packDir, id, tmpEntry.getKey()); try { FileUtils.rename(tmpExt, realExt, StandardCopyOption.ATOMIC_MOVE); @@ -1269,11 +1270,6 @@ private Pack writePack(@NonNull Set want, } } - private PackFile nameFor(String name, PackExt ext) { - return new PackFile(repo.getObjectDatabase().getPackDirectory(), - "pack-" + name).create(ext); //$NON-NLS-1$ - } - private void checkCancelled() throws CancelledException { if (pm.isCancelled() || Thread.currentThread().isInterrupted()) { throw new CancelledException(JGitText.get().operationCanceled); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java index ae5bce698..f112947ba 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LocalCachedPack.java @@ -17,6 +17,7 @@ import org.eclipse.jgit.internal.storage.pack.CachedPack; 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.internal.storage.pack.StoredObjectRepresentation; @@ -88,6 +89,6 @@ private Pack getPackFile(String packName) throws FileNotFoundException { private String getPackFilePath(String packName) { final File packDir = odb.getPackDirectory(); - return new File(packDir, "pack-" + packName + ".pack").getPath(); //$NON-NLS-1$ //$NON-NLS-2$ + return new PackFile(packDir, packName, PackExt.PACK).getPath(); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java index 04d2ff8ab..dba8ccd99 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectoryPackParser.java @@ -27,6 +27,7 @@ import org.eclipse.jgit.errors.LockFailedException; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.CoreConfig; @@ -426,10 +427,10 @@ private PackLock renameAndOpenPack(String lockMessage) d.update(oeBytes); } - final String name = ObjectId.fromRaw(d.digest()).name(); - final File packDir = new File(db.getDirectory(), "pack"); //$NON-NLS-1$ - final File finalPack = new File(packDir, "pack-" + name + ".pack"); //$NON-NLS-1$ //$NON-NLS-2$ - final File finalIdx = new File(packDir, "pack-" + name + ".idx"); //$NON-NLS-1$ //$NON-NLS-2$ + ObjectId id = ObjectId.fromRaw(d.digest()); + File packDir = new File(db.getDirectory(), "pack"); //$NON-NLS-1$ + PackFile finalPack = new PackFile(packDir, id, PackExt.PACK); + PackFile finalIdx = finalPack.create(PackExt.INDEX); final PackLock keep = new PackLock(finalPack, db.getFS()); if (!packDir.exists() && !packDir.mkdir() && !packDir.exists()) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java index c2e6f324d..19979d0ed 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java @@ -16,6 +16,7 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.PackExt; +import org.eclipse.jgit.lib.ObjectId; /** * A pack file (or pack related) File. @@ -36,6 +37,10 @@ public class PackFile extends File { private final PackExt packExt; + private static String createName(String id, PackExt extension) { + return PREFIX + id + '.' + extension.getExtension(); + } + /** * Create a PackFile for a pack or related file. * @@ -46,6 +51,34 @@ public PackFile(File file) { this(file.getParentFile(), file.getName()); } + /** + * Create a PackFile for a pack or related file. + * + * @param directory + * Directory to create the PackFile in. + * @param id + * the {@link ObjectId} for this pack + * @param ext + * the packExt of the name. + */ + public PackFile(File directory, ObjectId id, PackExt ext) { + this(directory, id.name(), ext); + } + + /** + * Create a PackFile for a pack or related file. + * + * @param directory + * Directory to create the PackFile in. + * @param id + * the id (40 Hex char) section of the pack name. + * @param ext + * the packExt of the name. + */ + public PackFile(File directory, String id, PackExt ext) { + this(directory, createName(id, ext)); + } + /** * Create a PackFile for a pack or related file. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackInserter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackInserter.java index a27a2b00c..d6209c4a7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackInserter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackInserter.java @@ -76,6 +76,7 @@ import org.eclipse.jgit.errors.LargeObjectException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; @@ -273,16 +274,16 @@ public void flush() throws IOException { } Collections.sort(objectList); - File tmpIdx = idxFor(tmpPack); + File tmpIdx = idxFor(tmpPack); // TODO(nasserg) Use PackFile? writePackIndex(tmpIdx, packHash, objectList); - File realPack = new File(db.getPackDirectory(), - "pack-" + computeName(objectList).name() + ".pack"); //$NON-NLS-1$ //$NON-NLS-2$ + PackFile realPack = new PackFile(db.getPackDirectory(), + computeName(objectList), PackExt.PACK); db.closeAllPackHandles(realPack); tmpPack.setReadOnly(); FileUtils.rename(tmpPack, realPack, ATOMIC_MOVE); - File realIdx = idxFor(realPack); + PackFile realIdx = realPack.create(PackExt.INDEX); tmpIdx.setReadOnly(); try { FileUtils.rename(tmpIdx, realIdx, ATOMIC_MOVE); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java index f2eac8d24..03ef852c7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java @@ -13,6 +13,7 @@ import static org.eclipse.jgit.transport.WalkRemoteObjectDatabase.ROOT_DIR; import java.io.BufferedOutputStream; +import java.io.File; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; @@ -26,6 +27,8 @@ import org.eclipse.jgit.errors.TransportException; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.storage.file.PackFile; +import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.internal.storage.pack.PackWriter; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; @@ -189,9 +192,8 @@ public void close() { private void sendpack(final List updates, final ProgressMonitor monitor) throws TransportException { - String pathPack = null; - String pathIdx = null; - + PackFile pack = null; + PackFile idx = null; try (PackWriter writer = new PackWriter(transport.getPackConfig(), local.newObjectReader())) { @@ -217,31 +219,33 @@ private void sendpack(final List updates, for (String n : dest.getPackNames()) packNames.put(n, n); - final String base = "pack-" + writer.computeName().name(); //$NON-NLS-1$ - final String packName = base + ".pack"; //$NON-NLS-1$ - pathPack = "pack/" + packName; //$NON-NLS-1$ - pathIdx = "pack/" + base + ".idx"; //$NON-NLS-1$ //$NON-NLS-2$ + File packDir = new File("pack"); //$NON-NLS-1$ + pack = new PackFile(packDir, writer.computeName(), + PackExt.PACK); + idx = pack.create(PackExt.INDEX); - if (packNames.remove(packName) != null) { + if (packNames.remove(pack.getName()) != null) { // The remote already contains this pack. We should // remove the index before overwriting to prevent bad // offsets from appearing to clients. // dest.writeInfoPacks(packNames.keySet()); - dest.deleteFile(pathIdx); + dest.deleteFile(idx.getPath()); } // Write the pack file, then the index, as readers look the // other direction (index, then pack file). // - String wt = "Put " + base.substring(0, 12); //$NON-NLS-1$ + String wt = "Put " + pack.getName().substring(0, 12); //$NON-NLS-1$ try (OutputStream os = new BufferedOutputStream( - dest.writeFile(pathPack, monitor, wt + "..pack"))) { //$NON-NLS-1$ + dest.writeFile(pack.getPath(), monitor, + wt + "." + pack.getPackExt().getExtension()))) { //$NON-NLS-1$ writer.writePack(monitor, monitor, os); } try (OutputStream os = new BufferedOutputStream( - dest.writeFile(pathIdx, monitor, wt + "..idx"))) { //$NON-NLS-1$ + dest.writeFile(idx.getPath(), monitor, + wt + "." + idx.getPackExt().getExtension()))) { //$NON-NLS-1$ writer.writeIndex(os); } @@ -250,22 +254,22 @@ private void sendpack(final List updates, // and discover the most recent objects there. // final ArrayList infoPacks = new ArrayList<>(); - infoPacks.add(packName); + infoPacks.add(pack.getName()); infoPacks.addAll(packNames.keySet()); dest.writeInfoPacks(infoPacks); } catch (IOException err) { - safeDelete(pathIdx); - safeDelete(pathPack); + safeDelete(idx); + safeDelete(pack); throw new TransportException(uri, JGitText.get().cannotStoreObjects, err); } } - private void safeDelete(String path) { + private void safeDelete(File path) { if (path != null) { try { - dest.deleteFile(path); + dest.deleteFile(path.getPath()); } catch (IOException cleanupFailure) { // Ignore the deletion failure. We probably are // already failing and were just trying to pick