From 51e39d536d9210dbae285f330a8ecb697d52aac6 Mon Sep 17 00:00:00 2001
From: Mike Frysinger <vapier@google.com>
Date: Fri, 4 Dec 2020 05:32:06 -0500
Subject: [PATCH] manifest_xml: harmonize list fields

We allow project.groups to be whitespace or comma delimited, but
repo-hooks.enabled-list is only whitespace delimited.  This hasn't
been a big deal as it's only ever had one valid value, but if we
want to add more, we should harmonize these a bit.

Refactor the groups method to be more generic, and run the enabled-
list attribute through it.  Then add missing docs for it.

Change-Id: Iaa96a0faa9c4a68b313b49336751831b73bf855d
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/290743
Reviewed-by: Michael Mortensen <mmortensen@google.com>
Tested-by: Mike Frysinger <vapier@google.com>
---
 docs/manifest-format.md    | 17 +++++++++++++++++
 manifest_xml.py            | 16 ++++++++++------
 tests/test_manifest_xml.py | 13 +++++++++++++
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/docs/manifest-format.md b/docs/manifest-format.md
index 2af34ac2..0201c88b 100644
--- a/docs/manifest-format.md
+++ b/docs/manifest-format.md
@@ -111,6 +111,10 @@ A description of the elements and their attributes follows.
 
 The root element of the file.
 
+### Element notice
+
+Arbitrary text that is displayed to users whenever `repo sync` finishes.
+The content is simply passed through as it exists in the manifest.
 
 ### Element remote
 
@@ -360,6 +364,19 @@ This element is mostly useful in a local manifest file, where
 the user can remove a project, and possibly replace it with their
 own definition.
 
+### Element repo-hooks
+
+NB: See the [practical documentation](./repo-hooks.md) for using repo hooks.
+
+Only one repo-hooks element may be specified at a time.
+Attempting to redefine it will fail to parse.
+
+Attribute `in-project`: The project where the hooks are defined.  The value
+must match the `name` attribute (**not** the `path` attribute) of a previously
+defined `project` element.
+
+Attribute `enabled-list`: List of hooks to use, whitespace or comma separated.
+
 ### Element include
 
 This element provides the capability of including another manifest
diff --git a/manifest_xml.py b/manifest_xml.py
index ad0017cc..00659316 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -292,8 +292,12 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if r.revision is not None:
       e.setAttribute('revision', r.revision)
 
-  def _ParseGroups(self, groups):
-    return [x for x in re.split(r'[,\s]+', groups) if x]
+  def _ParseList(self, field):
+    """Parse fields that contain flattened lists.
+
+    These are whitespace & comma separated.  Empty elements will be discarded.
+    """
+    return [x for x in re.split(r'[,\s]+', field) if x]
 
   def ToXml(self, peg_rev=False, peg_rev_upstream=True, peg_rev_dest_branch=True, groups=None):
     """Return the current manifest XML."""
@@ -302,7 +306,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if groups is None:
       groups = mp.config.GetString('manifest.groups')
     if groups:
-      groups = self._ParseGroups(groups)
+      groups = self._ParseList(groups)
 
     doc = xml.dom.minidom.Document()
     root = doc.createElement('manifest')
@@ -754,7 +758,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         path = node.getAttribute('path')
         groups = node.getAttribute('groups')
         if groups:
-          groups = self._ParseGroups(groups)
+          groups = self._ParseList(groups)
         revision = node.getAttribute('revision')
         remote = node.getAttribute('remote')
         if remote:
@@ -776,7 +780,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
       if node.nodeName == 'repo-hooks':
         # Get the name of the project and the (space-separated) list of enabled.
         repo_hooks_project = self._reqatt(node, 'in-project')
-        enabled_repo_hooks = self._reqatt(node, 'enabled-list').split()
+        enabled_repo_hooks = self._ParseList(self._reqatt(node, 'enabled-list'))
 
         # Only one project can be the hooks project
         if self._repo_hooks_project is not None:
@@ -989,7 +993,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     groups = ''
     if node.hasAttribute('groups'):
       groups = node.getAttribute('groups')
-    groups = self._ParseGroups(groups)
+    groups = self._ParseList(groups)
 
     if parent is None:
       relpath, worktree, gitdir, objdir, use_git_worktrees = \
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index 939717be..2a8c3f66 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -212,6 +212,19 @@ class XmlManifestTests(unittest.TestCase):
         '<manifest></manifest>')
     self.assertEqual(manifest.ToDict(), {})
 
+  def test_repo_hooks(self):
+    """Check repo-hooks settings."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <project name="repohooks" path="src/repohooks"/>
+  <repo-hooks in-project="repohooks" enabled-list="a, b"/>
+</manifest>
+""")
+    self.assertEqual(manifest.repo_hooks_project.name, 'repohooks')
+    self.assertEqual(manifest.repo_hooks_project.enabled_repo_hooks, ['a', 'b'])
+
   def test_project_group(self):
     """Check project group settings."""
     manifest = self.getXmlManifest("""