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>
diff --git a/command.py b/command.py index 8b6a4d1..3086e13 100644 --- a/command.py +++ b/command.py
@@ -17,7 +17,7 @@ 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 @@ 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 @@ 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 @@ ) 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 @@ 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 @@ "%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 d7938c5..ef6f7a0 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 @@ ): 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 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 @@ 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", + ]