From dd93c1243fadd7a8c7c483ee27a1837b0dfbeeb8 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Thu, 18 Sep 2025 20:57:35 +0200 Subject: [PATCH 1/8] specify username for all hg commands this avoids interactive mode and errors due to no user being set fixes #4849 --- .../org/opengrok/indexer/util/Executor.java | 43 +++++++++++++------ .../history/MercurialRepositoryTest.java | 6 ++- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java index 3550f5956e7..a3492165518 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java @@ -33,6 +33,7 @@ import java.io.Reader; import java.lang.Thread.UncaughtExceptionHandler; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -67,6 +68,7 @@ public class Executor { private byte[] stdout; private byte[] stderr; private int timeout; // in milliseconds, 0 means no timeout + private final Map environ; /** * Create a new instance of the Executor. @@ -88,38 +90,44 @@ public Executor(List cmdList) { * Create a new instance of the Executor with default command timeout value. * The timeout value will be based on the running context (indexer or web application). * @param cmdList A list containing the command to execute - * @param workingDirectory The directory the process should have as the - * working directory + * @param workingDirectory The directory the process should have as the working directory */ public Executor(List cmdList, File workingDirectory) { - RuntimeEnvironment env = RuntimeEnvironment.getInstance(); - int timeoutSec = env.isIndexer() ? env.getIndexerCommandTimeout() : env.getInteractiveCommandTimeout(); - - this.cmdList = cmdList; - this.workingDirectory = workingDirectory; - this.timeout = timeoutSec * 1000; + this(cmdList, workingDirectory, new HashMap<>()); } /** * Create a new instance of the Executor with specific timeout value. * @param cmdList A list containing the command to execute - * @param workingDirectory The directory the process should have as the - * working directory + * @param workingDirectory The directory the process should have as the working directory * @param timeout If the command runs longer than the timeout (seconds), * it will be terminated. If the value is 0, no timer * will be set up. */ public Executor(List cmdList, File workingDirectory, int timeout) { + this(cmdList, workingDirectory, new HashMap<>()); + + this.timeout = timeout * 1000; + } + + /** + * Create a new instance of the Executor with default command timeout value. + * The timeout value will be based on the running context (indexer or web application). + * @param cmdList A list containing the command to execute + * @param workingDirectory The directory the process should have as the working directory + */ + public Executor(List cmdList, File workingDirectory, Map environ) { this.cmdList = cmdList; this.workingDirectory = workingDirectory; - this.timeout = timeout * 1000; + this.environ = environ; + + setDefaultTimeout(); } /** * Create a new instance of the Executor with or without timeout. * @param cmdList A list containing the command to execute - * @param workingDirectory The directory the process should have as the - * working directory + * @param workingDirectory The directory the process should have as the working directory * @param useTimeout terminate the process after default timeout or not */ public Executor(List cmdList, File workingDirectory, boolean useTimeout) { @@ -129,6 +137,13 @@ public Executor(List cmdList, File workingDirectory, boolean useTimeout) } } + private void setDefaultTimeout() { + RuntimeEnvironment env = RuntimeEnvironment.getInstance(); + int timeoutSec = env.isIndexer() ? env.getIndexerCommandTimeout() : env.getInteractiveCommandTimeout(); + + this.timeout = timeoutSec * 1000; + } + /** * Execute the command and collect the output. All exceptions will be * logged. @@ -180,6 +195,8 @@ public int exec(final boolean reportExceptions, StreamHandler handler) { .map(File::toString) .orElseGet(() -> System.getProperty("user.dir")); + processBuilder.environment().putAll(environ); + String envStr = ""; if (LOGGER.isLoggable(Level.FINER)) { Map envMap = processBuilder.environment(); diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java index 3230461d0f4..745efddca20 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java @@ -50,7 +50,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.TreeSet; @@ -193,7 +195,9 @@ public static void runHgCommand(File reposRoot, String... args) { cmdargs.add(repo.getRepoCommand()); cmdargs.addAll(Arrays.asList(args)); - Executor exec = new Executor(cmdargs, reposRoot); + Map env = new HashMap<>(); + env.put("HGUSER", "foo "); + Executor exec = new Executor(cmdargs, reposRoot, env); int exitCode = exec.exec(); assertEquals(0, exitCode, "hg command '" + cmdargs + "' failed." + "\nexit code: " + exitCode From f719b58246b7e6f139a9a60910babb156d589311 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:01:59 +0200 Subject: [PATCH 2/8] add test --- .../opengrok/indexer/util/ExecutorTest.java | 47 +++++++++++++++++-- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java index b2602abbe61..4049766d3fa 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java @@ -18,7 +18,7 @@ */ /* - * Copyright (c) 2008, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2008, 2025, Oracle and/or its affiliates. All rights reserved. * Portions Copyright (c) 2019, Chris Fraire . */ package org.opengrok.indexer.util; @@ -30,9 +30,12 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.io.Reader; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -64,10 +67,14 @@ void testReader() throws IOException { cmdList.add("testing org.opengrok.indexer.util.Executor"); Executor instance = new Executor(cmdList); assertEquals(0, instance.exec()); - BufferedReader in = new BufferedReader(instance.getOutputReader()); + Reader outputReader = instance.getOutputReader(); + assertNotNull(outputReader); + BufferedReader in = new BufferedReader(outputReader); assertEquals("testing org.opengrok.indexer.util.Executor", in.readLine()); in.close(); - in = new BufferedReader(instance.getErrorReader()); + Reader errorReader = instance.getErrorReader(); + assertNotNull(errorReader); + in = new BufferedReader(errorReader); assertNull(in.readLine()); in.close(); } @@ -81,14 +88,44 @@ void testStream() throws IOException { assertEquals(0, instance.exec()); assertNotNull(instance.getOutputStream()); assertNotNull(instance.getErrorStream()); - BufferedReader in = new BufferedReader(instance.getOutputReader()); + Reader outputReader = instance.getOutputReader(); + assertNotNull(outputReader); + BufferedReader in = new BufferedReader(outputReader); assertEquals("testing org.opengrok.indexer.util.Executor", in.readLine()); in.close(); - in = new BufferedReader(instance.getErrorReader()); + Reader errorReader = instance.getErrorReader(); + assertNotNull(errorReader); + in = new BufferedReader(errorReader); assertNull(in.readLine()); in.close(); } + /** + * Test setting environment variable. Assumes the {@code env} program exists + * and reports list of environment variables. + */ + @Test + void testEnv() throws IOException { + List cmdList = List.of("env"); + final Map env = new HashMap<>(); + env.put("foo", "bar"); + Executor instance = new Executor(cmdList, null, env); + assertEquals(0, instance.exec()); + Reader outputReader = instance.getOutputReader(); + assertNotNull(outputReader); + BufferedReader in = new BufferedReader(outputReader); + String line; + boolean found = false; + while ((line = in.readLine()) != null) { + if (line.equals("foo=bar")) { + found = true; + break; + } + } + assertTrue(found); + in.close(); + } + /** * {@link Executor.StreamHandler} implementation that always fails with {@link IOException}. */ From 9d48dad80097d1f8d137796dd1bebf2da8f01c83 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:11:32 +0200 Subject: [PATCH 3/8] test constructor with timeout --- .../java/org/opengrok/indexer/util/Executor.java | 12 ++++++++---- .../java/org/opengrok/indexer/util/ExecutorTest.java | 7 +++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java index a3492165518..3233d8c8c78 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java @@ -92,7 +92,7 @@ public Executor(List cmdList) { * @param cmdList A list containing the command to execute * @param workingDirectory The directory the process should have as the working directory */ - public Executor(List cmdList, File workingDirectory) { + public Executor(List cmdList, @Nullable File workingDirectory) { this(cmdList, workingDirectory, new HashMap<>()); } @@ -104,7 +104,7 @@ public Executor(List cmdList, File workingDirectory) { * it will be terminated. If the value is 0, no timer * will be set up. */ - public Executor(List cmdList, File workingDirectory, int timeout) { + public Executor(List cmdList, @Nullable File workingDirectory, int timeout) { this(cmdList, workingDirectory, new HashMap<>()); this.timeout = timeout * 1000; @@ -116,7 +116,7 @@ public Executor(List cmdList, File workingDirectory, int timeout) { * @param cmdList A list containing the command to execute * @param workingDirectory The directory the process should have as the working directory */ - public Executor(List cmdList, File workingDirectory, Map environ) { + public Executor(List cmdList, @Nullable File workingDirectory, Map environ) { this.cmdList = cmdList; this.workingDirectory = workingDirectory; this.environ = environ; @@ -144,6 +144,10 @@ private void setDefaultTimeout() { this.timeout = timeoutSec * 1000; } + int getTimeout() { + return timeout / 1000; + } + /** * Execute the command and collect the output. All exceptions will be * logged. @@ -239,7 +243,7 @@ public int exec(final boolean reportExceptions, StreamHandler handler) { @Override public void run() { LOGGER.log(Level.WARNING, String.format("Terminating process of command [%s] in directory '%s' " + - "due to timeout %d seconds", cmd_str, dir_str, timeout / 1000)); + "due to timeout %d seconds", cmd_str, dir_str, getTimeout())); proc.destroy(); } }, timeout); diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java index 4049766d3fa..22fb6845d03 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java @@ -48,6 +48,13 @@ */ class ExecutorTest { + @Test + void testConstructorWithTimeout() { + int timeout = 42; + Executor executor = new Executor(List.of("foo"), null, timeout); + assertEquals(timeout, executor.getTimeout()); + } + @Test void testString() { List cmdList = new ArrayList<>(); From 86f13c89964a61966736838510d93e5db97f6527 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:26:48 +0200 Subject: [PATCH 4/8] fix some nits --- .../history/MercurialRepositoryTest.java | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java index 745efddca20..262e7b50129 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java @@ -189,17 +189,17 @@ void testGetHistoryPartial() throws Exception { * @param args {@code hg} command arguments */ public static void runHgCommand(File reposRoot, String... args) { - List cmdargs = new ArrayList<>(); + List commandWithArgs = new ArrayList<>(); MercurialRepository repo = new MercurialRepository(); - cmdargs.add(repo.getRepoCommand()); - cmdargs.addAll(Arrays.asList(args)); + commandWithArgs.add(repo.getRepoCommand()); + commandWithArgs.addAll(Arrays.asList(args)); - Map env = new HashMap<>(); - env.put("HGUSER", "foo "); - Executor exec = new Executor(cmdargs, reposRoot, env); + final Map env = new HashMap<>(); + env.put("HGUSER", "Snufkin "); + Executor exec = new Executor(commandWithArgs, reposRoot, env); int exitCode = exec.exec(); - assertEquals(0, exitCode, "hg command '" + cmdargs + "' failed." + assertEquals(0, exitCode, "command '" + commandWithArgs + "' failed." + "\nexit code: " + exitCode + "\nstdout:\n" + exec.getOutputString() + "\nstderr:\n" + exec.getErrorString()); @@ -303,10 +303,7 @@ void testGetHistoryGetRenamed() throws Exception { String exp_str = "This is totally plaintext file.\n"; byte[] buffer = new byte[1024]; - /* - * In our test repository the file was renamed twice since - * revision 3. - */ + // In our test repository the file was renamed twice since revision 3. InputStream input = mr.getHistoryGet(repositoryRoot.getCanonicalPath(), "novel.txt", "3"); assertNotNull(input); @@ -354,8 +351,7 @@ void testGetHistorySinceTillNullRev() throws Exception { assertNotNull(history); assertNotNull(history.getHistoryEntries()); assertEquals(6, history.getHistoryEntries().size()); - List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision). - collect(Collectors.toList()); + List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision).toList(); assertEquals(List.of(Arrays.copyOfRange(REVISIONS, 4, REVISIONS.length)), revisions); } @@ -366,8 +362,7 @@ void testGetHistorySinceTillRevNull() throws Exception { assertNotNull(history); assertNotNull(history.getHistoryEntries()); assertEquals(3, history.getHistoryEntries().size()); - List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision). - collect(Collectors.toList()); + List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision).toList(); assertEquals(List.of(Arrays.copyOfRange(REVISIONS, 0, 3)), revisions); } @@ -378,8 +373,7 @@ void testGetHistorySinceTillRevRev() throws Exception { assertNotNull(history); assertNotNull(history.getHistoryEntries()); assertEquals(5, history.getHistoryEntries().size()); - List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision). - collect(Collectors.toList()); + List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision).toList(); assertEquals(List.of(Arrays.copyOfRange(REVISIONS, 2, 7)), revisions); } @@ -393,8 +387,7 @@ void testGetHistoryRenamedFileTillRev() throws Exception { assertNotNull(history); assertNotNull(history.getHistoryEntries()); assertEquals(5, history.getHistoryEntries().size()); - List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision). - collect(Collectors.toList()); + List revisions = history.getHistoryEntries().stream().map(HistoryEntry::getRevision).toList(); assertEquals(List.of(Arrays.copyOfRange(REVISIONS, 2, 7)), revisions); } @@ -447,8 +440,10 @@ void testAnnotationNegative() throws Exception { } private static Stream, List>> provideParametersForPositiveAnnotationTest() { - return Stream.of(Triple.of("8:6a8c423f5624", List.of("7", "8"), List.of("8:6a8c423f5624", "7:db1394c05268")), - Triple.of("7:db1394c05268", List.of("7"), List.of("7:db1394c05268"))); + return Stream.of( + Triple.of("8:6a8c423f5624", List.of("7", "8"), List.of("8:6a8c423f5624", "7:db1394c05268")), + Triple.of("7:db1394c05268", List.of("7"), List.of("7:db1394c05268")) + ); } @ParameterizedTest @@ -583,7 +578,7 @@ void testBuildTagListOneMore() throws Exception { assertNotNull(tags); assertEquals(3, tags.size()); expectedTags = List.of("start_of_novel", newTagName, branchTagName); - assertEquals(expectedTags, tags.stream().map(TagEntry::getTags).collect(Collectors.toList())); + assertEquals(expectedTags, tags.stream().map(TagEntry::getTags).toList()); // cleanup IOUtils.removeRecursive(repositoryRootPath); From e1a6cc247113dce276b4f5e97ab635ee06f70fdf Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:31:20 +0200 Subject: [PATCH 5/8] fix javadoc for the new constructor --- .../src/main/java/org/opengrok/indexer/util/Executor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java index 3233d8c8c78..9be07820400 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java @@ -111,10 +111,11 @@ public Executor(List cmdList, @Nullable File workingDirectory, int timeo } /** - * Create a new instance of the Executor with default command timeout value. + * Create a new instance of the Executor with default command timeout value and environment. * The timeout value will be based on the running context (indexer or web application). * @param cmdList A list containing the command to execute * @param workingDirectory The directory the process should have as the working directory + * @param environ map of environment variables to be added/overridden */ public Executor(List cmdList, @Nullable File workingDirectory, Map environ) { this.cmdList = cmdList; From 712081fd44e313dbfc8749eb98c87f1108376eb2 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:46:42 +0200 Subject: [PATCH 6/8] add Nullable annotation --- .../src/main/java/org/opengrok/indexer/util/Executor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java index 9be07820400..1cf30a4fc76 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java +++ b/opengrok-indexer/src/main/java/org/opengrok/indexer/util/Executor.java @@ -131,7 +131,7 @@ public Executor(List cmdList, @Nullable File workingDirectory, Map cmdList, File workingDirectory, boolean useTimeout) { + public Executor(List cmdList, @Nullable File workingDirectory, boolean useTimeout) { this(cmdList, workingDirectory); if (!useTimeout) { this.timeout = 0; From f98b0d1dda87c987ea9de2146b362ba38d9a1ac5 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 11:53:09 +0200 Subject: [PATCH 7/8] add comment --- .../org/opengrok/indexer/history/MercurialRepositoryTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java index 262e7b50129..c9b29e4e02d 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/history/MercurialRepositoryTest.java @@ -196,6 +196,7 @@ public static void runHgCommand(File reposRoot, String... args) { commandWithArgs.addAll(Arrays.asList(args)); final Map env = new HashMap<>(); + // Set the user to record commits in order to prevent hg commands entering interactive mode. env.put("HGUSER", "Snufkin "); Executor exec = new Executor(commandWithArgs, reposRoot, env); int exitCode = exec.exec(); From a0ab741837f0dcc8083120cb1b6f8bffbdfc4cad Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Fri, 19 Sep 2025 12:02:19 +0200 Subject: [PATCH 8/8] augment comment --- .../src/test/java/org/opengrok/indexer/util/ExecutorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java index 22fb6845d03..3b03ccd4ae6 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/util/ExecutorTest.java @@ -109,7 +109,7 @@ void testStream() throws IOException { /** * Test setting environment variable. Assumes the {@code env} program exists - * and reports list of environment variables. + * and reports list of environment variables along with their values. */ @Test void testEnv() throws IOException {