gnodet-bot commented on code in PR #13581:
URL: https://github.com/apache/maven/pull/13581#discussion_r4314207744
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSettingsBuilderFactoryTest.java:
##########
@@ -207,6 +207,39 @@ void testRelativeLocalRepositoryIsResolvedToAbsolute() {
assertTrue(repoPath.isAbsolute(), "Relative local repository should be
resolved to absolute");
}
+ @Test
+ void emptySettingsGetCentralInBothRepoLists() {
+ Settings settings = execute("settings-simple").getEffectiveSettings();
+
+ assertEquals(1, settings.getRepositories().size());
+ assertEquals("central", settings.getRepositories().get(0).getId());
+
+ assertEquals(1, settings.getPluginRepositories().size());
+ assertEquals("central",
settings.getPluginRepositories().get(0).getId());
+ }
+
+ @Test
+ void reposOnlySettingsGetCentralInPluginRepos() {
+ Settings settings =
execute("settings-repos-only").getEffectiveSettings();
+
+ assertEquals(1, settings.getRepositories().size());
+ assertEquals("internal", settings.getRepositories().get(0).getId());
+
+ assertEquals(1, settings.getPluginRepositories().size());
+ assertEquals("central",
settings.getPluginRepositories().get(0).getId());
+ }
+
+ @Test
+ void pluginReposOnlySettingsGetCentralInRepos() {
+ Settings settings =
execute("settings-plugin-repos-only").getEffectiveSettings();
+
+ assertEquals(1, settings.getRepositories().size());
+ assertEquals("central", settings.getRepositories().get(0).getId());
+
+ assertEquals(1, settings.getPluginRepositories().size());
+ assertEquals("internal-plugins",
settings.getPluginRepositories().get(0).getId());
+ }
+
@Test
void testProxyFieldInterpolation() {
Review Comment:
💡 **Consider adding a negative test (both lists defined):**
The three new tests cover: both-empty, repos-only, plugin-repos-only.
There's no test for the case where *both* `<repositories>` and
`<pluginRepositories>` are defined — verifying that central is *not* injected
(and not duplicated) in that case. Low priority, but it rounds out the coverage.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java:
##########
@@ -126,8 +126,11 @@ public SettingsBuilderResult build(SettingsBuilderRequest
request) throws Settin
// If no repository is defined in the user/global settings,
// it means that we have "old" settings (as those are new in 4.0)
// so add central to the computed settings for backward compatibility.
Review Comment:
💡 **Stale comment framing:**
Lines 126–128 still say *"it means that we have 'old' settings"* — but with
the `||` change, the block now fires even for modern settings that simply omit
one of the two lists. The new comment you added (lines 129–131) clarifies the
intent correctly, but the original framing on the lines above now contradicts
it.
Suggestion: replace the old 3-line block with the new one:
```suggestion
// Each list is checked independently: a user may define
<repositories>
// (e.g. an internal mirror) without defining <pluginRepositories>,
and
// plugin resolution still needs central.
```
--
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]
