Fix svn history for files which were in a directory when the directory was renamed

harrisric · vladak · commit 279e66942596 · 2023-02-27T13:19:24.000+01:00
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionHistoryParser.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionHistoryParser.java
@@ -32,8 +32,11 @@
 import java.nio.charset.StandardCharsets;
 import java.text.ParseException;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -64,6 +67,8 @@ class SubversionHistoryParser implements Executor.StreamHandler {
 
     private static class Handler extends DefaultHandler2 {
 
+        private static final String COPYFROM_PATH = "copyfrom-path";
+
         /**
          * Example of the longest date format that we should accept - SimpleDateFormat cannot cope with micro/nano seconds.
          */
@@ -74,10 +79,12 @@ private static class Handler extends DefaultHandler2 {
         final int length;
         final List<HistoryEntry> entries = new ArrayList<>();
         final Set<String> renamedFiles = new HashSet<>();
+        final Map<String, String> renamedToDirectoryRevisions = new HashMap<>();
         final SubversionRepository repository;
         HistoryEntry entry;
         StringBuilder sb;
-        boolean isRenamed;
+        boolean isRenamedFile;
+        boolean isRenamedDir;
 
         Handler(String home, String prefix, int length, SubversionRepository repository) {
             this.home = home;
@@ -87,19 +94,31 @@ private static class Handler extends DefaultHandler2 {
             sb = new StringBuilder();
         }
 
-        List<String> getRenamedFiles() {
-            return new ArrayList<>(renamedFiles);
+        Set<String> getRenamedFiles() {
+            return renamedFiles;
+        }
+
+        Map<String, String> getRenamedDirectories() {
+          return renamedToDirectoryRevisions;
         }
 
         @Override
         public void startElement(String uri, String localName, String qname, Attributes attr) {
-            isRenamed = false;
+            isRenamedFile = false;
+            isRenamedDir = false;
             if ("logentry".equals(qname)) {
                 entry = new HistoryEntry();
                 entry.setActive(true);
                 entry.setRevision(attr.getValue("revision"));
             } else if ("path".equals(qname)) {
-                isRenamed = attr.getIndex("copyfrom-path") != -1;
+                if (attr.getIndex(COPYFROM_PATH) != -1) {
+                  LOGGER.info("rename for:" + attr.getValue(COPYFROM_PATH) );
+                  if ("dir".equals(attr.getValue("kind"))) {
+                    isRenamedDir = true;
+                  } else {
+                    isRenamedFile = true;
+                  }
+                }
             }
             sb.setLength(0);
         }
@@ -132,9 +151,12 @@ public void endElement(String uri, String localName, String qname) throws SAXExc
                     // The same file names may be repeated in many commits,
                     // so intern them to reduce the memory footprint.
                     entry.addFile(path.intern());
-                    if (isRenamed) {
+                    if (isRenamedFile) {
                         renamedFiles.add(path.intern());
                     }
+                    if (isRenamedDir) {
+                      renamedToDirectoryRevisions.put(path.intern(), entry.getRevision());
+                    }
                 } else {
                     LOGGER.log(Level.FINER, "Skipping file outside repository: " + s);
                 }
@@ -209,10 +231,26 @@ History parse(File file, SubversionRepository repos, String sinceRevision,
             repos.removeAndVerifyOldestChangeset(entries, sinceRevision);
         }
 
-        return new History(entries, handler.getRenamedFiles());
+        Set<String> allRenamedFiles = findRenamedFilesFromDirectories(handler.getRenamedDirectories(), repos, cmdType);
+        allRenamedFiles.addAll(handler.getRenamedFiles());
+
+        return new History(entries, new ArrayList<>(allRenamedFiles));
     }
 
-   /**
+    /**
+     * @return a set of files that were present in the directories at the given revisions.
+     */
+    private Set<String> findRenamedFilesFromDirectories(Map<String, String> renamedDirectories, SubversionRepository repos,
+       CommandTimeoutType cmdType) {
+
+      Set<String> renamedFiles = new HashSet<>();
+      for (Entry<String, String> entry : renamedDirectories.entrySet()) {
+        renamedFiles.addAll(repos.getFilesInDirectoryAtRevision(entry.getKey(), entry.getValue(), cmdType));
+      }
+      return renamedFiles;
+    }
+
+    /**
      * Process the output from the log command and insert the HistoryEntries
      * into the history field.
      *
diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionRepository.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionRepository.java
@@ -23,14 +23,21 @@
  */
 package org.opengrok.indexer.history;
 
+import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
 import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+
 import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -41,6 +48,7 @@
 import org.opengrok.indexer.configuration.RuntimeEnvironment;
 import org.opengrok.indexer.logger.LoggerFactory;
 import org.opengrok.indexer.util.Executor;
+import org.opengrok.indexer.util.Executor.StreamHandler;
 import org.opengrok.indexer.util.LazilyInstantiate;
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
@@ -249,7 +257,7 @@ boolean getHistoryGet(OutputStream out, String parent, String basename, String r
 
         String filepath;
         try {
-            filepath = (new File(parent, basename)).getCanonicalPath();
+            filepath = new File(parent, basename).getCanonicalPath();
         } catch (IOException exp) {
             LOGGER.log(Level.SEVERE,
                     "Failed to get canonical path: {0}", exp.getClass().toString());
@@ -282,6 +290,73 @@ boolean getHistoryGet(OutputStream out, String parent, String basename, String r
         return false;
     }
 
+    /**
+     * Get an executor to be used for retrieving the history log for the named
+     * file.
+     *
+     * @param dir The repository relative directory to retrieve file list for
+     * @param atRevision the revision number at which we want the directory contents
+     * @param cmdType command timeout type
+     * @return An Executor ready to be started
+     */
+    private Executor getDirectoryListExecutor(final String dir, String atRevision, CommandTimeoutType cmdType) {
+
+        List<String> cmd = new ArrayList<>();
+        ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK);
+        cmd.add(RepoCommand);
+        cmd.add("ls");
+        cmd.add(escapeFileName(dir));
+        cmd.add("-r" + atRevision);
+
+        return new Executor(cmd, new File(getDirectoryName()),
+                    RuntimeEnvironment.getInstance().getCommandTimeout(cmdType));
+    }
+
+
+    /**
+     * Provides a list of files that were in a directory at a given revision.
+     * This is useful for finding files that will need special renamed file history
+     * handling because they were in a directory when it was renamed.
+     *
+     * Note that this doesn't throw an exception even if the command was not completed
+     * because we will still be able to get the file history up to this point.
+     *
+     * @param directory the directory to check
+     * @param revision the revision to check at
+     * @param cmdType the timeout setting.
+     * @return the files that were in the directory at that revision
+     */
+    Set<String> getFilesInDirectoryAtRevision(String directory, String revision,
+        CommandTimeoutType cmdType) {
+
+      Executor executor = getDirectoryListExecutor(
+            RuntimeEnvironment.getInstance().getSourceRootPath() + File.separator + directory,
+            revision, cmdType);
+
+      Set<String> files = new HashSet<>();
+
+      StreamHandler directoryLogStreamHandler = new StreamHandler() {
+
+       @Override
+       public void processStream(InputStream in) throws IOException {
+         new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))
+             .lines()
+             .filter(s -> !s.isBlank())
+             .map(s -> directory + File.separator + s)
+             .forEach(files::add);
+       }
+      };
+
+      int status = executor.exec(true, directoryLogStreamHandler);
+      if (status != 0) {
+        LOGGER.warning("Failed to get history for: \"" + directory + "\" Exit code: " + status);
+      }
+      LOGGER.info("log from directory / revision [" + directory + "] [" + revision + "]\n" + files);
+
+      return files;
+    }
+
+
     @Override
     boolean hasHistoryForDirectories() {
         return true;
diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/FileHistoryCacheTest.java
@@ -48,6 +48,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.apache.commons.lang3.time.DateUtils;
 import org.eclipse.jgit.api.Git;
@@ -60,6 +61,7 @@
 import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mockito;
 import org.opengrok.indexer.condition.EnabledForRepository;
+import org.opengrok.indexer.configuration.CommandTimeoutType;
 import org.opengrok.indexer.configuration.Filter;
 import org.opengrok.indexer.configuration.IgnoredNames;
 import org.opengrok.indexer.configuration.RuntimeEnvironment;
@@ -73,6 +75,8 @@
  */
 class FileHistoryCacheTest {
 
+    private static final String SUBVERSION_REPO_LOC = "subversion";
+
     private static final String SVN_DATE_FORMAT = "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'";
 
     private final RuntimeEnvironment env = RuntimeEnvironment.getInstance();
@@ -633,7 +637,7 @@ void testRenamedFilePlusChangesBranched() throws Exception {
     void testMultipleRenamedFiles() throws Exception {
         createSvnRepository();
 
-        File reposRoot = new File(repositories.getSourceRoot(), "subversion");
+        File reposRoot = new File(repositories.getSourceRoot(), SUBVERSION_REPO_LOC);
         History updatedHistory;
 
         // The test expects support for renamed files.
@@ -645,7 +649,7 @@ void testMultipleRenamedFiles() throws Exception {
         cache.store(historyToStore, repo);
 
         // Check complete list of history entries for the renamed file.
-        File testFile = new File(reposRoot.toString() + File.separatorChar + "FileZ.txt");
+        File testFile = new File(reposRoot.toString(), "FileZ.txt");
         updatedHistory = cache.get(testFile, repo, false);
         assertEquals(3, updatedHistory.getHistoryEntries().size());
 
@@ -677,6 +681,82 @@ void testMultipleRenamedFiles() throws Exception {
         assertSameEntries(histConstruct.getHistoryEntries(), updatedHistory.getHistoryEntries(), false);
     }
 
+
+    /**
+     * For an Subversion repository, verify we can get a list of files that were in a directory
+     * at the point it was renamed.
+     */
+    @EnabledForRepository(SUBVERSION)
+    @Test
+    void testSubversionFilesInDirectoryLog() throws Exception {
+        createSvnRepository();
+
+        File reposRoot = new File(repositories.getSourceRoot(), SUBVERSION_REPO_LOC);
+
+        // The test expects support for renamed files.
+        env.setHandleHistoryOfRenamedFiles(true);
+
+        // Generate history index.
+        SubversionRepository svnRepo = (SubversionRepository) RepositoryFactory.getRepository(reposRoot);
+        Set<String> files = svnRepo.getFilesInDirectoryAtRevision(
+          Path.of(SUBVERSION_REPO_LOC, "renamedFolder").toString(), "12", CommandTimeoutType.INDEXER);
+        assertEquals(
+          Set.of(Path.of(SUBVERSION_REPO_LOC, "renamedFolder", "FileInRenamedFolder.txt").toString()), files);
+    }
+
+
+    /**
+     * Make sure produces correct history for a file which was in a directory that was renamed.
+     */
+    @EnabledForRepository(SUBVERSION)
+    @Test
+    void testRenamedDirectory() throws Exception {
+        createSvnRepository();
+
+        File reposRoot = new File(repositories.getSourceRoot(), SUBVERSION_REPO_LOC);
+        History updatedHistory;
+
+        // The test expects support for renamed files.
+        env.setHandleHistoryOfRenamedFiles(true);
+
+        // Generate history index.
+        Repository repo = RepositoryFactory.getRepository(reposRoot);
+        History historyToStore = repo.getHistory(reposRoot);
+        cache.store(historyToStore, repo);
+
+        // Check complete list of history entries for the file in the renamed folder.
+        File testFile = Path.of(reposRoot.toString(), "renamedFolder", "FileInRenamedFolder.txt").toFile();
+        updatedHistory = cache.get(testFile, repo, false);
+        assertEquals(3, updatedHistory.getHistoryEntries().size());
+
+        HistoryEntry e0 = new HistoryEntry(
+                "14",
+                DateUtils.parseDate("2021-03-02T11:34:28.030Z", SVN_DATE_FORMAT),
+                "RichardH",
+                "Update to renamed file.",
+                true);
+        HistoryEntry e1 = new HistoryEntry(
+                "13",
+                DateUtils.parseDate("2021-03-02T08:54:54.693Z", SVN_DATE_FORMAT),
+                "RichardH",
+                "rename folder",
+                true);
+        HistoryEntry e2 = new HistoryEntry(
+                "12",
+                DateUtils.parseDate("2021-03-02T08:54:30.615Z", SVN_DATE_FORMAT),
+                "RichardH",
+                "added file to new folder",
+                true);
+
+        History histConstruct = new History();
+        LinkedList<HistoryEntry> entriesConstruct = new LinkedList<>();
+        entriesConstruct.add(e0);
+        entriesConstruct.add(e1);
+        entriesConstruct.add(e2);
+        histConstruct.setHistoryEntries(entriesConstruct);
+        assertSameEntries(histConstruct.getHistoryEntries(), updatedHistory.getHistoryEntries(), false);
+    }
+
     private void createSvnRepository() throws Exception {
         var svnLog = FileHistoryCacheTest.class.getResource("/history/svnlog.dump");
         Path tempDir = Files.createTempDirectory("opengrok");
@@ -697,7 +777,7 @@ private void createSvnRepository() throws Exception {
         assertEquals(0, svnLoadRepoFromDumpProcess.waitFor());
 
         var svnCheckoutProcess = new ProcessBuilder("svn", "checkout", Path.of(repo).toUri().toString(),
-                Path.of(repositories.getSourceRoot()).resolve("subversion").toString())
+                Path.of(repositories.getSourceRoot()).resolve(SUBVERSION_REPO_LOC).toString())
                 .start();
         assertEquals(0, svnCheckoutProcess.waitFor());
     }
@@ -784,7 +864,7 @@ void testRenamedFileHistoryWithPerPartes(int maxCount) throws Exception {
     void testRenamedFile() throws Exception {
         createSvnRepository();
 
-        File reposRoot = new File(repositories.getSourceRoot(), "subversion");
+        File reposRoot = new File(repositories.getSourceRoot(), SUBVERSION_REPO_LOC);
         History updatedHistory;
 
         // The test expects support for renamed files.
@@ -796,8 +876,7 @@ void testRenamedFile() throws Exception {
         cache.store(historyToStore, repo);
 
         // Check complete list of history entries for the renamed file.
-        File testFile = new File(reposRoot.toString() + File.separatorChar
-                            + "subfolder" + File.separatorChar + "TestFileRenamedAgain.txt");
+        File testFile = Path.of(reposRoot.toString(), "subfolder", "TestFileRenamedAgain.txt").toFile();
         updatedHistory = cache.get(testFile, repo, false);
         assertEquals(4, updatedHistory.getHistoryEntries().size());
 
diff --git a/opengrok-indexer/src/test/resources/history/svnlog.dump b/opengrok-indexer/src/test/resources/history/svnlog.dump
