Copilot commented on code in PR #12631:
URL: https://github.com/apache/maven/pull/12631#discussion_r3683360476
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -128,11 +127,13 @@ public SettingsBuilderResult build(SettingsBuilderRequest
request) throws Settin
.build();
}
- // for the special case of a drive-relative Windows path, make sure
it's absolute to save plugins from trouble
+ // resolve relative local repository paths to absolute to save plugins
from trouble.
+ // paths containing property placeholders like ${user.home} must be
left as-is
+ // so that later interpolation can resolve them.
String localRepository = effective.getLocalRepository();
if (localRepository != null && !localRepository.isEmpty()) {
Path file = Paths.get(localRepository);
- if (!file.isAbsolute() &&
file.toString().startsWith(File.separator)) {
+ if (!file.isAbsolute() && !localRepository.contains("${")) {
effective =
effective.withLocalRepository(file.toAbsolutePath().toString());
}
}
Review Comment:
The placeholder check is applied after `Paths.get(localRepository)`, but the
intent (per comment) is to leave placeholder-based paths entirely untouched.
Consider moving `!localRepository.contains(\"${\")` into the outer guard
(before constructing the `Path`) so placeholder paths are skipped earlier and
the normalization logic is more clearly aligned with the documented behavior.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -160,6 +161,17 @@ void testSettingsWithDuplicateServersIds() throws
Exception {
problems.problems().findFirst().orElseThrow().getMessage());
}
+ @Test
+ void testRelativeLocalRepositoryIsResolvedToAbsolute() {
+ Settings settings =
execute("settings-relative-local-repo").getEffectiveSettings();
+
+ String localRepository = settings.getLocalRepository();
+ assertNotNull(localRepository);
+ assertFalse(localRepository.isEmpty());
+ Path repoPath = Paths.get(localRepository);
+ assertTrue(repoPath.isAbsolute(), "Relative local repository should be
resolved to absolute");
+ }
Review Comment:
This assertion only checks that the resulting path is absolute, which could
still pass if the setting is ignored and a default absolute local repository is
used. To more directly validate the new behavior, assert that the resolved path
matches `Paths.get(\"relative/repo\").toAbsolutePath()` (or at least that it
ends with the expected `relative/repo` path) to ensure the configured relative
value was actually used and normalized.
--
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]
