manifest, project: Store project groups as sets

This helps a lot when including common manifests with groups and they
use extend-project.

Change-Id: Ic574e7d6696139d0eb90d9915e8c7048d5e89c07
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/525323
Reviewed-by: Gavin Mak <[email protected]>
Tested-by: Peter Kjellerstedt <[email protected]>
Reviewed-by: Mike Frysinger <[email protected]>
Commit-Queue: Peter Kjellerstedt <[email protected]>

Author: Peter Kjellerstedt

docs/manifest-format.md

@@ -287,7 +287,7 @@ should be placed.  If not supplied, `revision` is used.
 
 `path` may not be an absolute path or use "." or ".." path components.
 
-Attribute `groups`: List of additional groups to which all projects
+Attribute `groups`: Set of additional groups to which all projects
 in the included submanifest belong. This appends and recurses, meaning
 all projects in submanifests carry all parent submanifest groups.
 Same syntax as the corresponding element of `project`.
@@ -355,7 +355,7 @@ When using `repo upload`, changes will be submitted for code
 review on this branch. If unspecified both here and in the
 default element, `revision` is used instead.
 
-Attribute `groups`: List of groups to which this project belongs,
+Attribute `groups`: Set of groups to which this project belongs,
 whitespace or comma separated.  All projects belong to the group
 "all", and each project automatically belongs to a group of
 its name:`name` and path:`path`.  E.g. for
@@ -403,7 +403,7 @@ of the repo client where the Git working directory for this project
 should be placed.  This is used to move a project in the checkout by
 overriding the existing `path` setting.
 
-Attribute `groups`: List of additional groups to which this project
+Attribute `groups`: Set of additional groups to which this project
 belongs.  Same syntax as the corresponding element of `project`.
 
 Attribute `revision`: If specified, overrides the revision of the original
@@ -572,7 +572,7 @@ the manifest repository's root.
 "name" may not be an absolute path or use "." or ".." path components.
 These restrictions are not enforced for [Local Manifests].
 
-Attribute `groups`: List of additional groups to which all projects
+Attribute `groups`: Set of additional groups to which all projects
 in the included manifest belong. This appends and recurses, meaning
 all projects in included manifests carry all parent include groups.
 This also applies to all extend-project elements in the included manifests.

man/repo-manifest.1

@@ -365,7 +365,7 @@ supplied, `revision` is used.
 .PP
 `path` may not be an absolute path or use "." or ".." path components.
 .PP
-Attribute `groups`: List of additional groups to which all projects in the
+Attribute `groups`: Set of additional groups to which all projects in the
 included submanifest belong. This appends and recurses, meaning all projects in
 submanifests carry all parent submanifest groups. Same syntax as the
 corresponding element of `project`.
@@ -427,7 +427,7 @@ Attribute `dest\-branch`: Name of a Git branch (e.g. `main`). When using `repo
 upload`, changes will be submitted for code review on this branch. If
 unspecified both here and in the default element, `revision` is used instead.
 .PP
-Attribute `groups`: List of groups to which this project belongs, whitespace or
+Attribute `groups`: Set of groups to which this project belongs, whitespace or
 comma separated. All projects belong to the group "all", and each project
 automatically belongs to a group of its name:`name` and path:`path`. E.g. for
 `<project name="monkeys" path="barrel\-of"/>`, that project definition is
@@ -471,8 +471,8 @@ repo client where the Git working directory for this project should be placed.
 This is used to move a project in the checkout by overriding the existing `path`
 setting.
 .PP
-Attribute `groups`: List of additional groups to which this project belongs.
-Same syntax as the corresponding element of `project`.
+Attribute `groups`: Set of additional groups to which this project belongs. Same
+syntax as the corresponding element of `project`.
 .PP
 Attribute `revision`: If specified, overrides the revision of the original
 project. Same syntax as the corresponding element of `project`.
