FileHistoryCache#get() should not modify history cache (#3828)

* add test for get() vs. store()

* do not modify history cache in FileHistoryCache#get()

Author: Vladimir Kotal

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/Configuration.java

@@ -107,11 +107,6 @@ public final class Configuration {
      * Should the history log be cached?
      */
     private boolean historyCache;
-    /**
-     * The maximum time in milliseconds {@code HistoryCache.get()} can take
-     * before its result is cached.
-     */
-    private int historyCacheTime;
     /**
      * flag to generate history. This is bigger hammer than @{code historyCache}
      * above. If set to false, no history query will be ever made and the webapp
@@ -543,7 +538,6 @@ public Configuration() {
         setGroupsCollapseThreshold(4);
         setHandleHistoryOfRenamedFiles(false);
         setHistoryCache(true);
-        setHistoryCacheTime(30);
         setHistoryEnabled(true);
         setHitsPerPage(25);
         setIgnoredNames(new IgnoredNames());
@@ -807,28 +801,6 @@ public void setHistoryCache(boolean historyCache) {
         this.historyCache = historyCache;
     }
 
-    /**
-     * How long can a history request take before it's cached? If the time is
-     * exceeded, the result is cached. This setting only affects
-     * {@code FileHistoryCache}.
-     *
-     * @return the maximum time in milliseconds a history request can take
-     * before it's cached
-     */
-    public int getHistoryCacheTime() {
-        return historyCacheTime;
-    }
-
-    /**
-     * Set the maximum time a history request can take before it's cached. This
-     * setting is only respected if {@code FileHistoryCache} is used.
-     *
-     * @param historyCacheTime maximum time in milliseconds
-     */
-    public void setHistoryCacheTime(int historyCacheTime) {
-        this.historyCacheTime = historyCacheTime;
-    }
-
     public boolean isFetchHistoryWhenNotInCache() {
         return fetchHistoryWhenNotInCache;
     }

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/RuntimeEnvironment.java

@@ -656,25 +656,6 @@ public Set<String> getCtagsLanguages() {
         return Collections.unmodifiableSet(ctagsLanguages);
     }
 
-    /**
-     * Get the max time a SCM operation may use to avoid being cached.
-     *
-     * @return the maximum time in milliseconds
-     */
-    public int getHistoryReaderTimeLimit() {
-        return syncReadConfiguration(Configuration::getHistoryCacheTime);
-    }
-
-    /**
-     * Specify the maximum time a SCM operation should take before it will be
-     * cached (in ms).
-     *
-     * @param historyCacheTime the max time in ms before it is cached
-     */
-    public void setHistoryReaderTimeLimit(int historyCacheTime) {
-        syncWriteConfiguration(historyCacheTime, Configuration::setHistoryCacheTime);
-    }
-
     /**
      * Is history cache currently enabled?
      *

opengrok-indexer/src/main/java/org/opengrok/indexer/history/FileHistoryCache.java

@@ -177,6 +177,10 @@ public void initialize() {
         }
     }
 
+    double getFileHistoryCacheHits() {
+        return fileHistoryCacheHits.count();
+    }
+
     @Override
     public void optimize() {
         // nothing to do
@@ -669,39 +673,14 @@ public History get(File file, Repository repository, boolean withFiles)
         }
 
         final History history;
-        long time;
         try {
-            time = System.currentTimeMillis();
             history = repository.getHistory(file);
-            time = System.currentTimeMillis() - time;
         } catch (UnsupportedOperationException e) {
             // In this case, we've found a file for which the SCM has no history
             // An example is a non-SCCS file somewhere in an SCCS-controlled workspace.
             return null;
         }
 
-        // Don't cache history-information for directories, since the
-        // history information on the directory may change if a file in
-        // a sub-directory change. This will cause us to present a stale
-        // history log until the current directory is updated and
-        // invalidates the cache entry.
-        if (!file.isDirectory()) {
-            // Either the cache is stale or retrieving the history took too long, cache it!
-            if (cacheFile.exists()) {
-                if (LOGGER.isLoggable(Level.FINEST)) {
-                    LOGGER.log(Level.FINEST, "refreshing history for ''{0}'': {1}",
-                            new Object[]{file, history.getRevisionList()});
-                }
-                storeFile(history, file, repository);
-            } else if (time > env.getHistoryReaderTimeLimit()) {
-                if (LOGGER.isLoggable(Level.FINEST)) {
-                    LOGGER.log(Level.FINEST, "getting history for ''{0}'' took longer than {1} ms, caching it: {2}",
-                            new Object[]{file, env.getHistoryReaderTimeLimit(), history.getRevisionList()});
-                }
-                storeFile(history, file, repository);
-            }
-        }
-
         return history;
     }
 

opengrok-indexer/src/test/java/org/opengrok/indexer/configuration/ConfigMergeTest.java

@@ -31,9 +31,9 @@
  *
  * @author vkotal
  */
-public class ConfigMergeTest {
+class ConfigMergeTest {
     @Test
-    public void basicTest() throws Exception {
+    void basicTest() throws Exception {
 
         String srcRoot = "/foo";
         String dataRoot = "/bar";

opengrok-indexer/src/test/java/org/opengrok/indexer/configuration/RuntimeEnvironmentTest.java

@@ -215,14 +215,6 @@ public void testCtags() {
                 "instance ctags should equals 'ctags' or the sys property");
     }
 
-    @Test
-    public void testHistoryReaderTimeLimit() {
-        RuntimeEnvironment instance = RuntimeEnvironment.getInstance();
-        assertEquals(30, instance.getHistoryReaderTimeLimit());
-        instance.setHistoryReaderTimeLimit(50);
-        assertEquals(50, instance.getHistoryReaderTimeLimit());
-    }
-
     @Test
     public void testFetchHistoryWhenNotInCache() {
         RuntimeEnvironment instance = RuntimeEnvironment.getInstance();

opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java

@@ -26,6 +26,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -42,6 +43,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.nio.file.attribute.FileTime;
 import java.util.Date;
 import java.util.Iterator;
 import java.util.LinkedList;
@@ -63,7 +65,6 @@
 import org.opengrok.indexer.configuration.IgnoredNames;
 import org.opengrok.indexer.configuration.RuntimeEnvironment;
 import org.opengrok.indexer.util.IOUtils;
-import org.opengrok.indexer.util.TandemPath;
 import org.opengrok.indexer.util.TestRepository;
 
 /**
@@ -80,7 +81,6 @@ class FileHistoryCacheTest {
     private FileHistoryCache cache;
 
     private boolean savedFetchHistoryWhenNotInCache;
-    private int savedHistoryReaderTimeLimit;
     private boolean savedIsHandleHistoryOfRenamedFiles;
     private boolean savedIsTagsEnabled;
 
@@ -96,7 +96,6 @@ public void setUp() throws Exception {
         cache.initialize();
 
         savedFetchHistoryWhenNotInCache = env.isFetchHistoryWhenNotInCache();
-        savedHistoryReaderTimeLimit = env.getHistoryReaderTimeLimit();
         savedIsHandleHistoryOfRenamedFiles = env.isHandleHistoryOfRenamedFiles();
         savedIsTagsEnabled = env.isTagsEnabled();
     }
@@ -112,7 +111,6 @@ public void tearDown() {
         cache = null;
 
         env.setFetchHistoryWhenNotInCache(savedFetchHistoryWhenNotInCache);
-        env.setHistoryReaderTimeLimit(savedHistoryReaderTimeLimit);
         env.setIgnoredNames(new IgnoredNames());
         env.setIncludedNames(new Filter());
         env.setHandleHistoryOfRenamedFiles(savedIsHandleHistoryOfRenamedFiles);
@@ -156,6 +154,57 @@ private void assertSameEntry(HistoryEntry expected, HistoryEntry actual, boolean
         }
     }
 
+    /**
+     * {@link FileHistoryCache#get(File, Repository, boolean)} should not disturb history cache
+     * if run between repository update and reindex.
+     */
+    @EnabledOnOs({OS.LINUX, OS.MAC, OS.SOLARIS, OS.AIX, OS.OTHER})
+    @EnabledForRepository(MERCURIAL)
+    @Test
+    void testStoreTouchGet() throws Exception {
+        File reposRoot = new File(repositories.getSourceRoot(), "mercurial");
+        Repository repo = RepositoryFactory.getRepository(reposRoot);
+        History historyToStore = repo.getHistory(reposRoot);
+
+        cache.store(historyToStore, repo);
+
+        // This makes sure that the file which contains the latest revision has indeed been created.
+        assertEquals("9:8b340409b3a8", cache.getLatestCachedRevision(repo));
+
+        File file = new File(reposRoot, "main.c");
+        assertTrue(file.exists());
+        FileTime fileTimeBeforeImport = Files.getLastModifiedTime(file.toPath());
+        History historyBeforeImport = cache.get(file, repo, false);
+
+        MercurialRepositoryTest.runHgCommand(reposRoot, "import",
+                Paths.get(getClass().getResource("/history/hg-export.txt").toURI()).toString());
+        FileTime fileTimeAfterImport = Files.getLastModifiedTime(file.toPath());
+        assertTrue(fileTimeBeforeImport.compareTo(fileTimeAfterImport) < 0);
+
+        // This get() call basically mimics a request through the UI or API.
+        History historyAfterImport = cache.get(file, repo, false);
+        assertNotNull(historyAfterImport);
+        assertNotEquals(historyBeforeImport, historyAfterImport);
+
+        // Simulates reindex, or at least its first part when history cache is updated.
+        repo.createCache(cache, cache.getLatestCachedRevision(repo));
+
+        // This makes sure that the file which contains the latest revision has indeed been created.
+        assertEquals("11:bbb3ce75e1b8", cache.getLatestCachedRevision(repo));
+
+        /*
+         * The history should not be disturbed.
+         * Make sure that get() retrieved the history from cache. Mocking/spying static methods
+         * (FileHistoryCache#readCache() in this case) is tricky so use the cache hits metric.
+         */
+        double cacheHitsBeforeGet = cache.getFileHistoryCacheHits();
+        History historyAfterReindex = cache.get(file, repo, false);
+        double cacheHitsAfterGet = cache.getFileHistoryCacheHits();
+        assertNotNull(historyAfterReindex);
+        assertEquals(historyAfterImport, historyAfterReindex);
+        assertEquals(1, cacheHitsAfterGet - cacheHitsBeforeGet);
+    }
+
     /**
      * Basic tests for the {@code store()} method on cache with disabled
      * handling of renamed files.
@@ -838,8 +887,7 @@ void testRenamedFile() throws Exception {
                 updatedHistory.getHistoryEntries(), false);
     }
 
-    private void checkNoHistoryFetchRepo(String reponame, String filename,
-            boolean hasHistory, boolean historyFileExists) throws Exception {
+    private void checkNoHistoryFetchRepo(String reponame, String filename, boolean hasHistory) throws Exception {
 
         File reposRoot = new File(repositories.getSourceRoot(), reponame);
         Repository repo = RepositoryFactory.getRepository(reposRoot);
@@ -853,13 +901,6 @@ private void checkNoHistoryFetchRepo(String reponame, String filename,
         // in history cache.
         History retrievedHistory = cache.get(repoFile, repo, true);
         assertEquals(hasHistory, retrievedHistory != null);
-
-        // The file in history cache should not exist since
-        // FetchHistoryWhenNotInCache is set to false.
-        File dataRoot = new File(repositories.getDataRoot(),
-                "historycache" + File.separatorChar + reponame);
-        File fileHistory = new File(dataRoot, TandemPath.join(filename, ".gz"));
-        assertEquals(historyFileExists, fileHistory.exists());
     }
 
     /*
@@ -871,19 +912,13 @@ void testNoHistoryFetch() throws Exception {
         // Do not create history cache for files which do not have it cached.
         env.setFetchHistoryWhenNotInCache(false);
 
-        // Make cache.get() predictable. Normally when the retrieval of
-        // history of given file is faster than the limit, the history of this
-        // file is not stored. For the sake of this test we want the history
-        // to be always stored.
-        env.setHistoryReaderTimeLimit(0);
-
         // Pretend we are done with first phase of indexing.
         cache.setHistoryIndexDone();
 
         // First try repo with ability to fetch history for directories.
-        checkNoHistoryFetchRepo("mercurial", "main.c", false, false);
+        checkNoHistoryFetchRepo("mercurial", "main.c", false);
         // Second try repo which can fetch history of individual files only.
-        checkNoHistoryFetchRepo("teamware", "header.h", true, true);
+        checkNoHistoryFetchRepo("teamware", "header.h", true);
     }
 
     /**

opengrok-indexer/src/test/java/org/opengrok/indexer/index/IndexerTest.java

@@ -320,35 +320,39 @@ public void reset() {
     /**
      * Test that reindex after changing a file does not wipe out history index
      * for this file. This is important for the incremental history indexing.
-     * @throws Exception
      */
     @Test
     @EnabledForRepository(MERCURIAL)
     void testRemoveFileOnFileChange() throws Exception {
         RuntimeEnvironment env = RuntimeEnvironment.getInstance();
 
+        String path = "/mercurial/bar.txt";
+
         TestRepository testrepo = new TestRepository();
         testrepo.create(HistoryGuru.class.getResourceAsStream("repositories.zip"));
 
         env.setSourceRoot(testrepo.getSourceRoot());
         env.setDataRoot(testrepo.getDataRoot());
         env.setRepositories(testrepo.getSourceRoot());
 
+        // Create history cache.
+        Indexer.getInstance().prepareIndexer(env, true, true,
+                false, null, List.of("mercurial"));
+        File historyFile = new File(env.getDataRootPath(),
+                TandemPath.join("historycache" + path, ".gz"));
+        assertTrue(historyFile.exists(), String.format("history cache for %s has to exist", path));
+
         // create index
         Project project = new Project("mercurial", "/mercurial");
         IndexDatabase idb = new IndexDatabase(project);
         assertNotNull(idb);
-        String path = "/mercurial/bar.txt";
         RemoveIndexChangeListener listener = new RemoveIndexChangeListener(path);
         idb.addIndexChangedListener(listener);
         idb.update();
         assertEquals(5, listener.filesToAdd.size());
         listener.reset();
 
         // Change a file so that it gets picked up by the indexer.
-        File historyFile = new File(env.getDataRootPath(),
-                TandemPath.join("historycache" + path, ".gz"));
-        assertTrue(historyFile.exists(), String.format("history cache for %s has to exist", path));
         File bar = new File(testrepo.getSourceRoot() + File.separator + "mercurial", "bar.txt");
         assertTrue(bar.exists());
         try (BufferedWriter bw = new BufferedWriter(new FileWriter(bar, true))) {