From e9fcd0145e05a100cae41f50fb5362f5410181d4 Mon Sep 17 00:00:00 2001 From: Karlheinz Friedberger Date: Wed, 23 Sep 2026 10:43:01 +0300 Subject: [PATCH 1/3] Read quoted arguments from parameter files, and refuse a file that includes itself CommandLineOptionsParser split an @argfile at every whitespace character and knew no quotes, so a path with a space in it became several arguments. Each was reported as "Skipping non-Java file", and the run still exited 0 (#40, from google/google-java-format#421). A parameter file that included itself recursed until the stack overflowed. This ports google/google-java-format#931 by Karlheinz Friedberger, its three commits squashed into one. An argument in a parameter file may be quoted with double or single quotes and then keeps its whitespace; an unclosed quote runs to the end of the file; and a file that is already being read is reported as "parameter file was included recursively". Escaped quotes are not handled, as upstream. The three CommandLineOptionsParserTest cases come from that PR. --- .../java/CommandLineOptionsParser.java | 64 +++++++++++++++---- .../java/CommandLineOptionsParserTest.java | 56 ++++++++++++++++ 2 files changed, 106 insertions(+), 14 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java index d7c595227..638ed34b1 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java @@ -14,9 +14,7 @@ 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; @@ -24,10 +22,13 @@ 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}. */ @@ -35,15 +36,29 @@ 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. + * + *

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 options) { CommandLineOptions.Builder optionsBuilder = CommandLineOptions.builder(); List expandedOptions = new ArrayList<>(); - expandParamsFiles(options, expandedOptions); + expandParamsFiles(options, expandedOptions, new ArrayDeque<>()); Iterator it = expandedOptions.iterator(); while (it.hasNext()) { String option = it.next(); @@ -226,7 +241,7 @@ private static Range 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 args, List expanded) { + private static void expandParamsFiles(Iterable args, List expanded, Deque paramFilesStack) { for (String arg : args) { if (arg.isEmpty()) { continue; @@ -236,14 +251,35 @@ private static void expandParamsFiles(Iterable args, List 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 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 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; } } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java index 344437e94..ced15a57c 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java +++ b/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")) From cee25339477d5ef96d813c6487617c5070a35def Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Wed, 23 Sep 2026 10:44:48 +0300 Subject: [PATCH 2/3] Shut down the command line's thread pool when formatting is done Main.formatFiles created a fixed thread pool on every call and never shut it down. The command line does not notice, because the process exits, but a tool that runs Main in-process kept up to MAX_THREADS idle threads per call (#40, from google/google-java-format#384). The pool is now closed when formatFiles returns. On Java 21 ExecutorService is AutoCloseable, and close() waits for the submitted tasks, which formatFiles has already waited for by then. The new MainTest runs format from a thread of its own thread group, which the pool's threads join, and fails without the change because a pool thread is still running. --- .../com/palantir/javaformat/java/Main.java | 11 ++++++-- .../palantir/javaformat/java/MainTest.java | 25 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java b/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java index a05188668..a3b53566f 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java +++ b/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 inputs = new LinkedHashMap<>(); Map> results = new LinkedHashMap<>(); boolean allOk = true; diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java index cbbccee6a..08fc5dabb 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java +++ b/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 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")); From fe8966da6c878bf4bbb2ddf583b22654f3a2fe4e Mon Sep 17 00:00:00 2001 From: rootkiller6788 Date: Wed, 23 Sep 2026 10:45:38 +0300 Subject: [PATCH 3/3] Name --replace in the error for --dry-run with in-place formatting "cannot use --dry-run and --in-place at the same time" named a flag that does not exist: in-place formatting is -i, -r, -replace or --replace (#40, from google/google-java-format#1094). The message now says --replace, the long form the usage text shows. This ports google/google-java-format#1451 by rootkiller6788, including its test for the --replace --dry-run spelling next to -i -n. --- .../src/main/java/com/palantir/javaformat/java/Main.java | 2 +- .../palantir/javaformat/java/CommandLineFlagsTest.java | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java b/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java index a3b53566f..5551b325d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/Main.java @@ -280,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; } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java index 7ce5ee956..2dea6c882 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineFlagsTest.java +++ b/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"); } }