From a84f43a0065e7af2a30fd6b99bf3f13efcc7961c Mon Sep 17 00:00:00 2001
From: Jack Neus <jackneus@google.com>
Date: Tue, 21 Sep 2021 22:23:55 +0000
Subject: manifest: make repo-hooks more robust wrt element ordering

Currently, repo will fail to sync to a manifest if the definition
of the repo-hooks project comes after the repo-hooks element.

BUG=none
TEST=new test, run_tests

Change-Id: I0bf85625173492af6c6404d4b67543e96e670562
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/318520
Reviewed-by: Mike Frysinger <vapier@google.com>
Tested-by: Jack Neus <jackneus@google.com>
---
 manifest_xml.py            | 49 ++++++++++++++++++++++++----------------------
 tests/test_manifest_xml.py | 13 ++++++++++++
 2 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/manifest_xml.py b/manifest_xml.py
index 55ad6c08..7099d5fe 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -852,6 +852,8 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
       for subproject in project.subprojects:
         recursively_add_projects(subproject)
 
+    repo_hooks_project = None
+    enabled_repo_hooks = None
     for node in itertools.chain(*node_list):
       if node.nodeName == 'project':
         project = self._ParseProject(node)
@@ -886,32 +888,15 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
           if remote:
             p.remote = remote.ToRemoteSpec(name)
       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._ParseList(self._reqatt(node, 'enabled-list'))
-
         # Only one project can be the hooks project
-        if self._repo_hooks_project is not None:
+        if repo_hooks_project is not None:
           raise ManifestParseError(
               'duplicate repo-hooks in %s' %
               (self.manifestFile))
 
-        # Store a reference to the Project.
-        try:
-          repo_hooks_projects = self._projects[repo_hooks_project]
-        except KeyError:
-          raise ManifestParseError(
-              'project %s not found for repo-hooks' %
-              (repo_hooks_project))
-
-        if len(repo_hooks_projects) != 1:
-          raise ManifestParseError(
-              'internal error parsing repo-hooks in %s' %
-              (self.manifestFile))
-        self._repo_hooks_project = repo_hooks_projects[0]
-
-        # Store the enabled hooks in the Project object.
-        self._repo_hooks_project.enabled_repo_hooks = enabled_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._ParseList(self._reqatt(node, 'enabled-list'))
       if node.nodeName == 'superproject':
         name = self._reqatt(node, 'name')
         # There can only be one superproject.
@@ -944,12 +929,30 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
           # If the manifest removes the hooks project, treat it as if it deleted
           # the repo-hooks element too.
-          if self._repo_hooks_project and (self._repo_hooks_project.name == name):
-            self._repo_hooks_project = None
+          if repo_hooks_project == name:
+            repo_hooks_project = None
         elif not XmlBool(node, 'optional', False):
           raise ManifestParseError('remove-project element specifies non-existent '
                                    'project: %s' % name)
 
+    # Store repo hooks project information.
+    if repo_hooks_project:
+      # Store a reference to the Project.
+      try:
+        repo_hooks_projects = self._projects[repo_hooks_project]
+      except KeyError:
+        raise ManifestParseError(
+            'project %s not found for repo-hooks' %
+            (repo_hooks_project))
+
+      if len(repo_hooks_projects) != 1:
+        raise ManifestParseError(
+            'internal error parsing repo-hooks in %s' %
+            (self.manifestFile))
+      self._repo_hooks_project = repo_hooks_projects[0]
+      # Store the enabled hooks in the Project object.
+      self._repo_hooks_project.enabled_repo_hooks = enabled_repo_hooks
+
   def _AddMetaProjectMirror(self, m):
     name = None
     m_url = m.GetRemote(m.remote.name).url
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index 59f2a779..20459d1d 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -261,6 +261,19 @@ class XmlManifestTests(ManifestParseTestCase):
   <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_repo_hooks_unordered(self):
+    """Check repo-hooks settings work even if the project def comes second."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <repo-hooks in-project="repohooks" enabled-list="a, b"/>
+  <project name="repohooks" path="src/repohooks"/>
+</manifest>
 """)
     self.assertEqual(manifest.repo_hooks_project.name, 'repohooks')
     self.assertEqual(manifest.repo_hooks_project.enabled_repo_hooks, ['a', 'b'])
-- 
cgit v1.2.3-54-g00ecf