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", + ]