Fix exception handling for opening bitmap index files

When creating a new PackFile instance it is specified whether this pack
has an associated bitmap index file or not. This information is cached
and the public method getBitmapIndex() will always assume a bitmap index
file must exist if the cached data tells so. But it may happen that the
packfiles are repacked during a gc in a different process causing the
packfile, bitmap-index and index file to be deleted. Since JGit still
has an open FileHandle on the packfile this file is not really deleted
and can still be accessed. But index and bitmap index file are deleted.
Fix getBitmapIndex() to invalidate the cached packfile instance if such
a situation occurs.

This problem showed up when a gerrit server was serving repositories
which where garbage collected with native git regularly. Fetch and
clone commands for certain repositories failed permanently after a
native git gc had deleted old bitmap index files.

Change-Id: I8e620bec74dd3f310ba42024f9a657062f868f0e
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 114 insertions, 2 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java
index 07a7be7467..776226b4ef 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcConcurrentTest.java
@@ -45,8 +45,12 @@ package org.eclipse.jgit.internal.storage.file;
 
 import static java.lang.Integer.valueOf;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CyclicBarrier;
@@ -56,8 +60,14 @@ import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.internal.storage.pack.PackWriter;
+import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.EmptyProgressMonitor;
+import org.eclipse.jgit.lib.NullProgressMonitor;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Sets;
 import org.eclipse.jgit.revwalk.RevBlob;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Test;
 
 public class GcConcurrentTest extends GcTestCase {
@@ -116,4 +126,97 @@ public class GcConcurrentTest extends GcTestCase {
 			pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
 		}
 	}
+
+	@Test
+	public void repackAndGetStats() throws Exception {
+		TestRepository<FileRepository>.BranchBuilder test = tr.branch("test");
+		test.commit().add("a", "a").create();
+		GC gc1 = new GC(tr.getRepository());
+		gc1.setPackExpireAgeMillis(0);
+		gc1.gc();
+		test.commit().add("b", "b").create();
+
+		// Create a new Repository instance and trigger a gc
+		// from that instance. Reusing the existing repo instance
+		// tr.getRepository() would not show the problem.
+		FileRepository r2 = new FileRepository(
+				tr.getRepository().getDirectory());
+		GC gc2 = new GC(r2);
+		gc2.setPackExpireAgeMillis(0);
+		gc2.gc();
+
+		new GC(tr.getRepository()).getStatistics();
+	}
+
+	@Test
+	public void repackAndUploadPack() throws Exception {
+		TestRepository<FileRepository>.BranchBuilder test = tr.branch("test");
+		// RevCommit a = test.commit().add("a", "a").create();
+		test.commit().add("a", "a").create();
+
+		GC gc1 = new GC(tr.getRepository());
+		gc1.setPackExpireAgeMillis(0);
+		gc1.gc();
+
+		RevCommit b = test.commit().add("b", "b").create();
+
+		FileRepository r2 = new FileRepository(
+				tr.getRepository().getDirectory());
+		GC gc2 = new GC(r2);
+		gc2.setPackExpireAgeMillis(0);
+		gc2.gc();
+
+		// Simulate parts of an UploadPack. This is the situation on
+		// server side (e.g. gerrit) when when clients are
+		// cloning/fetching while the server side repo's
+		// are gc'ed by an external process (e.g. scheduled
+		// native git gc)
+		try (PackWriter pw = new PackWriter(tr.getRepository())) {
+			pw.setUseBitmaps(true);
+			pw.preparePack(NullProgressMonitor.INSTANCE, Sets.of(b),
+					Collections.<ObjectId> emptySet());
+			new GC(tr.getRepository()).getStatistics();
+		}
+	}
+
+	PackFile getSinglePack(FileRepository r) {
+		Collection<PackFile> packs = r.getObjectDatabase().getPacks();
+		assertEquals(1, packs.size());
+		return packs.iterator().next();
+	}
+
+	@Test
+	public void repackAndCheckBitmapUsage() throws Exception {
+		// create a test repository with one commit and pack all objects. After
+		// packing create loose objects to trigger creation of a new packfile on
+		// the next gc
+		TestRepository<FileRepository>.BranchBuilder test = tr.branch("test");
+		test.commit().add("a", "a").create();
+		FileRepository repository = tr.getRepository();
+		GC gc1 = new GC(repository);
+		gc1.setPackExpireAgeMillis(0);
+		gc1.gc();
+		String oldPackName = getSinglePack(repository).getPackName();
+		RevCommit b = test.commit().add("b", "b").create();
+
+		// start the garbage collection on a new repository instance,
+		FileRepository repository2 = new FileRepository(repository.getDirectory());
+		GC gc2 = new GC(repository2);
+		gc2.setPackExpireAgeMillis(0);
+		gc2.gc();
+		String newPackName = getSinglePack(repository2).getPackName();
+		// make sure gc() has caused creation of a new packfile
+		assertNotEquals(oldPackName, newPackName);
+
+		// Even when asking again for the set of packfiles outdated data
+		// will be returned. As long as the repository can work on cached data
+		// it will do so and not detect that a new packfile exists.
+		assertNotEquals(getSinglePack(repository).getPackName(), newPackName);
+
+		// Only when accessing object content it is required to rescan the pack
+		// directory and the new packfile will be detected.
+		repository.getObjectDatabase().open(b).getSize();
+		assertEquals(getSinglePack(repository).getPackName(), newPackName);
+		assertNotNull(getSinglePack(repository).getBitmapIndex());
+	}
 }
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 038172f92d..b5889f2dd4 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
@@ -1105,8 +1105,17 @@ public class PackFile implements Iterable<PackIndex.MutableEntry> {
 		if (invalid || invalidBitmap)
 			return null;
 		if (bitmapIdx == null && hasExt(BITMAP_INDEX)) {
-			final PackBitmapIndex idx = PackBitmapIndex.open(
-					extFile(BITMAP_INDEX), idx(), getReverseIdx());
+			final PackBitmapIndex idx;
+			try {
+				idx = PackBitmapIndex.open(extFile(BITMAP_INDEX), 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;
+			}
 
 			// At this point, idx() will have set packChecksum.
 			if (Arrays.equals(packChecksum, idx.packChecksum))