Merge "Don't delete .idx file if .pack file can't be deleted"

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

@@ -49,6 +49,7 @@ import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
+import java.io.FileInputStream;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
@@ -66,6 +67,7 @@ import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.file.GC.RepoStatistics;
 import org.eclipse.jgit.internal.storage.file.PackIndex.MutableEntry;
+import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.junit.TestRepository;
@@ -620,6 +622,40 @@ public class GCTest extends LocalDiskRepositoryTestCase {
 		assertEquals(2, stats.numberOfPackFiles);
 	}
 
+	@Test
+	public void testPruneOldPacksWithOpenHandleOnPack() throws Exception {
+		gc.setExpireAgeMillis(0);
+
+		BranchBuilder bb = tr.branch("refs/heads/master");
+		bb.commit().add("A", "A").add("B", "B").create();
+		fsTick();
+		gc.gc();
+
+		Collection<PackFile> packs = repo.getObjectDatabase().getPacks();
+		assertEquals(1, packs.size());
+		PackFile pack = packs.iterator().next();
+		File packFile = pack.getPackFile();
+		File indexFile = new File(packFile.getParentFile(), "pack-"
+				+ pack.getPackName()
+				+ "."
+				+ PackExt.INDEX.getExtension());
+		FileInputStream fis = new FileInputStream(packFile);
+		try {
+			bb.commit().add("A", "A2").add("B", "B2").create();
+			fsTick();
+			gc.gc();
+			if (packFile.exists()) {
+				assertTrue(
+						"The pack was present but the index file was missing.",
+						indexFile.exists());
+			}
+
+		} finally {
+			fis.close();
+		}
+
+	}
+
 	@Test
 	public void testPackCommitsAndLooseOneWithPruneNowNoReflog()
 			throws Exception {

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

@@ -175,21 +175,9 @@ public class GC {
 	 *
 	 * @param oldPacks
 	 * @param newPacks
-	 * @param ignoreErrors
-	 *            <code>true</code> if we should ignore the fact that a certain
-	 *            pack files or index files couldn't be deleted.
-	 *            <code>false</code> if an exception should be thrown in such
-	 *            cases
-	 * @throws IOException
-	 *             if a pack file couldn't be deleted and
-	 *             <code>ignoreErrors</code> is set to <code>false</code>
 	 */
 	private void deleteOldPacks(Collection<PackFile> oldPacks,
-			Collection<PackFile> newPacks, boolean ignoreErrors)
+			Collection<PackFile> newPacks) {
-			throws IOException {
-		int deleteOptions = FileUtils.RETRY | FileUtils.SKIP_MISSING;
-		if (ignoreErrors)
-			deleteOptions |= FileUtils.IGNORE_ERRORS;
 		oldPackLoop: for (PackFile oldPack : oldPacks) {
 			String oldName = oldPack.getPackName();
 			// check whether an old pack file is also among the list of new
@@ -200,10 +188,7 @@ public class GC {
 
 			if (!oldPack.shouldBeKept()) {
 				oldPack.close();
-				for (PackExt ext : PackExt.values()) {
+				prunePack(oldName);
-					File f = nameFor(oldName, "." + ext.getExtension()); //$NON-NLS-1$
-					FileUtils.delete(f, deleteOptions);
-				}
 			}
 		}
 		// close the complete object database. Thats my only chance to force
@@ -211,6 +196,42 @@ public class GC {
 		repo.getObjectDatabase().close();
 	}
 
+	/**
+	 * Delete files associated with a single pack file. First try to delete the
+	 * ".pack" file because on some platforms the ".pack" file may be locked and
+	 * can't be deleted. In such a case it is better to detect this early and
+	 * give up on deleting files for this packfile. Otherwise we may delete the
+	 * ".index" file and when failing to delete the ".pack" file we are left
+	 * with a ".pack" file without a ".index" file.
+	 *
+	 * @param packName
+	 */
+	private void prunePack(String packName) {
+		PackExt[] extensions = PackExt.values();
+		try {
+			// Delete the .pack file first and if this fails give up on deleting
+			// the other files
+			int deleteOptions = FileUtils.RETRY | FileUtils.SKIP_MISSING;
+			for (PackExt ext : extensions)
+				if (PackExt.PACK.equals(ext)) {
+					File f = nameFor(packName, "." + ext.getExtension()); //$NON-NLS-1$
+					FileUtils.delete(f, deleteOptions);
+					break;
+				}
+			// The .pack file has been deleted. Delete as many as the other
+			// files as you can.
+			deleteOptions |= FileUtils.IGNORE_ERRORS;
+			for (PackExt ext : extensions) {
+				if (!PackExt.PACK.equals(ext)) {
+					File f = nameFor(packName, "." + ext.getExtension()); //$NON-NLS-1$
+					FileUtils.delete(f, deleteOptions);
+				}
+			}
+		} catch (IOException e) {
+			// Deletion of the .pack file failed. Silently return.
+		}
+	}
+
 	/**
 	 * Like "git prune-packed" this method tries to prune all loose objects
 	 * which can be found in packs. If certain objects can't be pruned (e.g.
@@ -533,7 +554,7 @@ public class GC {
 			if (rest != null)
 				ret.add(rest);
 		}
-		deleteOldPacks(toBeDeleted, ret, true);
+		deleteOldPacks(toBeDeleted, ret);
 		prunePacked();
 
 		lastPackedRefs = refsBefore;