make sure getHistoryGet() implementations for CVSRepository are aligned (#3942)

fixes #3929

Author: vladak

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

@@ -24,12 +24,11 @@
 package org.opengrok.indexer.history;
 
 import java.io.BufferedReader;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileReader;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -65,7 +64,7 @@ public CVSRepository() {
          * that this repository is always marked as working even though it does
          * not have the binary available on the system.
          *
-         * Setting this to null does restores the default behavior (as java
+         * Setting this to null restores the default behavior (as java
          * default for reference is null) for this repository - detecting the
          * binary and act upon that.
          *
@@ -194,22 +193,19 @@ Executor getHistoryLogExecutor(final File file) throws IOException {
     }
 
     @Override
-    public InputStream getHistoryGet(String parent, String basename, String rev) {
-        InputStream ret = null;
-
+    boolean getHistoryGet(OutputStream out, String parent, String basename, String rev) {
         String revision = rev;
-
         if (rev.indexOf(':') != -1) {
             revision = rev.substring(0, rev.indexOf(':'));
         }
+
         try {
             ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK);
             String[] argv = {RepoCommand, "up", "-p", "-r", revision, basename};
             Executor executor = new Executor(Arrays.asList(argv), new File(parent),
                 RuntimeEnvironment.getInstance().getInteractiveCommandTimeout());
             executor.exec();
 
-            ByteArrayOutputStream out = new ByteArrayOutputStream();
             byte[] buffer = new byte[32 * 1024];
             try (InputStream in = executor.getOutputStream()) {
                 int len;
@@ -220,13 +216,12 @@ public InputStream getHistoryGet(String parent, String basename, String rev) {
                     }
                 }
             }
-
-            ret = new ByteArrayInputStream(out.toByteArray());
         } catch (Exception exp) {
             LOGGER.log(Level.WARNING, "Failed to get history", exp);
+            return false;
         }
 
-        return ret;
+        return true;
     }
 
     @Override

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

@@ -319,7 +319,7 @@ public History getHistory(File file, boolean withFiles, boolean ui) throws Histo
     }
 
     /**
-     * Gets a named revision of the specified file into the specified target.
+     * Gets a named revision of the specified file into the specified target file.
      *
      * @param target a require target file
      * @param parent The directory containing the file
@@ -328,12 +328,9 @@ public History getHistory(File file, boolean withFiles, boolean ui) throws Histo
      * @return {@code true} if content was found
      * @throws java.io.IOException if an I/O error occurs
      */
-    public boolean getRevision(File target, String parent, String basename,
-            String rev) throws IOException {
-
+    public boolean getRevision(File target, String parent, String basename, String rev) throws IOException {
         Repository repo = getRepository(new File(parent));
-        return repo != null && repo.getHistoryGet(target, parent,
-                basename, rev);
+        return repo != null && repo.getHistoryGet(target, parent, basename, rev);
     }
 
     /**

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

@@ -44,6 +44,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import org.jetbrains.annotations.Nullable;
 import org.opengrok.indexer.configuration.CommandTimeoutType;
 import org.opengrok.indexer.configuration.RuntimeEnvironment;
 import org.opengrok.indexer.logger.LoggerFactory;
@@ -153,7 +154,7 @@ public HistoryEntry getLastHistoryEntry(File file, boolean ui) throws HistoryExc
         }
     }
 
-    public Repository() {
+    Repository() {
         super();
         ignoredFiles = new ArrayList<>();
         ignoredDirs = new ArrayList<>();
@@ -225,9 +226,8 @@ History getHistory(File file, String sinceRevision) throws HistoryException {
      * @param revision the revision we expect the oldest entry to have
      * @throws HistoryException if the oldest entry was not the one we expected
      */
-    void removeAndVerifyOldestChangeset(List<HistoryEntry> entries,
-            String revision)
-            throws HistoryException {
+    void removeAndVerifyOldestChangeset(List<HistoryEntry> entries, String revision) throws HistoryException {
+
         HistoryEntry entry = entries.isEmpty() ? null : entries.remove(entries.size() - 1);
 
         // TODO We should check more thoroughly that the changeset is the one
@@ -244,7 +244,7 @@ void removeAndVerifyOldestChangeset(List<HistoryEntry> entries,
 
     /**
      * Gets the contents of a specific version of a named file, and copies
-     * into the specified target.
+     * into the specified target file.
      *
      * @param target a required target file which will be overwritten
      * @param parent the name of the directory containing the file
@@ -267,6 +267,7 @@ public boolean getHistoryGet(File target, String parent, String basename, String
      * @param rev the revision to get
      * @return a defined instance if contents were found; or else {@code null}
      */
+    @Nullable
     public InputStream getHistoryGet(String parent, String basename, String rev) {
         ByteArrayOutputStream out = new ByteArrayOutputStream();
         if (getHistoryGet(out, parent, basename, rev)) {

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

@@ -30,6 +30,8 @@
 import java.io.IOException;
 import java.net.URISyntaxException;
 import java.nio.channels.FileChannel;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -51,7 +53,7 @@
 import static org.opengrok.indexer.condition.RepositoryInstalled.Type.CVS;
 
 /**
- *
+ * Tests for {@link CVSRepository} functionality.
  * @author austvik
  */
 @EnabledForRepository(CVS)
@@ -120,7 +122,7 @@ public static void runCvsCommand(File reposRoot, String... args) {
     @Test
     void testGetBranchNoBranch() throws Exception {
         setUpTestRepository();
-        File root = new File(repository.getSourceRoot(), "cvs_test/cvsrepo");
+        File root = Path.of(repository.getSourceRoot(), "cvs_test", "cvsrepo").toFile();
         CVSRepository cvsrepo = (CVSRepository) RepositoryFactory.getRepository(root);
         assertNull(cvsrepo.getBranch());
     }
@@ -135,27 +137,26 @@ void testGetBranchNoBranch() throws Exception {
     @Test
     void testNewBranch() throws Exception {
         setUpTestRepository();
-        File root = new File(repository.getSourceRoot(), "cvs_test/cvsrepo");
+        File root = Path.of(repository.getSourceRoot(), "cvs_test", "cvsrepo").toFile();
 
         // Create new branch and switch to it.
         runCvsCommand(root, "tag", "-b", "mybranch");
         // Note that the 'update' command will change the entries in 'cvsroot' directory.
         runCvsCommand(root, "update", "-r", "mybranch");
 
-        // Now the repository object can be instantiated so that determineBranch()
-        // will be called.
+        // Now the repository object can be instantiated and determineBranch() will be called.
         CVSRepository cvsrepo = (CVSRepository) RepositoryFactory.getRepository(root);
 
         assertEquals("mybranch", cvsrepo.getBranch());
 
         // Change the content and commit.
         File mainC = new File(root, "main.c");
-        FileChannel outChan = new FileOutputStream(mainC, true).getChannel();
-        outChan.truncate(0);
-        outChan.close();
-        FileWriter fw = new FileWriter(mainC);
-        fw.write("#include <foo.h>\n");
-        fw.close();
+        try (FileChannel outChan = new FileOutputStream(mainC, true).getChannel()) {
+            outChan.truncate(0);
+        }
+        try (FileWriter fw = new FileWriter(mainC)) {
+            fw.write("#include <foo.h>\n");
+        }
         runCvsCommand(root, "commit", "-m", "change on a branch", "main.c");
 
         // Check that annotation for the changed line has branch revision.
@@ -169,6 +170,32 @@ void testNewBranch() throws Exception {
         assertEquals("1.1", mainCHistory.getHistoryEntries().get(2).getRevision());
     }
 
+    @Test
+    void testGetHistoryGet() throws Exception {
+        setUpTestRepository();
+        File root = Path.of(repository.getSourceRoot(), "cvs_test", "cvsrepo").toFile();
+        CVSRepository cvsRepo = (CVSRepository) RepositoryFactory.getRepository(root);
+
+        Path repoRoot = Path.of(repository.getSourceRoot(), "cvs_test", "cvsrepo");
+        assertTrue(repoRoot.toFile().exists());
+        String repoFile = "main.c";
+        String revision = "1.2";
+
+        File tmpFile = File.createTempFile("cvsGetHistoryGetTest", "");
+        assertTrue(tmpFile.exists());
+        try (FileOutputStream out = new FileOutputStream(tmpFile)) {
+            assertTrue(cvsRepo.getHistoryGet(out, repoRoot.toString(), repoFile, revision));
+        }
+
+        String revisionContents1 = new String(Files.readAllBytes(tmpFile.toPath()));
+        tmpFile.delete();
+        assertTrue(revisionContents1.length() > 0);
+
+        String revisionContents2 = new String(cvsRepo.getHistoryGet(repoRoot.toString(), repoFile, revision).
+                readAllBytes());
+        assertEquals(revisionContents1, revisionContents2);
+    }
+
     /**
      * Assert that revision strings in history entries are sorted semantically.
      * This is necessary for displaying revisions on a branch in correct order.