You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/06/08 06:30:42 UTC
Re: [PR] ISSUE #1375 - Improve svn provider english outputs [maven-scm]
Copilot commented on code in PR #1376:
URL: https://github.com/apache/maven-scm/pull/1376#discussion_r3371198774
##########
maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/SvnCommandLineUtils.java:
##########
@@ -44,6 +44,41 @@
public final class SvnCommandLineUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(SvnCommandLineUtils.class);
+ /**
+ * Factory interface for creating {@link Commandline} instances.
+ * The default implementation simply returns {@code new Commandline()}.
+ * Tests may replace it via {@link #setCommandlineFactory(CommandlineFactory)}
+ * to return a subclass that captures environment variables, arguments, etc.
+ */
+ @FunctionalInterface
+ public interface CommandlineFactory {
+ Commandline create();
+ }
+
+ /**
+ * Default factory: plain {@code new Commandline()} — production behaviour.
+ */
+ private static final CommandlineFactory DEFAULT_FACTORY = Commandline::new;
+
+ /**
+ * Active factory. Replaced only in tests; never {@code null}.
+ */
+ private static CommandlineFactory commandlineFactory = DEFAULT_FACTORY;
+
+ /**
+ * Replaces the factory used to create {@link Commandline} instances.
+ * <p>
+ * Intended for tests only. Always restore the default after the test:
+ * <pre>
+ * SvnCommandLineUtils.setCommandlineFactory(null); // resets to default
+ * </pre>
+ *
+ * @param factory the factory to use, or {@code null} to reset to the default
+ */
+ public static void setCommandlineFactory(CommandlineFactory factory) {
+ commandlineFactory = (factory != null) ? factory : DEFAULT_FACTORY;
+ }
Review Comment:
setCommandlineFactory introduces a new public test hook on a production utility class. Since it’s intended for tests and the tests live in the same package, it should be package-private to avoid expanding the public API surface and to discourage non-test callers from mutating global state at runtime.
##########
maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/SvnCommandLineUtils.java:
##########
@@ -71,11 +106,15 @@ public static void addTarget(Commandline cl, List<File> files) throws IOExceptio
public static Commandline getBaseSvnCommandLine(
File workingDirectory, SvnScmProviderRepository repository, boolean interactive) {
- Commandline cl = new Commandline();
+ Commandline cl = commandlineFactory.create();
cl.setExecutable("svn");
try {
cl.addSystemEnvironment();
+ // ISSUE-1375: ensure that LC_ALL is unset to avoid locale issues with svn command output parsing
+ // (setting just LC_ALL=C goes too far, since it would imply that LC_CTYPE=C, which would cause
+ // svn to output non-ASCII chars in an unreadable way.)
+ cl.addEnvironment("LC_ALL", "");
cl.addEnvironment("LC_MESSAGES", "C");
Review Comment:
getBaseSvnCommandLine() now sets LC_MESSAGES="C", but SvnCommandLineUtils.execute(Commandline, StreamConsumer, ...) unconditionally overwrites LC_MESSAGES to "en" later. That means most svn invocations won’t actually run with LC_MESSAGES=C, making the behavior inconsistent across call sites and potentially negating the intended locale stabilization. Consider aligning these (e.g., use "C" consistently, or only set LC_MESSAGES in one place).
##########
maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/SvnCommandLineUtils.java:
##########
@@ -44,6 +44,41 @@
public final class SvnCommandLineUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(SvnCommandLineUtils.class);
+ /**
+ * Factory interface for creating {@link Commandline} instances.
+ * The default implementation simply returns {@code new Commandline()}.
+ * Tests may replace it via {@link #setCommandlineFactory(CommandlineFactory)}
+ * to return a subclass that captures environment variables, arguments, etc.
+ */
+ @FunctionalInterface
+ public interface CommandlineFactory {
+ Commandline create();
+ }
Review Comment:
CommandlineFactory is declared public, which makes it part of the library’s public API. Since it’s described as test-only and is only used from tests in the same package, it should be package-private to avoid committing to this type as a supported extension point.
##########
maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/SvnCommandLineUtils.java:
##########
@@ -44,6 +44,41 @@
public final class SvnCommandLineUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(SvnCommandLineUtils.class);
+ /**
+ * Factory interface for creating {@link Commandline} instances.
+ * The default implementation simply returns {@code new Commandline()}.
+ * Tests may replace it via {@link #setCommandlineFactory(CommandlineFactory)}
+ * to return a subclass that captures environment variables, arguments, etc.
+ */
+ @FunctionalInterface
+ public interface CommandlineFactory {
+ Commandline create();
+ }
+
+ /**
+ * Default factory: plain {@code new Commandline()} — production behaviour.
+ */
+ private static final CommandlineFactory DEFAULT_FACTORY = Commandline::new;
+
+ /**
+ * Active factory. Replaced only in tests; never {@code null}.
+ */
+ private static CommandlineFactory commandlineFactory = DEFAULT_FACTORY;
Review Comment:
commandlineFactory is mutable global state and can be swapped via setCommandlineFactory(). Without volatile (or synchronization), updates may not be visible across threads that are concurrently building command lines. Marking it volatile is a low-cost way to make reads/writes safely published.
##########
maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/src/test/java/org/apache/maven/scm/provider/svn/svnexe/command/SvnCommandLineUtilsTest.java:
##########
@@ -136,4 +138,39 @@ void testCryptPassword() throws Exception {
new File("."),
SvnCommandLineUtils.getBaseSvnCommandLine(new File("."), repo, false));
}
+
+ @Test
+ // ISSUE-1375: ensure that LC_ALL is unset and LC_MESSAGES is set to C to avoid locale issues with svn command
+ // output parsing
+ void testGetBaseSvnCommandLineLcAllIsSetToC() throws Exception {
+ Map<String, String> capturedEnvVarsMap = new HashMap<String, String>();
Review Comment:
The test name says LC_ALL is “set to C”, but the behavior under test is that LC_ALL is unset (set to an empty string) and LC_MESSAGES is set to C. Renaming the test to match the asserted behavior will make it easier to understand and maintain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org