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 <quic_nasserg@quicinc.com>

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 24f7741f16..0232156a49 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 class TestRepository<R extends Repository> implements AutoCloseable {
 					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 @@ public class TestRepository<R extends Repository> implements AutoCloseable {
 		}
 	}
 
-	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 45d864d45d..bd36337f35 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.Collection;
 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 class AbbreviationTest extends LocalDiskRepositoryTestCase {
 			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 ca63507354..df5d952ee3 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 @@ public class ConcurrentRepackTest extends RepositoryTestCase {
 				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 @@ public class ConcurrentRepackTest extends RepositoryTestCase {
 		}
 	}
 
-	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 88d25b73d7..619cfcac31 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 static org.junit.Assert.assertTrue;
 
 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 class PackFileTest {
 					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 6e8584a9c8..e422ab9db3 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 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 			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 9366404ba4..9ffff9f662 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 @@ public class GC {
 			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 @@ public class GC {
 			}
 
 			// 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 @@ public class GC {
 				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 @@ public class GC {
 		}
 	}
 
-	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 ae5bce6985..f112947bae 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 java.util.List;
 
 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 @@ class LocalCachedPack extends CachedPack {
 
 	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 04d2ff8ab2..dba8ccd99b 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 java.util.zip.Deflater;
 
 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 @@ public class ObjectDirectoryPackParser extends PackParser {
 			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 c2e6f324d1..19979d0ed5 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 java.text.MessageFormat;
 
 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.
 	 *
@@ -51,6 +56,34 @@ public class PackFile extends File {
 	 *
 	 * @param directory
 	 *            Directory to create the PackFile in.
+	 * @param id
+	 *            the {@link ObjectId} for this pack
+	 * @param ext
+	 *            the <code>packExt</code> 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 <code>id</code> (40 Hex char) section of the pack name.
+	 * @param ext
+	 *            the <code>packExt</code> 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.
+	 *
+	 * @param directory
+	 *            Directory to create the PackFile in.
 	 * @param name
 	 *            Filename (last path section) of the PackFile
 	 */
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 a27a2b00c3..d6209c4a79 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.IncorrectObjectTypeException;
 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 class PackInserter extends ObjectInserter {
 		}
 
 		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 f2eac8d24a..03ef852c7f 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 @@ package org.eclipse.jgit.transport;
 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 java.util.TreeMap;
 
 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 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 
 	private void sendpack(final List<RemoteRefUpdate> 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 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			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 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			// and discover the most recent objects there.
 			//
 			final ArrayList<String> 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