Copilot commented on code in PR #2224:
URL: https://github.com/apache/maven-resolver/pull/2224#discussion_r4349954932
##########
maven-resolver-api/src/test/java/org/eclipse/aether/repository/LocalRepositoryTest.java:
##########
@@ -49,4 +54,33 @@ void allSomePaths() {
private void doIt(File file) {
new LocalRepository(file.toPath().toUri().toASCIIString());
}
+
+ /**
+ * Verifies that a plain Windows path string (e.g. {@code
C:\Users\foo\repo}) passed to
+ * {@link LocalRepository#LocalRepository(String)} preserves the drive
letter and does not
+ * resolve against the JVM working directory. Previously, {@code
RepositoryUriUtils.basedir()}
+ * treated the drive letter as a URL scheme and stripped it, producing a
drive-relative path
+ * ({@code \Users\foo\repo}) that {@code Path.of(...).toUri()} then
resolved against the JVM
+ * working directory — which can be on a different drive on Windows CI
runners.
+ */
+ @Test
+ @EnabledOnOs(OS.WINDOWS)
+ void windowsPlainPathPreservesDriveLetter() {
Review Comment:
Given the fix special-cases drive-letter parsing, consider adding a second
assertion/case in this Windows test for a lowercase drive letter input (e.g.,
`c:\\...`) and/or forward-slash form (`C:/...`). This helps prevent regressions
where drive-letter detection only works for one casing or separator style.
##########
maven-resolver-api/src/test/java/org/eclipse/aether/repository/LocalRepositoryTest.java:
##########
@@ -49,4 +54,33 @@ void allSomePaths() {
private void doIt(File file) {
new LocalRepository(file.toPath().toUri().toASCIIString());
}
+
+ /**
+ * Verifies that a plain Windows path string (e.g. {@code
C:\Users\foo\repo}) passed to
+ * {@link LocalRepository#LocalRepository(String)} preserves the drive
letter and does not
+ * resolve against the JVM working directory. Previously, {@code
RepositoryUriUtils.basedir()}
+ * treated the drive letter as a URL scheme and stripped it, producing a
drive-relative path
+ * ({@code \Users\foo\repo}) that {@code Path.of(...).toUri()} then
resolved against the JVM
+ * working directory — which can be on a different drive on Windows CI
runners.
+ */
+ @Test
+ @EnabledOnOs(OS.WINDOWS)
+ void windowsPlainPathPreservesDriveLetter() {
+ // Use the current drive letter so the path is always valid on any
Windows machine.
+ String currentDrive =
Paths.get(".").toAbsolutePath().getRoot().toString(); // e.g. "C:\"
+ char driveLetter = currentDrive.charAt(0);
+ String plainPath = driveLetter +
":\\Users\\runneradmin\\AppData\\Local\\Temp\\my-repo";
+
+ LocalRepository repo = new LocalRepository(plainPath);
+ String repoPath = repo.getBasePath().toString();
Review Comment:
The test hard-codes a user/profile-specific subpath
(`\\Users\\runneradmin\\...`) and later asserts full string equality. Even if
the repository doesn’t need to exist, string round-tripping can differ due to
normalization (separator normalization, case normalization, etc.), which risks
flakiness across Windows environments. Prefer building the path from an
OS-provided base (e.g., `java.io.tmpdir` /
`Paths.get(System.getProperty(\"java.io.tmpdir\"))`) and asserting using `Path`
equality/normalization (e.g., compare `Paths.get(plainPath)` to
`repo.getBasePath()`), while still separately asserting the drive letter is
preserved.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/repository/RepositoryUriUtils.java:
##########
@@ -76,7 +78,15 @@ private static String basedir(String url) {
String retValue;
if (!protocol.isEmpty()) {
- retValue = url.substring(protocol.length() + 1);
+ // A single uppercase letter is a Windows drive letter (e.g.
"C:\path"), not a URL scheme.
+ // Keep it in the path; do not strip it as if it were a protocol
prefix.
+ if (protocol.length() == 1
+ && Character.isLetter(protocol.charAt(0))
+ && Character.isUpperCase(protocol.charAt(0))) {
+ retValue = url;
+ } else {
+ retValue = url.substring(protocol.length() + 1);
+ }
} else {
Review Comment:
Drive-letter detection is currently limited to *uppercase* single-letter
prefixes. Windows paths can be provided as `c:\\...` as well; in that case this
code will still treat `c` as a URL scheme and strip the drive letter
(reintroducing the original bug). Consider detecting drive letters by pattern
(e.g., `^[A-Za-z]:[\\\\/].*`) rather than `isUpperCase`, and apply the same
predicate consistently in both `basedir()` and the `toUri()` protocol-branch
condition.
##########
maven-resolver-api/src/test/java/org/eclipse/aether/repository/LocalRepositoryTest.java:
##########
@@ -49,4 +54,33 @@ void allSomePaths() {
private void doIt(File file) {
new LocalRepository(file.toPath().toUri().toASCIIString());
}
+
+ /**
+ * Verifies that a plain Windows path string (e.g. {@code
C:\Users\foo\repo}) passed to
+ * {@link LocalRepository#LocalRepository(String)} preserves the drive
letter and does not
+ * resolve against the JVM working directory. Previously, {@code
RepositoryUriUtils.basedir()}
+ * treated the drive letter as a URL scheme and stripped it, producing a
drive-relative path
+ * ({@code \Users\foo\repo}) that {@code Path.of(...).toUri()} then
resolved against the JVM
+ * working directory — which can be on a different drive on Windows CI
runners.
+ */
+ @Test
+ @EnabledOnOs(OS.WINDOWS)
+ void windowsPlainPathPreservesDriveLetter() {
+ // Use the current drive letter so the path is always valid on any
Windows machine.
+ String currentDrive =
Paths.get(".").toAbsolutePath().getRoot().toString(); // e.g. "C:\"
+ char driveLetter = currentDrive.charAt(0);
+ String plainPath = driveLetter +
":\\Users\\runneradmin\\AppData\\Local\\Temp\\my-repo";
+
+ LocalRepository repo = new LocalRepository(plainPath);
+ String repoPath = repo.getBasePath().toString();
+
+ // The resolved path must start with the same drive letter, not a
different one.
+ assertEquals(
+ String.valueOf(driveLetter),
+ repoPath.substring(0, 1).toUpperCase(),
+ "Drive letter must be preserved when parsing a plain Windows
path: " + repoPath);
Review Comment:
`String#toUpperCase()` is locale-sensitive; in uncommon locales this can
produce unexpected results for certain letters. Since this is a single
character comparison, prefer a locale-independent approach (e.g.,
`Character.toUpperCase(...)`) or use `toUpperCase(Locale.ROOT)` to keep the
assertion deterministic.
--
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]
