Command line: quoted argfile paths, a closed thread pool, and the right flag in the --dry-run error

open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java

@@ -14,36 +14,51 @@
 
 package com.palantir.javaformat.java;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.base.CharMatcher;
+import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableRangeSet;
 import com.google.common.collect.Range;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Deque;
 import java.util.Iterator;
 import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 
 /** A parser for {@link CommandLineOptions}. */
 final class CommandLineOptionsParser {
 
     private static final Splitter COMMA_SPLITTER = Splitter.on(',');
     private static final Splitter COLON_SPLITTER = Splitter.on(':');
-    private static final Splitter ARG_SPLITTER =
-            Splitter.on(CharMatcher.breakingWhitespace()).omitEmptyStrings().trimResults();
+
+    /**
+     * Splits the arguments of a parameter file on whitespace (including tabs and line breaks), and lets an argument be
+     * quoted so that it keeps the whitespace inside it unchanged.
+     *
+     * <p>The regex matches either a quoted string (single or double quotes are allowed) or a plain unquoted string.
+     * Double quotes may appear inside a single-quoted string and vice versa, and are then kept as they are. For
+     * simplicity, escaped quotes are not handled.
+     */
+    private static final Pattern ARG_MATCHER = Pattern.compile(
+            "\"([^\"]*)(?:\"|$)" // group 1: string in double quotes (or until EOF), with whitespace allowed
+                    + "|" // OR
+                    + "'([^']*)(?:'|$)" // group 2: string in single quotes (or until EOF), with whitespace allowed
+                    + "|" // OR
+                    + "([^\\s\"']+)" // group 3: unquoted string, without whitespace and without any quotes
+            );
 
     /** Parses {@link CommandLineOptions}. */
     @SuppressWarnings("for-rollout:NullAway")
     static CommandLineOptions parse(Iterable<String> options) {
         CommandLineOptions.Builder optionsBuilder = CommandLineOptions.builder();
         List<String> expandedOptions = new ArrayList<>();
-        expandParamsFiles(options, expandedOptions);
+        expandParamsFiles(options, expandedOptions, new ArrayDeque<>());
         Iterator<String> it = expandedOptions.iterator();
         while (it.hasNext()) {
             String option = it.next();
@@ -226,7 +241,7 @@ private static Range<Integer> parseRange(String arg) {
      * Pre-processes an argument list, expanding arguments of the form {@code @filename} by reading the content of the
      * file and appending whitespace-delimited options to {@code arguments}.
      */
-    private static void expandParamsFiles(Iterable<String> args, List<String> expanded) {
+    private static void expandParamsFiles(Iterable<String> args, List<String> expanded, Deque<String> paramFilesStack) {
         for (String arg : args) {
             if (arg.isEmpty()) {
                 continue;
@@ -236,14 +251,35 @@ private static void expandParamsFiles(Iterable<String> args, List<String> expand
             } else if (arg.startsWith("@@")) {
                 expanded.add(arg.substring(1));
             } else {
-                Path path = Paths.get(arg.substring(1));
-                try {
-                    String sequence = new String(Files.readAllBytes(path), UTF_8);
-                    expandParamsFiles(ARG_SPLITTER.split(sequence), expanded);
-                } catch (IOException e) {
-                    throw new UncheckedIOException(path + ": could not read file: " + e.getMessage(), e);
+                String filename = arg.substring(1);
+                if (paramFilesStack.contains(filename)) {
+                    throw new IllegalArgumentException("parameter file was included recursively: " + filename);
+                }
+                paramFilesStack.push(filename);
+                expandParamsFiles(getParamsFromFile(filename), expanded, paramFilesStack);
+                String finishedFilename = paramFilesStack.pop();
+                Preconditions.checkState(filename.equals(finishedFilename));
+            }
+        }
+    }
+
+    /** Reads the parameters from a file, keeping quoted parameters whole. */
+    private static List<String> getParamsFromFile(String filename) {
+        String fileContent;
+        try {
+            fileContent = Files.readString(Path.of(filename));
+        } catch (IOException e) {
+            throw new UncheckedIOException(filename + ": could not read file: " + e.getMessage(), e);
+        }
+        List<String> paramsFromFile = new ArrayList<>();
+        Matcher m = ARG_MATCHER.matcher(fileContent);
+        while (m.find()) {
+            for (int i = 1; i <= m.groupCount(); i++) {
+                if (m.group(i) != null) { // only one group matches: double quotes, single quotes or unquoted string.
+                    paramsFromFile.add(m.group(i));
                 }
             }
         }
+        return paramsFromFile;
     }
 }

open-java-format/src/main/java/com/palantir/javaformat/java/Main.java

@@ -120,11 +120,18 @@ public int format(String... args) throws UsageException {
         }
     }
 
-    @SuppressWarnings("for-rollout:RedundantControlFlow")
     private int formatFiles(CommandLineOptions parameters, JavaFormatterOptions options) {
         int numThreads = Math.min(MAX_THREADS, parameters.files().size());
-        ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
+        // Closing the pool ends its threads, so that a tool that runs Main in-process does not keep them. The close
+        // waits for the submitted tasks, which formatFiles has already waited for.
+        try (ExecutorService executorService = Executors.newFixedThreadPool(numThreads)) {
+            return formatFiles(parameters, options, executorService);
+        }
+    }
 
+    @SuppressWarnings("for-rollout:RedundantControlFlow")
+    private int formatFiles(
+            CommandLineOptions parameters, JavaFormatterOptions options, ExecutorService executorService) {
         Map<Path, String> inputs = new LinkedHashMap<>();
         Map<Path, Future<String>> results = new LinkedHashMap<>();
         boolean allOk = true;
@@ -273,7 +280,7 @@ public static CommandLineOptions processArgs(String... args) throws UsageExcepti
             throw new UsageException("--assume-filename is only supported when formatting standard input");
         }
         if (parameters.dryRun() && parameters.inPlace()) {
-            throw new UsageException("cannot use --dry-run and --in-place at the same time");
+            throw new UsageException("cannot use --dry-run and --replace at the same time");
         }
         return parameters;
     }

open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java

@@ -102,11 +102,18 @@ public void inPlaceStdin() {
 
     @Test
     public void inPlaceDryRun() {
+        try {
+            Main.processArgs("--replace", "--dry-run", "A.java");
+            fail("fail");
+        } catch (UsageException e) {
+            assertThat(e).hasMessageThat().contains("cannot use --dry-run and --replace at the same time");
+        }
+
         try {
             Main.processArgs("-i", "-n", "A.java");
             fail("fail");
         } catch (UsageException e) {
-            assertThat(e).hasMessageThat().contains("cannot use --dry-run and --in-place at the same time");
+            assertThat(e).hasMessageThat().contains("cannot use --dry-run and --replace at the same time");
         }
     }
 

open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java

@@ -16,6 +16,7 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
 
 import com.google.common.collect.Range;
@@ -176,6 +177,61 @@ public void paramsFile() throws IOException {
         assertThat(options.files()).containsExactly("L", "M", "ℕ", "@O", "P", "Q");
     }
 
+    @Test
+    public void paramsFileWithNesting() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer"));
+        Path exit = Files.createFile(testFolder.resolve("exit"));
+        Path nested1 = Files.createFile(testFolder.resolve("nested1"));
+        Path nested2 = Files.createFile(testFolder.resolve("nested2"));
+        Path nested3 = Files.createFile(testFolder.resolve("nested3"));
+
+        String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"};
+
+        Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8));
+        Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8));
+        Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8));
+        Files.write(nested2, ("O\n@" + nested3.toAbsolutePath() + "\nR").getBytes(UTF_8));
+        Files.write(nested3, "P\n\n   \n@@Q\n".getBytes(UTF_8));
+
+        CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args));
+        assertThat(options.files()).containsExactly("L", "M", "ℕ", "O", "P", "@Q", "R", "S", "T", "U");
+    }
+
+    @Test
+    public void paramsFileWithRecursion() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer"));
+        Path exit = Files.createFile(testFolder.resolve("exit"));
+        Path nested1 = Files.createFile(testFolder.resolve("nested1"));
+        Path nested2 = Files.createFile(testFolder.resolve("nested2"));
+
+        String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"};
+
+        Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8));
+        Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8));
+        Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8));
+        Files.write(nested2, ("O\n@" + nested1.toAbsolutePath() + "\nR").getBytes(UTF_8));
+
+        assertThatThrownBy(() -> CommandLineOptionsParser.parse(Arrays.asList(args)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageStartingWith("parameter file was included recursively: ");
+    }
+
+    @Test
+    public void paramsFileWithQuotesAndWhitespaces() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer with whitespace"));
+        Path exit = Files.createFile(testFolder.resolve("exit with whitespace"));
+        Path nested = Files.createFile(testFolder.resolve("nested with whitespace"));
+
+        String[] args = {"--dry-run", "@" + exit, "L +w", "@" + outer, "Q +w"};
+
+        Files.write(exit, "--set-exit-if-changed 'K +w".getBytes(UTF_8));
+        Files.write(outer, ("\"'M' +w\"\n\"@" + nested.toAbsolutePath() + "\"\n'\"P\" +w'").getBytes(UTF_8));
+        Files.write(nested, "\"ℕ +w\"\n\n   \n\"@@O +w".getBytes(UTF_8));
+
+        CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args));
+        assertThat(options.files()).containsExactly("K +w", "L +w", "'M' +w", "ℕ +w", "@O +w", "\"P\" +w", "Q +w");
+    }
+
     @Test
     public void assumeFilename() {
         assertThat(CommandLineOptionsParser.parse(Arrays.asList("--assume-filename", "Foo.java"))

open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java

@@ -35,6 +35,8 @@
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.EnumSet;
 import java.util.Locale;
+import java.util.concurrent.FutureTask;
+import java.util.concurrent.TimeUnit;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.api.parallel.Execution;
@@ -91,6 +93,29 @@ public void version() throws UsageException {
         assertThat(err.toString()).contains("open-java-format: Version ");
     }
 
+    // Main used to leave its thread pool running after format returned. The command line does not notice, because it
+    // exits, but anything that runs Main in-process kept the idle threads (#40, from google/google-java-format#384).
+    @Test
+    public void formatLeavesNoPoolThreadRunning() throws Exception {
+        Path path = Files.writeString(testFolder.resolve("A.java"), "class A {}\n");
+        Main main = new Main(
+                new PrintWriter(new StringWriter(), true), new PrintWriter(new StringWriter(), true), System.in);
+        // The pool's threads join the thread group of the thread that creates the pool.
+        ThreadGroup group = new ThreadGroup("formatLeavesNoPoolThreadRunning");
+        FutureTask<Integer> format = new FutureTask<>(() -> main.format(path.toString()));
+        new Thread(group, format).start();
+        assertThat(format.get()).isEqualTo(0);
+
+        Thread[] threads = new Thread[group.activeCount() + 16];
+        int count = group.enumerate(threads);
+        for (int i = 0; i < count; i++) {
+            threads[i].join(TimeUnit.SECONDS.toMillis(10));
+            assertWithMessage(threads[i].getName() + " is still running")
+                    .that(threads[i].isAlive())
+                    .isFalse();
+        }
+    }
+
     @Test
     public void preserveOriginalFile() throws Exception {
         Path path = Files.createFile(testFolder.resolve("Test.java"));