command: fix group selection across submanifests

When GetProjects walks all manifests without explicit groups, it
resolves the effective groups once from the selected manifest and reuses
them for projects from child manifests. This can ignore a child
manifest's own group selection and include projects that should be
excluded.

When no groups are specified, resolve them from each project's owning
manifest. Explicit groups still apply to all projects in the call, and
single-manifest behavior remains unchanged.

Add regression tests for per-manifest group selection, explicit
overrides, and name lookup across manifests.

Bug: 382086294
Change-Id: Id5c7e52410431b86b395f489cfd494ca05620975
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/624821
Reviewed-by: Mike Frysinger <vapier@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Victor Pushkarev <corvinus.v@gmail.com>
Tested-by: Victor Pushkarev <corvinus.v@gmail.com>

2 files changed, 180 insertions, 13 deletions

diff --git a/command.py b/command.py
index 8b6a4d170..3086e13a2 100644
--- a/command.py
+++ b/command.py
@@ -17,7 +17,7 @@ import multiprocessing
 import optparse
 import os
 import re
-from typing import TYPE_CHECKING
+from typing import List, TYPE_CHECKING
 
 from error import InvalidProjectGroupsError
 from error import NoSuchProjectError
@@ -398,7 +398,11 @@ class Command:
         Args:
             args: a list of (case-insensitive) strings, projects to search for.
             manifest: an XmlManifest, the manifest to use, or None for default.
-            groups: a string, the manifest groups in use.
+            groups: a string, the manifest group selection to apply.
+                Non-empty values apply to all candidate projects in this call.
+                When empty or omitted, single-manifest calls use the selected
+                manifest's effective groups; all-manifest calls use each
+                candidate project's owning manifest's effective groups.
             missing_ok: a boolean, whether to allow missing projects.
             submodules_ok: whether to allow submodules. True allows them for
                 all projects, False disallows them for all projects, and None
@@ -425,9 +429,33 @@ class Command:
                 return project.sync_s
             return submodules_ok
 
-        if not groups:
-            groups = manifest.GetManifestGroupsStr()
-        groups = [x for x in re.split(r"[,\s]+", groups) if x]
+        def parse_groups(value: str) -> List[str]:
+            return [x for x in re.split(r"[,\s]+", value) if x]
+
+        if groups:
+            groups_for_all_projects = parse_groups(groups)
+        elif all_manifests:
+            # In all-manifest mode, each project uses its owning
+            # manifest's effective groups.
+            groups_for_all_projects = None
+        else:
+            groups_for_all_projects = parse_groups(
+                manifest.GetManifestGroupsStr()
+            )
+
+        groups_by_manifest = {}
+
+        def matches_groups(project: "Project") -> bool:
+            if groups_for_all_projects is not None:
+                return project.MatchesGroups(groups_for_all_projects)
+
+            project_manifest = project.manifest
+            if project_manifest not in groups_by_manifest:
+                groups_by_manifest[project_manifest] = parse_groups(
+                    project_manifest.GetManifestGroupsStr()
+                )
+
+            return project.MatchesGroups(groups_by_manifest[project_manifest])
 
         if not args:
             derived_projects = {}
@@ -439,9 +467,7 @@ class Command:
                     )
             all_projects_list.extend(derived_projects.values())
             for project in all_projects_list:
-                if (missing_ok or project.Exists) and project.MatchesGroups(
-                    groups
-                ):
+                if (missing_ok or project.Exists) and matches_groups(project):
                     result.append(project)
         else:
             self._ResetPathToProjectMap(all_projects_list)
@@ -455,7 +481,7 @@ class Command:
                     for project in manifest.GetProjectsWithName(
                         arg, all_manifests=all_manifests
                     )
-                    if project.MatchesGroups(groups)
+                    if matches_groups(project)
                 ]
 
                 if not projects:
@@ -498,7 +524,7 @@ class Command:
                             "%s (%s)"
                             % (arg, project.RelPath(local=not all_manifests))
                         )
-                    if not project.MatchesGroups(groups):
+                    if not matches_groups(project):
                         raise InvalidProjectGroupsError(arg)
 
                 result.extend(projects)
diff --git a/tests/test_command.py b/tests/test_command.py
index d7938c5f2..ef6f7a031 100644
--- a/tests/test_command.py
+++ b/tests/test_command.py
@@ -14,6 +14,8 @@
 
 """Unittests for the command.py module."""
 
+from typing import Iterable, List, Optional
+
 import pytest
 
 from command import Command
@@ -33,6 +35,8 @@ class FakeProject:
     ):
         self.name = name
         self.relpath = relpath
+        self.worktree = f"/work/{relpath}"
+        self.manifest = None
         self.gitdir = gitdir or f"/git/{relpath}"
         self.sync_s = sync_s
         self.Exists = True
