use the all-or-nothing semantics for filling directory entries (#4324)

Author: vladak

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

@@ -759,11 +759,13 @@ private String getCachedRevision(Repository repository, String revPath) {
     }
 
     @Override
-    public void fillLastHistoryEntries(List<DirectoryEntry> entries) {
+    public boolean fillLastHistoryEntries(List<DirectoryEntry> entries) {
         if (entries == null) {
-            return;
+            return false;
         }
 
+        boolean ret = true;
+
         for (DirectoryEntry directoryEntry : entries) {
             try {
                 File file = directoryEntry.getFile();
@@ -780,11 +782,23 @@ public void fillLastHistoryEntries(List<DirectoryEntry> entries) {
                 } else {
                     LOGGER.log(Level.FINE, "cannot get last history entry for ''{0}''",
                             directoryEntry.getFile());
+                    ret = false;
+                    break;
                 }
             } catch (CacheException e) {
                 LOGGER.log(Level.FINER, "cannot get last history entry for ''{0}''", directoryEntry.getFile());
+                ret = false;
+                break;
             }
         }
+
+        // Enforce the all-or-nothing semantics.
+        if (!ret) {
+            entries.forEach(e -> e.setDate(null));
+            entries.forEach(e -> e.setDescription(null));
+        }
+
+        return ret;
     }
 
     @Override

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

@@ -91,13 +91,14 @@ interface HistoryCache extends Cache {
     @Nullable String getLatestCachedRevision(Repository repository) throws CacheException;
 
     /**
-     * Get the last modified times for all files and subdirectories in the
-     * specified directory.
+     * Get the last modified times and descriptions for all files and subdirectories in the specified directory.
+     * If any of the entries cannot be filled, these properties will be reset for all entries.
      *
      * @param entries list of {@link DirectoryEntry} instances
+     * @return whether all directory entries were filled
      * @throws CacheException on error
      */
-    void fillLastHistoryEntries(List<DirectoryEntry> entries) throws CacheException;
+    boolean fillLastHistoryEntries(List<DirectoryEntry> entries) throws CacheException;
 
     /**
      * Clear entry for single file from history cache.

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

@@ -750,9 +750,7 @@ public boolean fillLastHistoryEntries(File directory, List<DirectoryEntry> entri
             return true;
         }
 
-        historyCache.fillLastHistoryEntries(entries);
-
-        return false;
+        return !historyCache.fillLastHistoryEntries(entries);
     }
 
     /**

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

@@ -43,14 +43,14 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.attribute.FileTime;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.apache.commons.lang3.time.DateUtils;
 import org.eclipse.jgit.api.Git;
@@ -1013,9 +1013,12 @@ void testGetLastHistoryEntry() throws Exception {
      * getting history cache entries for directories.
      */
     @Test
-    void testGetLastHistoryEntries() throws Exception {
+    void testFillLastHistoryEntries() throws Exception {
         File repositoryRoot = new File(repositories.getSourceRoot(), "git");
         Repository repository = RepositoryFactory.getRepository(repositoryRoot);
+
+        // Create non-empty directory without repository involvement. This will be used to check
+        // that fillLastHistoryEntries() does not attempt to get history entry for it.
         File subDir = new File(repositoryRoot, "subdir");
         assertTrue(subDir.mkdir());
         File subFile = new File(subDir, "subfile.txt");
@@ -1031,14 +1034,51 @@ void testGetLastHistoryEntries() throws Exception {
         assertNotNull(files);
         assertTrue(files.length > 0);
         assertTrue(Arrays.stream(files).anyMatch(File::isDirectory));
-        List<DirectoryEntry> directoryEntries = new ArrayList<>();
-        for (File file : files) {
-            directoryEntries.add(new DirectoryEntry(file));
-        }
+        List<DirectoryEntry> directoryEntries = Arrays.stream(files).map(DirectoryEntry::new).
+                collect(Collectors.toList());
 
-        spyCache.fillLastHistoryEntries(directoryEntries);
+        assertTrue(spyCache.fillLastHistoryEntries(directoryEntries));
         Mockito.verify(spyCache, never()).getLastHistoryEntry(ArgumentMatchers.eq(subDir));
 
+        assertEquals(directoryEntries.size() - 3,
+                (int) directoryEntries.stream().filter(e -> e.getDate() != null).count());
+        assertEquals(directoryEntries.size(),
+                (int) directoryEntries.stream().filter(e -> e.getDescription() != null).count());
+
+        // Cleanup.
+        cache.clear(repository);
+    }
+
+    /**
+     * Test {@link FileHistoryCache#fillLastHistoryEntries(List)}, in particular that it
+     * returns {@code false} and resets date/descriptions if some entries cannot be filled.
+     */
+    @Test
+    void testFillLastHistoryEntriesAllOrNothing() throws Exception {
+        File repositoryRoot = new File(repositories.getSourceRoot(), "git");
+        Repository repository = RepositoryFactory.getRepository(repositoryRoot);
+
+        // This file will be created without any repository involvement, therefore it will not be possible
+        // to get history entry for it. This should make fillLastHistoryEntries() to return false.
+        File subFile = new File(repositoryRoot, "file.txt");
+        assertFalse(subFile.exists());
+        assertTrue(subFile.createNewFile());
+
+        FileHistoryCache spyCache = Mockito.spy(cache);
+        spyCache.clear(repository);
+        History historyToStore = repository.getHistory(repositoryRoot);
+        spyCache.store(historyToStore, repository);
+
+        File[] files = repositoryRoot.listFiles();
+        assertNotNull(files);
+        assertTrue(files.length > 0);
+        List<DirectoryEntry> directoryEntries = Arrays.stream(files).map(DirectoryEntry::new).
+                collect(Collectors.toList());
+
+        assertFalse(spyCache.fillLastHistoryEntries(directoryEntries));
+        assertEquals(0, (int) directoryEntries.stream().filter(e -> e.getDate() != null).count());
+        assertEquals(0, (int) directoryEntries.stream().filter(e -> e.getDescription() != null).count());
+
         // Cleanup.
         cache.clear(repository);
     }