Address comments

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java

@@ -208,7 +208,7 @@ private static ImmutableTable<ModuleExtensionId, String, RepositoryName> resolve
       ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesTable,
       ImmutableMap<ModuleExtensionId, String> extensionUniqueNames,
       ImmutableBiMap<RepositoryName, ModuleKey> canonicalRepoNameLookup) {
-    RepositoryMapping rootModuleMapping =
+    RepositoryMapping rootModuleMappingWithoutOverrides =
         BazelDepGraphValue.getRepositoryMapping(
             ModuleKey.ROOT,
             depGraph,
@@ -228,7 +228,7 @@ private static ImmutableTable<ModuleExtensionId, String, RepositoryName> resolve
           repoOverridesBuilder.put(
               extensionId,
               override.getKey(),
-              rootModuleMapping.get(override.getValue().overridingRepoName()));
+              rootModuleMappingWithoutOverrides.get(override.getValue().overridingRepoName()));
         }
       }
     }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphValue.java

@@ -108,7 +108,7 @@ public static BazelDepGraphValue createEmptyDepGraph() {
 
   /**
    * For each module extension, a mapping from the name of the repo exported by the extension to the
-   * canonical repo name of the repo that should override it (if any).
+   * canonical name of the repo that should override it (if any).
    */
   public abstract ImmutableTable<ModuleExtensionId, String, RepositoryName> getRepoOverrides();
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java

@@ -131,26 +131,29 @@ public final boolean getHasNonDevUseExtension() {
     return getProxies().stream().anyMatch(p -> !p.isDevDependency());
   }
 
+  /**
+   * Represents a repo that overrides another repo within the scope of the extension.
+   *
+   * @param overridingRepoName The apparent name of the overriding repo in the root module.
+   * @param mustExist Whether this override should apply to an existing repo.
+   * @param location The location of the {@code override_repo} or {@code inject_repo} call.
+   */
   @GenerateTypeAdapter
-  public record RepoOverride(
-      // The apparent name of the overriding repo in the root module.
-      String overridingRepoName,
-      // Whether this override should apply to an existing repo.
-      boolean mustExist,
-      Location location) {}
+  public record RepoOverride(String overridingRepoName, boolean mustExist, Location location) {}
 
   /**
-   * Maps repo names local to the extension to repo they are overridden by.
+   * Contains information about overrides that apply to repos generated by this extension. Keyed by
+   * the extension-local repo name.
    *
    * <p>This is only non-empty for root module usages and repos that override other repos are not
-   * themselves overridden.
+   * themselves overridden, so overrides do need to be applied recursively.
    */
   public abstract ImmutableMap<String, RepoOverride> getRepoOverrides();
 
   public abstract Builder toBuilder();
 
   public static Builder builder() {
-    return new AutoValue_ModuleExtensionUsage.Builder().setRepoOverrides(ImmutableMap.of());
+    return new AutoValue_ModuleExtensionUsage.Builder();
   }
 
   /**
@@ -208,8 +211,7 @@ public Builder addTag(Tag value) {
     }
 
     @CanIgnoreReturnValue
-    public abstract Builder setRepoOverrides(
-        ImmutableMap<String, RepoOverride> extensionLocalToApparentName);
+    public abstract Builder setRepoOverrides(ImmutableMap<String, RepoOverride> repoOverrides);
 
     public abstract ModuleExtensionUsage build();
   }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

@@ -648,12 +648,13 @@ public void useRepo(
   @StarlarkMethod(
       name = "override_repo",
       doc =
-          "Overrides one or more repos defined by the given module extension with the given repos"
-              + " visible to the current module. This is ignored if the current module is not the"
-              + " root module or `--ignore_dev_dependency` is enabled."
-              + ""
-              + "<p>Use <a href=\"#inject_repo\"><code>inject_repo</code></a> instead to add a new"
-              + " repo.",
+          """
+          Overrides one or more repos defined by the given module extension with the given repos
+          visible to the current module. This is ignored if the current module is not the root
+          module or `--ignore_dev_dependency` is enabled.
+
+          <p>Use <a href="#inject_repo"><code>inject_repo</code></a> instead to add a new repo.
+          """,
       parameters = {
         @Param(
             name = "extension_proxy",
@@ -663,15 +664,17 @@ public void useRepo(
           @Param(
               name = "args",
               doc =
-                  "The repos in the extension that should be overridden with the repos of the same"
-                      + " name in the current module."),
+                  """
+                  The repos in the extension that should be overridden with the repos of the same
+                  name in the current module."""),
       extraKeywords =
           @Param(
               name = "kwargs",
               doc =
-                  "The overrides to apply to the repos generated by the extension, where the values"
-                      + " are the names of repos in the scope of the current module and the keys"
-                      + " are the names of the repos they will override in the extension."),
+                  """
+                  The overrides to apply to the repos generated by the extension, where the values
+                  are the names of repos in the scope of the current module and the keys are the
+                  names of the repos they will override in the extension."""),
       useStarlarkThread = true)
   public void overrideRepo(
       ModuleExtensionProxy extensionProxy,
@@ -699,12 +702,12 @@ public void overrideRepo(
   @StarlarkMethod(
       name = "inject_repo",
       doc =
-          "Injects one or more new repos into the given module extension."
-              + " This is ignored if the current module is not the root module or"
-              + " `--ignore_dev_dependency` is enabled."
-              + ""
-              + "<p>Use <a href=\"#override_repo\"><code>override_repo</code></a> instead to"
-              + " override an existing repo.",
+          """
+          Injects one or more new repos into the given module extension.
+          This is ignored if the current module is not the root module or `--ignore_dev_dependency`
+          is enabled.
+          <p>Use <a href="#override_repo"><code>override_repo</code></a> instead to override an
+          existing repo.""",
       parameters = {
         @Param(
             name = "extension_proxy",
@@ -714,15 +717,17 @@ public void overrideRepo(
           @Param(
               name = "args",
               doc =
-                  "The repos visible to the current module that should be injected into the"
-                      + " extension under the same name."),
+                  """
+                  The repos visible to the current module that should be injected into the
+                  extension under the same name."""),
       extraKeywords =
           @Param(
               name = "kwargs",
               doc =
-                  "The new repos to inject into the extension, where the values are the names of"
-                      + " repos in the scope of the current module and the keys are the name they"
-                      + " will be visible under in the extension."),
+                  """
+                  The new repos to inject into the extension, where the values are the names of
+                  repos in the scope of the current module and the keys are the name they will be
+                  visible under in the extension."""),
       useStarlarkThread = true)
   public void injectRepo(
       ModuleExtensionProxy extensionProxy,

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleThreadContext.java

@@ -83,7 +83,7 @@ record RepoOverride(
       String overriddenRepoName,
       String overridingRepoName,
       boolean mustExist,
-      ModuleExtensionUsageBuilder extensionUsageBuilder,
+      String extensionName,
       ImmutableList<StarlarkThread.CallStackEntry> stack) {
     Location location() {
       // Skip over the override_repo builtin frame.
@@ -201,7 +201,8 @@ public void addRepoOverride(
       RepoOverride collision =
           repoOverrides.put(
               overriddenRepoName,
-              new RepoOverride(overriddenRepoName, overridingRepoName, mustExist, this, stack));
+              new RepoOverride(
+                  overriddenRepoName, overridingRepoName, mustExist, extensionName, stack));
       if (collision != null) {
         throw Starlark.errorf(
             "The repo exported as '%s' by module extension '%s' is already overridden with '%s' at"
@@ -334,31 +335,15 @@ public InterimModule buildModule(@Nullable Registry registry) throws EvalExcepti
     if (overridingAndOverridden.isPresent()) {
       var override = overridingRepos.get(overridingAndOverridden.get());
       var overrideOnOverride = overriddenRepos.get(overridingAndOverridden.get());
-      if (override.overriddenRepoName.equals(
-          override.extensionUsageBuilder.imports.get(overrideOnOverride.overridingRepoName))) {
-        throw Starlark.errorf(
-                "The repo '%s' used as an override for '%s' in module extension '%s' is itself"
-                    + " overridden with '%s' at %s, which forms a cycle.",
-                override.overridingRepoName,
-                override.overriddenRepoName,
-                override.extensionUsageBuilder.extensionName,
-                overrideOnOverride.overridingRepoName,
-                overrideOnOverride.location())
-            .withCallStack(override.stack);
-      } else {
-        throw Starlark.errorf(
-                "The repo '%s' used as an override for '%s' in module extension '%s' is itself"
-                    + " overridden with '%s' at %s, which is not supported. Please directly"
-                    + " override '%s' with '%s' instead.",
-                override.overridingRepoName,
-                override.overriddenRepoName,
-                override.extensionUsageBuilder.extensionName,
-                overrideOnOverride.overridingRepoName,
-                overrideOnOverride.location(),
-                override.overriddenRepoName,
-                overrideOnOverride.overridingRepoName)
-            .withCallStack(override.stack);
-      }
+      throw Starlark.errorf(
+              "The repo '%s' used as an override for '%s' in module extension '%s' is itself"
+                  + " overridden with '%s' at %s, which is not supported.",
+              override.overridingRepoName,
+              override.overriddenRepoName,
+              override.extensionName,
+              overrideOnOverride.overridingRepoName,
+              overrideOnOverride.location())
+          .withCallStack(override.stack);
     }
 
     return module

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionUsagesValue.java

@@ -57,6 +57,7 @@ public abstract class SingleExtensionUsagesValue implements SkyValue {
   /** The repo mappings to use for each module that used this extension. */
   public abstract ImmutableMap<ModuleKey, RepositoryMapping> getRepoMappings();
 
+  /** Maps an extension-local repo name to the canonical name of the repo it is overridden with. */
   public abstract ImmutableMap<String, RepositoryName> getRepoOverrides();
 
   public static SingleExtensionUsagesValue create(

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java

@@ -220,6 +220,7 @@ private static ModuleExtensionUsage createModuleExtensionUsage(
         .setExtensionBzlFile(bzlFile)
         .setExtensionName(name)
         .setIsolationKey(Optional.empty())
+        .setRepoOverrides(ImmutableMap.of())
         .addProxy(
             ModuleExtensionUsage.Proxy.builder()
                 .setDevDependency(false)

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java

@@ -776,6 +776,7 @@ public void testModuleExtensions_good() throws Exception {
                         .setExtensionBzlFile("@mymod//:defs.bzl")
                         .setExtensionName("myext1")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -806,6 +807,7 @@ public void testModuleExtensions_good() throws Exception {
                         .setExtensionBzlFile("@mymod//:defs.bzl")
                         .setExtensionName("myext2")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -850,6 +852,7 @@ public void testModuleExtensions_good() throws Exception {
                         .setExtensionBzlFile("@rules_jvm_external//:defs.bzl")
                         .setExtensionName("maven")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -927,6 +930,7 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception {
                         .setExtensionBzlFile("@//:defs.bzl")
                         .setExtensionName("myext")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -1058,6 +1062,7 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception {
                         .setExtensionBzlFile("@mymod//:defs.bzl")
                         .setExtensionName("myext")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -1177,6 +1182,7 @@ public void testModuleExtensions_innate() throws Exception {
                         .setExtensionBzlFile("//:MODULE.bazel")
                         .setExtensionName("_repo_rules")
                         .setIsolationKey(Optional.empty())
+                        .setRepoOverrides(ImmutableMap.of())
                         .addProxy(
                             ModuleExtensionUsage.Proxy.builder()
                                 .setLocation(
@@ -1781,7 +1787,7 @@ public void testOverrideRepo_chain_singleExtension() throws Exception {
             ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last):
             \tFile "/workspace/MODULE.bazel", line 5, column 14, in <toplevel>
             \t\toverride_repo(ext, baz = "bar")
-            Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead.""");
+            Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported.""");
   }
 
   @Test
@@ -1809,7 +1815,7 @@ public void testOverrideRepo_chain_multipleExtensions() throws Exception {
             ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last):
             \tFile "/workspace/MODULE.bazel", line 5, column 14, in <toplevel>
             \t\toverride_repo(ext1, baz = "bar")
-            Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported. Please directly override 'baz' with 'override' instead.""");
+            Error in override_repo: The repo 'bar' used as an override for 'baz' in module extension 'ext1' is itself overridden with 'override' at /workspace/MODULE.bazel:6:14, which is not supported.""");
   }
 
   @Test
@@ -1835,7 +1841,7 @@ public void testOverrideRepo_cycle_singleExtension() throws Exception {
             ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last):
             \tFile "/workspace/MODULE.bazel", line 5, column 14, in <toplevel>
             \t\toverride_repo(ext, foo = "my_bar", bar = "my_foo")
-            Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which forms a cycle.""");
+            Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:5:14, which is not supported.""");
   }
 
   @Test
@@ -1863,6 +1869,6 @@ public void testOverrideRepo_cycle_multipleExtensions() throws Exception {
             ERROR /workspace/MODULE.bazel:5:14: Traceback (most recent call last):
             \tFile "/workspace/MODULE.bazel", line 5, column 14, in <toplevel>
             \t\toverride_repo(ext2, bar = "my_foo")
-            Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which forms a cycle.""");
+            Error in override_repo: The repo 'my_foo' used as an override for 'bar' in module extension 'ext2' is itself overridden with 'my_bar' at /workspace/MODULE.bazel:4:14, which is not supported.""");
   }
 }

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java

@@ -46,7 +46,8 @@ private static ModuleExtensionUsage.Builder getBaseUsageBuilder() {
     return ModuleExtensionUsage.builder()
         .setExtensionBzlFile("//:rje.bzl")
         .setExtensionName("maven")
-        .setIsolationKey(Optional.empty());
+        .setIsolationKey(Optional.empty())
+        .setRepoOverrides(ImmutableMap.of());
   }
 
   /** A builder for ModuleExtension that sets all the mandatory but irrelevant fields. */

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/modcommand/ModExecutorTest.java

@@ -607,6 +607,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception {
                 ModuleExtensionUsage.builder()
                     .setExtensionBzlFile("//extensions:extensions.bzl")
                     .setExtensionName("maven")
+                    .setRepoOverrides(ImmutableMap.of())
                     .addProxy(
                         ModuleExtensionUsage.Proxy.builder()
                             .setLocation(Location.fromFileLineColumn("[email protected]/MODULE.bazel", 2, 23))
@@ -621,6 +622,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception {
                 ModuleExtensionUsage.builder()
                     .setExtensionBzlFile("//extensions:extensions.bzl")
                     .setExtensionName("maven")
+                    .setRepoOverrides(ImmutableMap.of())
                     .addProxy(
                         ModuleExtensionUsage.Proxy.builder()
                             .setLocation(Location.fromFileLineColumn("[email protected]/MODULE.bazel", 1, 10))
@@ -635,6 +637,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception {
                 ModuleExtensionUsage.builder()
                     .setExtensionBzlFile("//extensions:extensions.bzl")
                     .setExtensionName("gradle")
+                    .setRepoOverrides(ImmutableMap.of())
                     .addProxy(
                         ModuleExtensionUsage.Proxy.builder()
                             .setLocation(Location.fromFileLineColumn("[email protected]/MODULE.bazel", 2, 13))
@@ -649,6 +652,7 @@ public void testExtensionsInfoTextAndGraph() throws Exception {
                 ModuleExtensionUsage.builder()
                     .setExtensionBzlFile("//extensions:extensions.bzl")
                     .setExtensionName("maven")
+                    .setRepoOverrides(ImmutableMap.of())
                     .addProxy(
                         ModuleExtensionUsage.Proxy.builder()
                             .setLocation(Location.fromFileLineColumn("[email protected]/MODULE.bazel", 13, 10))