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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
