Fix #12604: DefaultSettingsBuilder dead Windows drive-relative path handling - #12631
Conversation
…andling
Remove the always-false condition !file.isAbsolute() && file.toString().startsWith(File.separator) which is dead code on all platforms - paths starting with the file separator are already absolute.
Replace with !file.isAbsolute() (skipping paths containing ${} placeholders that need later interpolation). This correctly resolves all relative local repository paths to absolute, not just the never-reached Windows-specific case.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
LGTM ✅
Clean, well-scoped fix that replaces an effectively dead condition with a broader relative-path resolution guard. The new contains("${") check follows established Maven conventions (used in 7+ other places in the codebase), the import cleanup is correct, and the test adequately covers both the happy path and the placeholder-preservation case.
Notes:
- The PR description states the old condition
!file.isAbsolute() && file.toString().startsWith(File.separator)is "always false on all platforms." This is accurate for Unix/Mac. On Windows,Path.isAbsolute()returnsfalsefor drive-relative paths like\foo, so the old condition could theoretically be true — but the new broader condition is a strict superset and an improvement. - The
!localRepository.contains("${")guard is consistent with the same pattern used inDefaultDependencyResolver,DefaultModelBuilder,DefaultModelValidator,DefaultArtifactDescriptorReader, andMavenValidator.
Minor (non-blocking): The new test resource settings-relative-local-repo.xml lacks the ASF license header that sibling files have. RAT excludes src/test/resources*/** so it's not enforced, but adding it would be consistent.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes #12604 by making local-repository path normalization effective again, ensuring relative <localRepository> values become absolute while preserving ${...} placeholders for later interpolation.
Changes:
- Update
DefaultSettingsBuilderto resolve non-absolute, non-placeholder local repository paths to absolute. - Add a test settings XML using a relative
<localRepository>. - Add a JUnit test verifying the effective local repository path becomes absolute.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java | Replaces dead condition with a relative-path resolution rule while skipping ${...} placeholder paths. |
| impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java | Adds a regression test asserting relative local repository paths are resolved to absolute. |
| impl/maven-impl/src/test/resources/settings/settings-relative-local-repo.xml | Adds a minimal settings fixture defining a relative local repository path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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()); | ||
| } | ||
| } |
| @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"); | ||
| } |
Fixes #12604
Problem
The condition
!file.isAbsolute() && file.toString().startsWith(File.separator)is always false on all platforms:/is absolute, so!file.isAbsolute()is false\is absolute (drive-relative), so!file.isAbsolute()is falseThis made the entire local repository path resolution block dead code.
Fix
Replace the impossible condition with
!file.isAbsolute()(skipping paths containing${placeholders that need later interpolation). This correctly resolves relative paths likerelative/repoto absolute.Testing
Added
testRelativeLocalRepositoryIsResolvedToAbsolutewhich loads a settings file with<localRepository>relative/repo</localRepository>and verifies the output path is absolute.Existing test
testSettingsWithServersAndAliasescontinues to pass because paths containing${user.home}placeholders are left untouched.