5 files changed, 62 insertions, 60 deletions

diff --git a/docs/manifest-format.md b/docs/manifest-format.md
index 42e1ab18..b45b9180 100644
--- a/docs/manifest-format.md
+++ b/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.
diff --git a/man/repo-manifest.1 b/man/repo-manifest.1
index 1a97ff7d..38c728e6 100644
--- a/man/repo-manifest.1
+++ b/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
diff --git a/manifest_xml.py b/manifest_xml.py
index 33a55282..2d476748 100644
--- a/manifest_xml.py
+++ b/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 @@ class _XmlSubmanifest:
         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 @@ class _XmlSubmanifest:
         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 @@ class _XmlSubmanifest:
         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 @@ class _XmlSubmanifest:
 
     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 @@ class SubmanifestSpec:
         self.manifestName = manifestName
         self.revision = revision
         self.path = path
-        self.groups = groups or []
+        self.groups = groups
 
 
 class XmlManifest:
@@ -393,7 +391,7 @@ class XmlManifest:
         manifest_file,
         local_manifests=None,
         outer_client=None,
-        parent_groups="",
+        parent_groups=None,
         submanifest_path="",
         default_groups=None,
     ):
@@ -409,7 +407,8 @@ class XmlManifest:
                 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 @@ class XmlManifest:
             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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         """
         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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                     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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                                 # 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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         self,
         path,
         include_root,
-        parent_groups="",
+        parent_groups=None,
         restrict_includes=True,
         parent_node=None,
     ):
@@ -1271,11 +1277,11 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                         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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                     "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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                     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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
         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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 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):
diff --git a/project.py b/project.py
index 41606442..b65cb1ae 100644
--- a/project.py
+++ b/project.py
@@ -554,7 +554,7 @@ class Project:
         revisionExpr,
         revisionId,
         rebase=True,
-        groups=None,
+        groups=set(),
         sync_c=False,
         sync_s=False,
         sync_tags=True,
@@ -839,9 +839,9 @@ class Project:
         """
         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:
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index f5991515..960d0cd8 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -605,12 +605,12 @@ class ProjectElementTests(ManifestParseTestCase):
             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())