@@ -634,7 +634,7 @@ repository's root.
 "name" may not be an absolute path or use "." or ".." path components. These
 restrictions are not enforced for [Local Manifests].
 .PP
-Attribute `groups`: List of additional groups to which all projects in the
+Attribute `groups`: Set of additional groups to which all projects in the
 included manifest belong. This appends and recurses, meaning all projects in
 included manifests carry all parent include groups. This also applies to all
 extend\-project elements in the included manifests. Same syntax as the

manifest_xml.py

@@ -255,7 +255,7 @@ class _XmlSubmanifest:
       project: a string, the name of the manifest project.
       revision: a string, the commitish.
       manifestName: a string, the submanifest file name.
-      groups: a list of strings, the groups to add to all projects in the
+      groups: a set of strings, the groups to add to all projects in the
           submanifest.
       default_groups: a list of strings, the default groups to sync.
       path: a string, the relative path for the submanifest checkout.
@@ -281,7 +281,7 @@ def __init__(
         self.project = project
         self.revision = revision
         self.manifestName = manifestName
-        self.groups = groups
+        self.groups = groups or set()
         self.default_groups = default_groups
         self.path = path
         self.parent = parent
@@ -304,7 +304,7 @@ def __init__(
         self.repo_client = RepoClient(
             parent.repodir,
             linkFile,
-            parent_groups=",".join(groups) or "",
+            parent_groups=groups,
             submanifest_path=os.path.join(parent.path_prefix, self.relpath),
             outer_client=outer_client,
             default_groups=default_groups,
@@ -345,7 +345,7 @@ def ToSubmanifestSpec(self):
         manifestName = self.manifestName or "default.xml"
         revision = self.revision or self.name
         path = self.path or revision.split("/")[-1]
-        groups = self.groups or []
+        groups = self.groups
 
         return SubmanifestSpec(
             self.name, manifestUrl, manifestName, revision, path, groups
@@ -359,9 +359,7 @@ def relpath(self):
 
     def GetGroupsStr(self):
         """Returns the `groups` given for this submanifest."""
-        if self.groups:
-            return ",".join(self.groups)
-        return ""
+        return ",".join(sorted(self.groups))
 
     def GetDefaultGroupsStr(self):
         """Returns the `default-groups` given for this submanifest."""
@@ -381,7 +379,7 @@ def __init__(self, name, manifestUrl, manifestName, revision, path, groups):
         self.manifestName = manifestName
         self.revision = revision
         self.path = path
-        self.groups = groups or []
+        self.groups = groups
 
 
 class XmlManifest:
@@ -393,7 +391,7 @@ def __init__(
         manifest_file,
         local_manifests=None,
         outer_client=None,
-        parent_groups="",
+        parent_groups=None,
         submanifest_path="",
         default_groups=None,
     ):
@@ -409,7 +407,8 @@ def __init__(
                 manifests. This will usually be
                 |repodir|/|LOCAL_MANIFESTS_DIR_NAME|.
             outer_client: RepoClient of the outer manifest.
-            parent_groups: a string, the groups to apply to this projects.
+            parent_groups: a set of strings, the groups to apply to this
+                manifest.
             submanifest_path: The submanifest root relative to the repo root.
             default_groups: a string, the default manifest groups to use.
         """
@@ -432,7 +431,7 @@ def __init__(
             self.manifestFileOverrides = {}
         self.local_manifests = local_manifests
         self._load_local_manifests = True
-        self.parent_groups = parent_groups
+        self.parent_groups = parent_groups or set()
         self.default_groups = default_groups
 
         if submanifest_path and not outer_client:
@@ -567,6 +566,14 @@ def _ParseList(self, field):
         """
         return [x for x in re.split(r"[,\s]+", field) if x]
 
+    def _ParseSet(self, field):
+        """Parse fields that contain flattened sets.
+
+        These are whitespace & comma separated.  Empty elements will be
+        discarded.
+        """
+        return set(self._ParseList(field))
+
     def ToXml(
         self,
         peg_rev=False,
@@ -725,10 +732,9 @@ def output_project(parent, parent_node, p):
                 le.setAttribute("dest", lf.dest)
                 e.appendChild(le)
 
-            default_groups = ["all", "name:%s" % p.name, "path:%s" % p.relpath]
-            egroups = [g for g in p.groups if g not in default_groups]
+            egroups = p.groups - {"all", f"name:{p.name}", f"path:{p.relpath}"}
             if egroups:
-                e.setAttribute("groups", ",".join(egroups))
+                e.setAttribute("groups", ",".join(sorted(egroups)))
 
             for a in p.annotations:
                 if a.keep == "true":
@@ -1171,12 +1177,12 @@ def _Load(self, initial_client=None, submanifest_depth=0):
                     b = b[len(R_HEADS) :]
                 self.branch = b
 
-                parent_groups = self.parent_groups
+                parent_groups = self.parent_groups.copy()
                 if self.path_prefix:
-                    parent_groups = (
+                    parent_groups |= {
                         f"{SUBMANIFEST_GROUP_PREFIX}:path:"
-                        f"{self.path_prefix},{parent_groups}"
-                    )
+                        f"{self.path_prefix}"
+                    }
 
                 # The manifestFile was specified by the user which is why we
                 # allow include paths to point anywhere.
@@ -1202,16 +1208,16 @@ def _Load(self, initial_client=None, submanifest_depth=0):
                                 # Since local manifests are entirely managed by
                                 # the user, allow them to point anywhere the
                                 # user wants.
-                                local_group = (
+                                local_group = {
                                     f"{LOCAL_MANIFEST_GROUP_PREFIX}:"
                                     f"{local_file[:-4]}"
-                                )
+                                }
                                 nodes.append(
                                     self._ParseManifestXml(
                                         local,
                                         self.subdir,
                                         parent_groups=(
-                                            f"{local_group},{parent_groups}"
+                                            local_group | parent_groups
                                         ),
                                         restrict_includes=False,
                                     )
@@ -1262,7 +1268,7 @@ def _ParseManifestXml(
         self,
         path,
         include_root,
-        parent_groups="",
+        parent_groups=None,
         restrict_includes=True,
         parent_node=None,
     ):
@@ -1271,11 +1277,11 @@ def _ParseManifestXml(
         Args:
             path: The XML file to read & parse.
             include_root: The path to interpret include "name"s relative to.
-            parent_groups: The groups to apply to this projects.
+            parent_groups: The set of groups to apply to this manifest.
             restrict_includes: Whether to constrain the "name" attribute of
                 includes.
-            parent_node: The parent include node, to apply attribute to this
-                projects.
+            parent_node: The parent include node, to apply attributes to this
+                manifest.
 
         Returns:
             List of XML nodes.
@@ -1307,12 +1313,10 @@ def _ParseManifestXml(
                         raise ManifestInvalidPathError(
                             f'<include> invalid "name": {name}: {msg}'
                         )
-                include_groups = ""
-                if parent_groups:
-                    include_groups = parent_groups
+                include_groups = (parent_groups or set()).copy()
                 if node.hasAttribute("groups"):
-                    include_groups = (
-                        node.getAttribute("groups") + "," + include_groups
+                    include_groups |= self._ParseSet(
+                        node.getAttribute("groups")
                     )
                 fp = os.path.join(include_root, name)
                 if not os.path.isfile(fp):
@@ -1339,12 +1343,12 @@ def _ParseManifestXml(
                     "project",
                     "extend-project",
                 ):
-                    nodeGroups = parent_groups
+                    nodeGroups = parent_groups.copy()
                     if node.hasAttribute("groups"):
-                        nodeGroups = (
-                            node.getAttribute("groups") + "," + nodeGroups
+                        nodeGroups |= self._ParseSet(
+                            node.getAttribute("groups")
                         )
-                    node.setAttribute("groups", nodeGroups)
+                    node.setAttribute("groups", ",".join(sorted(nodeGroups)))
                 if (
                     parent_node
                     and node.nodeName == "project"
@@ -1461,7 +1465,7 @@ def recursively_add_projects(project):
                 dest_path = node.getAttribute("dest-path")
                 groups = node.getAttribute("groups")
                 if groups:
-                    groups = self._ParseList(groups)
+                    groups = self._ParseSet(groups or "")
                 revision = node.getAttribute("revision")
                 remote_name = node.getAttribute("remote")
                 if not remote_name:
@@ -1482,7 +1486,7 @@ def recursively_add_projects(project):
                     if path and p.relpath != path:
                         continue
                     if groups:
-                        p.groups.extend(groups)
+                        p.groups |= groups
                     if revision:
                         if base_revision:
                             if p.revisionExpr != base_revision:
@@ -1813,7 +1817,7 @@ def _ParseSubmanifest(self, node):
         groups = ""
         if node.hasAttribute("groups"):
             groups = node.getAttribute("groups")
-        groups = self._ParseList(groups)
+        groups = self._ParseSet(groups)
         default_groups = self._ParseList(node.getAttribute("default-groups"))
         path = node.getAttribute("path")
         if path == "":
@@ -1922,11 +1926,6 @@ def _ParseProject(self, node, parent=None, **extra_proj_attrs):
 
         upstream = node.getAttribute("upstream") or self._default.upstreamExpr
 
-        groups = ""
-        if node.hasAttribute("groups"):
-            groups = node.getAttribute("groups")
-        groups = self._ParseList(groups)
-
         if parent is None:
             (
                 relpath,
@@ -1941,8 +1940,11 @@ def _ParseProject(self, node, parent=None, **extra_proj_attrs):
                 parent, name, path
             )
 
-        default_groups = ["all", "name:%s" % name, "path:%s" % relpath]
-        groups.extend(set(default_groups).difference(groups))
+        groups = ""
+        if node.hasAttribute("groups"):
+            groups = node.getAttribute("groups")
+        groups = self._ParseSet(groups)
+        groups |= {"all", f"name:{name}", f"path:{relpath}"}
 
         if self.IsMirror and node.hasAttribute("force-path"):
             if XmlBool(node, "force-path", False):

project.py

@@ -554,7 +554,7 @@ def __init__(
         revisionExpr,
         revisionId,
         rebase=True,
-        groups=None,
+        groups=set(),
         sync_c=False,
         sync_s=False,
         sync_tags=True,
@@ -839,9 +839,9 @@ def MatchesGroups(self, manifest_groups):
         """
         default_groups = self.manifest.default_groups or ["default"]
         expanded_manifest_groups = manifest_groups or default_groups
-        expanded_project_groups = ["all"] + (self.groups or [])
+        expanded_project_groups = {"all"} | self.groups
         if "notdefault" not in expanded_project_groups:
-            expanded_project_groups += ["default"]
+            expanded_project_groups |= {"default"}
 
         matched = False
         for group in expanded_manifest_groups:

tests/test_manifest_xml.py

@@ -605,12 +605,12 @@ def test_group(self):
             manifest.projects[0].name: manifest.projects[0].groups,
             manifest.projects[1].name: manifest.projects[1].groups,
         }
-        self.assertCountEqual(
-            result["test-name"], ["name:test-name", "all", "path:test-path"]
+        self.assertEqual(
+            result["test-name"], {"name:test-name", "all", "path:test-path"}
         )
-        self.assertCountEqual(
+        self.assertEqual(
             result["extras"],
-            ["g1", "g2", "g1", "name:extras", "all", "path:path"],
+            {"g1", "g2", "name:extras", "all", "path:path"},
         )
         groupstr = "default,platform-" + platform.system().lower()
         self.assertEqual(groupstr, manifest.GetGroupsStr())