do not modify history cache in FileHistoryCache#get()

Author: vladak

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

@@ -65,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;
 
 /**
@@ -157,8 +156,9 @@ private void assertSameEntry(HistoryEntry expected, HistoryEntry actual, boolean
 
     /**
      * {@link FileHistoryCache#get(File, Repository, boolean)} should not disturb history cache
-     * if run after repository update and reindex.
+     * 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 {
@@ -192,14 +192,17 @@ void testStoreTouchGet() throws Exception {
         // 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.
+        /*
+         * 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);
-        assertTrue(cacheHitsAfterGet - cacheHitsBeforeGet == 1);
+        assertEquals(1, cacheHitsAfterGet - cacheHitsBeforeGet);
     }
 
     /**
@@ -884,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);
@@ -899,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());
     }
 
     /*
@@ -921,9 +916,9 @@ void testNoHistoryFetch() throws Exception {
         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))) {