Fix pack files scan when filesnapshot isn't modified

Do not reload packfiles when their associated filesnapshot is not
modified on disk compared to the one currently stored in memory.

Fix the regression introduced by fef78212 which, in conjunction with
core.trustfolderstats = false, caused any lookup of objects inside
the packlist to loop forever when the object was not found in the pack
list.

Bug: 546190
Change-Id: I38d752ebe47cefc3299740aeba319a2641f19391
Signed-off-by: Luca Milanesio <luca.milanesio@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 54 insertions, 6 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java
index 923f283894..e9b3d2bc70 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java
@@ -42,6 +42,10 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.concurrent.Callable;
@@ -49,14 +53,29 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
-import org.eclipse.jgit.internal.storage.file.ObjectDirectory;
 import org.eclipse.jgit.junit.RepositoryTestCase;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
 
+@RunWith(Parameterized.class)
 public class ObjectDirectoryTest extends RepositoryTestCase {
 
+	@Parameter
+	public Boolean trustFolderStats;
+
+	@Parameters(name= "core.trustfolderstat={0}")
+	public static Iterable<? extends Object> data() {
+		return Arrays.asList(Boolean.TRUE, Boolean.FALSE);
+	}
+
 	@Test
 	public void testConcurrentInsertionOfBlobsToTheSameNewFanOutDirectory()
 			throws Exception {
@@ -69,6 +88,34 @@ public class ObjectDirectoryTest extends RepositoryTestCase {
 		}
 	}
 
+	@Test
+	public void testShouldNotSearchPacksAgainTheSecondTime() throws Exception {
+		FileRepository bareRepository = newTestRepositoryWithOnePackfile();
+		ObjectDirectory dir = bareRepository.getObjectDatabase();
+
+		// Make sure that timestamps are modified and read so that a full
+		// file snapshot check is performed
+		Thread.sleep(3000L);
+
+		assertTrue(dir.searchPacksAgain(dir.packList.get()));
+		assertFalse(dir.searchPacksAgain(dir.packList.get()));
+	}
+
+	private FileRepository newTestRepositoryWithOnePackfile() throws Exception {
+		FileRepository repository = createBareRepository();
+		TestRepository<FileRepository> testRepository = new TestRepository<FileRepository>(repository);
+		testRepository.commit();
+		testRepository.packAndPrune();
+
+		FileBasedConfig repoConfig = repository.getConfig();
+		repoConfig.setBoolean(ConfigConstants.CONFIG_CORE_SECTION,null,
+				ConfigConstants.CONFIG_KEY_TRUSTFOLDERSTAT,
+				trustFolderStats.booleanValue());
+		repoConfig.save();
+
+		return repository;
+	}
+
 	private Collection<Callable<ObjectId>> blobInsertersForTheSameFanOutDir(
 			final ObjectDirectory dir) {
 		Callable<ObjectId> callable = new Callable<ObjectId>() {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
index 44ad99bb9a..7c7a39ecc4 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
@@ -127,8 +127,6 @@ public class ObjectDirectory extends FileObjectDatabase {
 
 	private final File alternatesFile;
 
-	private final AtomicReference<PackList> packList;
-
 	private final FS fs;
 
 	private final AtomicReference<AlternateHandle[]> alternates;
@@ -141,6 +139,8 @@ public class ObjectDirectory extends FileObjectDatabase {
 
 	private Set<ObjectId> shallowCommitsIds;
 
+	final AtomicReference<PackList> packList;
+
 	/**
 	 * Initialize a reference to an on-disk object directory.
 	 *
@@ -674,7 +674,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 		return InsertLooseObjectResult.FAILURE;
 	}
 
-	private boolean searchPacksAgain(PackList old) {
+	boolean searchPacksAgain(PackList old) {
 		// Whether to trust the pack folder's modification time. If set
 		// to false we will always scan the .git/objects/pack folder to
 		// check for new pack files. If set to true (default) we use the
@@ -822,7 +822,8 @@ public class ObjectDirectory extends FileObjectDatabase {
 			final String packName = base + PACK.getExtension();
 			final File packFile = new File(packDirectory, packName);
 			final PackFile oldPack = forReuse.remove(packName);
-			if (oldPack != null && oldPack.getFileSnapshot().isModified(packFile)) {
+			if (oldPack != null
+					&& !oldPack.getFileSnapshot().isModified(packFile)) {
 				list.add(oldPack);
 				continue;
 			}
@@ -960,7 +961,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 		return new File(new File(getDirectory(), d), f);
 	}
 
-	private static final class PackList {
+	static final class PackList {
 		/** State just before reading the pack directory. */
 		final FileSnapshot snapshot;