Merge "Gc#deleteOrphans: avoid dependence on PackExt alphabetical ordering"

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcOrphanFilesTest.java

@@ -10,6 +10,7 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static org.eclipse.jgit.internal.storage.pack.PackExt.REVERSE_INDEX;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -36,6 +37,14 @@ public class GcOrphanFilesTest extends GcTestCase {
 
 	private static final String PACK_File_3 = PACK + "-3.pack";
 
+	private static final String REVERSE_File_2 = PACK + "-2."
+			+ REVERSE_INDEX.getExtension();
+
+	private static final String REVERSE_File_4 = PACK + "-4."
+			+ REVERSE_INDEX.getExtension();
+
+	private static final String NONEXISTENT_EXT = PACK + "-4.xxxxx";
+
 	private File packDir;
 
 	@Override
@@ -46,14 +55,27 @@ public void setUp() throws Exception {
 	}
 
 	@Test
-	public void bitmapAndIdxDeletedButPackNot() throws Exception {
+	public void indexesDeletedButPackNot() throws Exception {
 		createFileInPackFolder(BITMAP_File_1);
 		createFileInPackFolder(IDX_File_2);
 		createFileInPackFolder(PACK_File_3);
+		createFileInPackFolder(REVERSE_File_4);
 		gc.gc().get();
 		assertFalse(new File(packDir, BITMAP_File_1).exists());
 		assertFalse(new File(packDir, IDX_File_2).exists());
 		assertTrue(new File(packDir, PACK_File_3).exists());
+		assertFalse(new File(packDir, REVERSE_File_4).exists());
+	}
+
+	@Test
+	public void noPacks_allIndexesDeleted() throws Exception {
+		createFileInPackFolder(BITMAP_File_1);
+		createFileInPackFolder(IDX_File_2);
+		createFileInPackFolder(REVERSE_File_4);
+		gc.gc().get();
+		assertFalse(new File(packDir, BITMAP_File_1).exists());
+		assertFalse(new File(packDir, IDX_File_2).exists());
+		assertFalse(new File(packDir, REVERSE_File_4).exists());
 	}
 
 	@Test
@@ -76,19 +98,28 @@ public void malformedIdxNotDeleted() throws Exception {
 		assertTrue(new File(packDir, IDX_File_malformed).exists());
 	}
 
+	@Test
+	public void nonexistantExtensionNotDeleted() throws Exception {
+		createFileInPackFolder(NONEXISTENT_EXT);
+		gc.gc().get();
+		assertTrue(new File(packDir, NONEXISTENT_EXT).exists());
+	}
+
 	@Test
 	public void keepPreventsDeletionOfIndexFilesForMissingPackFile()
 			throws Exception {
 		createFileInPackFolder(BITMAP_File_1);
-		createFileInPackFolder(IDX_File_2);
 		createFileInPackFolder(BITMAP_File_2);
+		createFileInPackFolder(IDX_File_2);
 		createFileInPackFolder(KEEP_File_2);
+		createFileInPackFolder(REVERSE_File_2);
 		createFileInPackFolder(PACK_File_3);
 		gc.gc().get();
 		assertFalse(new File(packDir, BITMAP_File_1).exists());
 		assertTrue(new File(packDir, BITMAP_File_2).exists());
 		assertTrue(new File(packDir, IDX_File_2).exists());
 		assertTrue(new File(packDir, KEEP_File_2).exists());
+		assertTrue(new File(packDir, REVERSE_File_2).exists());
 		assertTrue(new File(packDir, PACK_File_3).exists());
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java

@@ -14,6 +14,7 @@
 import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.KEEP;
 import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK;
+import static org.eclipse.jgit.internal.storage.pack.PackExt.REVERSE_INDEX;
 
 import java.io.File;
 import java.io.FileOutputStream;
@@ -44,6 +45,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.CompletableFuture;
@@ -110,14 +112,10 @@ public class GC {
 	private static final Pattern PATTERN_LOOSE_OBJECT = Pattern
 			.compile("[0-9a-fA-F]{38}"); //$NON-NLS-1$
 
-	private static final String PACK_EXT = "." + PackExt.PACK.getExtension();//$NON-NLS-1$
+	private static final Set<PackExt> PARENT_EXTS = Set.of(PACK, KEEP);
 
-	private static final String BITMAP_EXT = "." //$NON-NLS-1$
+	private static final Set<PackExt> CHILD_EXTS = Set.of(BITMAP_INDEX, INDEX,
-			+ PackExt.BITMAP_INDEX.getExtension();
+			REVERSE_INDEX);
-
-	private static final String INDEX_EXT = "." + PackExt.INDEX.getExtension(); //$NON-NLS-1$
-
-	private static final String KEEP_EXT = "." + PackExt.KEEP.getExtension(); //$NON-NLS-1$
 
 	private static final int DEFAULT_AUTOPACKLIMIT = 50;
 
@@ -937,47 +935,52 @@ private void delete(Path d) {
 		}
 	}
 
+	private static Optional<PackFile> toPackFileWithValidExt(
+			Path packFilePath) {
+		try {
+			PackFile packFile = new PackFile(packFilePath.toFile());
+			if (packFile.getPackExt() == null) {
+				return Optional.empty();
+			}
+			return Optional.of(packFile);
+		} catch (IllegalArgumentException e) {
+			return Optional.empty();
+		}
+	}
+
 	/**
 	 * Deletes orphans
 	 * <p>
-	 * A file is considered an orphan if it is either a "bitmap" or an index
+	 * A file is considered an orphan if it is some type of index file, but
-	 * file, and its corresponding pack file is missing in the list.
+	 * there is not a corresponding pack or keep file present in the directory.
 	 * </p>
 	 */
 	private void deleteOrphans() {
 		Path packDir = repo.getObjectDatabase().getPackDirectory().toPath();
-		List<String> fileNames = null;
+		List<PackFile> childFiles;
+		Set<String> seenParentIds = new HashSet<>();
 		try (Stream<Path> files = Files.list(packDir)) {
-			fileNames = files.map(path -> path.getFileName().toString())
+			childFiles = files.map(GC::toPackFileWithValidExt)
-					.filter(name -> (name.endsWith(PACK_EXT)
+					.filter(Optional::isPresent).map(Optional::get)
-							|| name.endsWith(BITMAP_EXT)
+					.filter(packFile -> {
-							|| name.endsWith(INDEX_EXT)
+						PackExt ext = packFile.getPackExt();
-							|| name.endsWith(KEEP_EXT)))
+						if (PARENT_EXTS.contains(ext)) {
-					// sort files with same base name in the order:
+							seenParentIds.add(packFile.getId());
-					// .pack, .keep, .index, .bitmap to avoid look ahead
+							return false;
-					.sorted(Collections.reverseOrder())
+						}
-					.collect(Collectors.toList());
+						return CHILD_EXTS.contains(ext);
+					}).collect(Collectors.toList());
 		} catch (IOException e) {
 			LOG.error(e.getMessage(), e);
 			return;
 		}
-		if (fileNames == null) {
-			return;
-		}
 
-		String latestId = null;
+		for (PackFile child : childFiles) {
-		for (String n : fileNames) {
+			if (!seenParentIds.contains(child.getId())) {
-			PackFile pf = new PackFile(packDir.toFile(), n);
-			PackExt ext = pf.getPackExt();
-			if (ext.equals(PACK) || ext.equals(KEEP)) {
-				latestId = pf.getId();
-			}
-			if (latestId == null || !pf.getId().equals(latestId)) {
-				// no pack or keep for this id
 				try {
-					FileUtils.delete(pf,
+					FileUtils.delete(child,
 							FileUtils.RETRY | FileUtils.SKIP_MISSING);
-					LOG.warn(JGitText.get().deletedOrphanInPackDir, pf);
+					LOG.warn(JGitText.get().deletedOrphanInPackDir, child);
 				} catch (IOException e) {
 					LOG.error(e.getMessage(), e);
 				}