Copilot commented on code in PR #10906:
URL: https://github.com/apache/maven/pull/10906#discussion_r4247763869
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -211,6 +211,26 @@ public class MavenCli {
private static final Pattern NEXT_LINE = Pattern.compile("\r?\n");
+ /** Matches a path starting with a drive letter, like /d/whatever or
/C/... */
+ private static final Pattern MSYS_PATH =
Pattern.compile("^/([a-zA-Z])/(.*)$");
+
+ /**
+ * Converts a POSIX-style Windows path as used by MSYS2 and Git Bash,
+ * e.g. "/c/Users/alice/file.txt", to a native Windows path
"C:\Users\alice\file.txt".
+ * Returns the original string if it is not such a path, a native Windows
path for instance.
+ */
+ static String msysToWindowsPath(final String path) {
+ if (path == null) {
+ return null;
+ }
+ final Matcher matcher = MSYS_PATH.matcher(path);
+ if (matcher.matches()) {
+ return Character.toUpperCase(matcher.group(1).charAt(0)) + ":\\"
+ + matcher.group(2).replace('/', '\\');
+ }
+ return path;
+ }
Review Comment:
This MSYS conversion logic duplicates the newly added `MsysPaths` utility
(same regex and transformation), which risks divergence over time (bug
fixes/edge cases applied in one place but not the other). If feasible across
modules, centralize path normalization in a single shared utility; otherwise,
at least align behavior/tests explicitly (e.g., add a shared test matrix or
document why the implementations must remain separate).
##########
compat/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -791,6 +795,63 @@ public void calculateTransferListener(boolean ciEnv,
String[] cliArgs, Class<Tra
assertEquals(expectedSubClass, transferListener.getClass());
}
+ /**
+ * “/d/…” and “/c/…” should convert to proper Windows drive paths.
+ */
+ @Test
+ void msysPathsAreConverted() {
+ assertEquals(
+ "D:/projects/foo",
MavenCli.msysToWindowsPath("/d/projects/foo").replace('\\', '/'));
+ assertEquals("C:/x", MavenCli.msysToWindowsPath("/c/x").replace('\\',
'/'));
+ }
+
+ /**
+ * Only a path starting with a drive letter is converted.
+ */
+ @Test
+ void otherPathsAreKept() {
+ assertNull(MavenCli.msysToWindowsPath(null));
+ assertEquals("", MavenCli.msysToWindowsPath(""));
+ assertEquals("/tmp/repo", MavenCli.msysToWindowsPath("/tmp/repo"));
+ assertEquals("/c", MavenCli.msysToWindowsPath("/c"));
+ assertEquals("C:\\Users\\alice",
MavenCli.msysToWindowsPath("C:\\Users\\alice"));
+ assertEquals("relative/path",
MavenCli.msysToWindowsPath("relative/path"));
+ }
+
+ /**
+ * Full `initialize` run: ensures the JVM properties are rewritten when
running on Windows.
+ */
+ @Test
+ @EnabledOnOs(OS.WINDOWS)
+ void initializeNormalisesUserHome(@TempDir Path tmpDir) throws Exception {
+ // Backup current system properties to restore afterwards
+ final Properties backup = (Properties) System.getProperties().clone();
+ try {
+ // Pretend we are on Windows so Os.isFamily("windows") returns true
+ System.setProperty("os.name", "Windows 10");
+
+ // Required by MavenCli.initialize
+ System.setProperty(MavenCli.MULTIMODULE_PROJECT_DIRECTORY,
tmpDir.toString());
+
+ // Inject MSYS‑style paths
+ System.setProperty("user.home", "/d/test/home");
+ System.setProperty(Constants.MAVEN_REPO_LOCAL, "/d/test/repo");
+
+ final MavenCli cli = new MavenCli();
+ final CliRequest req = new CliRequest(new String[0], null);
+ cli.initialize(req);
+
+ // Assertions: both properties should now be real Windows paths
+ assertEquals("D:/test/home",
System.getProperty("user.home").replace('\\', '/'));
+ assertEquals(
+ "D:/test/repo",
+
System.getProperty(Constants.MAVEN_REPO_LOCAL).replace('\\', '/'));
+ } finally {
+ // Restore original system properties
+ System.setProperties(backup);
+ }
+ }
Review Comment:
This test mutates global JVM `System` properties (`os.name`, `user.home`,
etc.). If the test suite runs tests in parallel, this can cause cross-test
interference/flakiness. Consider serializing access using JUnit 5
`@ResourceLock` for system properties (or equivalent project mechanism) at the
test or class level.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/MsysPaths.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cling.invoker;
+
+import java.util.Locale;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Converts the paths MSYS2 and Git Bash hand over in their POSIX style, like
{@code /c/Users/alice}, to native Windows
+ * paths, like {@code C:\Users\alice}.
+ */
+final class MsysPaths {
+
+ /** A drive letter, the only single letter directory right below the root
of such a path. */
+ private static final Pattern DRIVE_PATH =
Pattern.compile("^/([a-zA-Z])/(.*)$");
+
+ private MsysPaths() {}
+
+ /**
+ * Converts the path on Windows, and returns it as it is anywhere else.
+ *
+ * @param path the path, may be {@code null}
+ * @return the native path, {@code null} if the path is
+ */
+ static String toNativePath(String path) {
+ return toNativePath(path, isWindows());
+ }
Review Comment:
The `@return` Javadoc is truncated/incomplete (`{@code null} if the path is
...`). Please complete the sentence to accurately describe the return value
(e.g., '... if the input path is null').
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -125,6 +125,13 @@ public final int invoke(InvokerRequest invokerRequest) {
oldProps.putAll(System.getProperties());
ClassLoader oldCL = Thread.currentThread().getContextClassLoader();
try (C context = createContext(invokerRequest)) {
+ // MSYS2 and Git Bash hand over Windows paths in their POSIX
style, e.g. /c/Users/alice
+ String userHome = System.getProperty("user.home");
+ String nativeUserHome = MsysPaths.toNativePath(userHome);
+ if (!Objects.equals(userHome, nativeUserHome)) {
+ System.setProperty("user.home", nativeUserHome);
+ context.logger.debug("Converted user.home '" + userHome + "'
to '" + nativeUserHome + "'");
Review Comment:
The debug log message is built via string concatenation, which incurs
allocation overhead even when debug logging is disabled. Prefer parameterized
logging (if supported by `context.logger`) or guard with a debug-enabled check
before building the message.
--
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]