@@ -51,11 +55,58 @@ class FakeProject:
 class FakeManifest:
     """Minimal manifest double for Command.GetProjects tests."""
 
-    def __init__(self, projects):
-        self.projects = projects
+    def __init__(
+        self,
+        projects: Iterable[FakeProject],
+        *,
+        all_projects: Optional[Iterable[FakeProject]] = None,
+        effective_groups: str = "default",
+    ):
+        self.projects = list(projects)
+        self.all_projects = (
+            list(self.projects) if all_projects is None else list(all_projects)
+        )
+        self._effective_groups = effective_groups
+
+        # all_projects may include projects owned by child manifests,
+        # so only set this manifest on its direct projects.
+        for project in self.projects:
+            self._set_project_manifest(project)
+
+    def _set_project_manifest(self, project: FakeProject) -> None:
+        project.manifest = self
+        for subproject in project.GetDerivedSubprojects():
+            self._set_project_manifest(subproject)
 
     def GetManifestGroupsStr(self):
-        return "default"
+        return self._effective_groups
+
+    def GetProjectsWithName(
+        self, name: str, all_manifests: bool = False
+    ) -> List[FakeProject]:
+        projects = self.all_projects if all_manifests else self.projects
+        return [project for project in projects if project.name == name]
+
+
+class GroupMatchingFakeProject(FakeProject):
+    """Fake project with predictable group matches for GetProjects tests.
+
+    This lets the tests check which groups GetProjects uses without
+    reimplementing Project.MatchesGroups.
+    """
+
+    def __init__(
+        self,
+        name: str,
+        relpath: str,
+        *,
+        matching_groups: Iterable[str],
+    ):
+        super().__init__(name, relpath)
+        self._matching_groups = set(matching_groups)
+
+    def MatchesGroups(self, groups: Iterable[str]) -> bool:
+        return bool(self._matching_groups.intersection(groups))
 
 
 def test_get_projects_keeps_derived_subprojects_for_repeated_repo():
@@ -117,3 +168,93 @@ def test_get_projects_submodule_override(
     projects = cmd.GetProjects([], submodules_ok=submodules_ok)
 
     assert (submodule in projects) is includes_submodule
+
+
+@pytest.mark.parametrize(
+    ("groups", "expected_relpaths"),
+    [
+        (None, ["outer", "sub/child"]),
+        ("", ["outer", "sub/child"]),
+        ("override-group", ["sub/override"]),
+    ],
+    ids=("groups-omitted", "groups-empty", "explicit-override"),
+)
+def test_get_projects_uses_groups_from_each_manifest_unless_overridden(
+    groups: Optional[str],
+    expected_relpaths: List[str],
+) -> None:
+    """Use each manifest's effective groups unless the caller overrides them."""
+    outer_project = GroupMatchingFakeProject(
+        "outer",
+        "outer",
+        matching_groups={"outer-group"},
+    )
+
+    # Both child projects also match "outer". Reusing the outer manifest's
+    # groups would therefore select both child projects.
+    child_project = GroupMatchingFakeProject(
+        "child",
+        "sub/child",
+        matching_groups={"outer-group", "child-group"},
+    )
+    override_project = GroupMatchingFakeProject(
+        "override",
+        "sub/override",
+        matching_groups={"outer-group", "override-group"},
+    )
+
+    child_manifest = FakeManifest(
+        [child_project, override_project],
+        effective_groups="child-group",
+    )
+    outer_manifest = FakeManifest(
+        [outer_project],
+        all_projects=[outer_project, *child_manifest.projects],
+        effective_groups="outer-group",
+    )
+    cmd = Command(manifest=outer_manifest)
+
+    projects = cmd.GetProjects(
+        [],
+        manifest=outer_manifest,
+        groups=groups,
+        all_manifests=True,
+    )
+
+    assert [project.relpath for project in projects] == expected_relpaths
+
+
+def test_get_projects_by_name_uses_groups_from_each_manifest() -> None:
+    """Name matches use the groups from each project's owning manifest."""
+    outer_project = GroupMatchingFakeProject(
+        "shared",
+        "outer/shared",
+        matching_groups={"outer-group"},
+    )
+    child_project = GroupMatchingFakeProject(
+        "shared",
+        "sub/shared",
+        matching_groups={"child-group"},
+    )
+
+    child_manifest = FakeManifest(
+        [child_project],
+        effective_groups="child-group",
+    )
+    outer_manifest = FakeManifest(
+        [outer_project],
+        all_projects=[outer_project, *child_manifest.projects],
+        effective_groups="outer-group",
+    )
+    cmd = Command(manifest=outer_manifest)
+
+    projects = cmd.GetProjects(
+        ["shared"],
+        manifest=outer_manifest,
+        all_manifests=True,
+    )
+
+    assert [project.relpath for project in projects] == [
+        "outer/shared",
+        "sub/shared",
+    ]