manifest: add optional base check on remove and extend

This adds an optional, built-in checker for
guarding against patches hanging on wrong
base revisions, which is useful if a lower layer of
the manifest changes after a patch was done.

When adding a patch with a new revision using
extend-project or remove-project/project:

          C---D---E patches in project bla
         /
    A---B project bla in manifest state 1

<extend-project name="bla" revision="E" base-rev="B">

If project bla gets updated, in a new snap ID
or by a supplier or similar, to a new state:

          C---D---E patches in project bla
         /
    A---B---F---G project bla in manifest state 2

Parsing will fail because revision of bla is now G,
giving the choice to create a new patch branch
from G and updating base-rev, or keeping previous
branch for some reason and only updating base-rev.

Intended for use in a layered manifest with
hashed revisions. Named refs like branches and tags
also work fine when comparing, but will be misleading
if a branch is used as base-rev.

Change-Id: Ic6211550a7d3cc9656057f6a2087c505b40cad2b
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/436777
Reviewed-by: Josip Sokcevic <[email protected]>
Tested-by: Fredrik de Groot <[email protected]>
Commit-Queue: Josip Sokcevic <[email protected]>

Author: SpecificityD3luxe

docs/manifest-format.md

@@ -107,11 +107,13 @@ following DTD:
   <!ATTLIST extend-project remote CDATA #IMPLIED>
   <!ATTLIST extend-project dest-branch CDATA #IMPLIED>
   <!ATTLIST extend-project upstream CDATA #IMPLIED>
+  <!ATTLIST extend-project base-rev CDATA #IMPLIED>
 
   <!ELEMENT remove-project EMPTY>
   <!ATTLIST remove-project name     CDATA #IMPLIED>
   <!ATTLIST remove-project path     CDATA #IMPLIED>
   <!ATTLIST remove-project optional CDATA #IMPLIED>
+  <!ATTLIST remove-project base-rev CDATA #IMPLIED>
 
   <!ELEMENT repo-hooks EMPTY>
   <!ATTLIST repo-hooks in-project CDATA #REQUIRED>
@@ -433,6 +435,14 @@ project.  Same syntax as the corresponding element of `project`.
 Attribute `upstream`: If specified, overrides the upstream of the original
 project.  Same syntax as the corresponding element of `project`.
 
+Attribute `base-rev`: If specified, adds a check against the revision
+to be extended. Manifest parse will fail and give a list of mismatch extends
+if the revisions being extended have changed since base-rev was set.
+Intended for use with layered manifests using hash revisions to prevent
+patch branches hiding newer upstream revisions. Also compares named refs
+like branches or tags but is misleading if branches are used as base-rev.
+Same syntax as the corresponding element of `project`.
+
 ### Element annotation
 
 Zero or more annotation elements may be specified as children of a
@@ -496,6 +506,14 @@ name. Logic otherwise behaves like both are specified.
 Attribute `optional`: Set to true to ignore remove-project elements with no
 matching `project` element.
 
+Attribute `base-rev`: If specified, adds a check against the revision
+to be removed. Manifest parse will fail and give a list of mismatch removes
+if the revisions being removed have changed since base-rev was set.
+Intended for use with layered manifests using hash revisions to prevent
+patch branches hiding newer upstream revisions. Also compares named refs
+like branches or tags but is misleading if branches are used as base-rev.
+Same syntax as the corresponding element of `project`.
+
 ### Element repo-hooks
 
 NB: See the [practical documentation](./repo-hooks.md) for using repo hooks.

manifest_xml.py

@@ -1445,6 +1445,7 @@ def recursively_add_projects(project):
 
         repo_hooks_project = None
         enabled_repo_hooks = None
+        failed_revision_changes = []
         for node in itertools.chain(*node_list):
             if node.nodeName == "project":
                 project = self._ParseProject(node)
@@ -1471,6 +1472,7 @@ def recursively_add_projects(project):
                     remote = self._get_remote(node)
                 dest_branch = node.getAttribute("dest-branch")
                 upstream = node.getAttribute("upstream")
+                base_revision = node.getAttribute("base-rev")
 
                 named_projects = self._projects[name]
                 if dest_path and not path and len(named_projects) > 1:
@@ -1484,6 +1486,13 @@ def recursively_add_projects(project):
                     if groups:
                         p.groups.extend(groups)
                     if revision:
+                        if base_revision:
+                            if p.revisionExpr != base_revision:
+                                failed_revision_changes.append(
+                                    "extend-project name %s mismatch base "
+                                    "%s vs revision %s"
+                                    % (name, base_revision, p.revisionExpr)
+                                )
                         p.SetRevision(revision)
 
                     if remote_name:
@@ -1558,6 +1567,7 @@ def recursively_add_projects(project):
             if node.nodeName == "remove-project":
                 name = node.getAttribute("name")
                 path = node.getAttribute("path")
+                base_revision = node.getAttribute("base-rev")
 
                 # Name or path needed.
                 if not name and not path:
@@ -1571,13 +1581,31 @@ def recursively_add_projects(project):
                 for projname, projects in list(self._projects.items()):
                     for p in projects:
                         if name == projname and not path:
+                            if base_revision:
+                                if p.revisionExpr != base_revision:
+                                    failed_revision_changes.append(
+                                        "remove-project name %s mismatch base "
+                                        "%s vs revision %s"
+                                        % (name, base_revision, p.revisionExpr)
+                                    )
                             del self._paths[p.relpath]
                             if not removed_project:
                                 del self._projects[name]
                                 removed_project = name
                         elif path == p.relpath and (
                             name == projname or not name
                         ):
+                            if base_revision:
+                                if p.revisionExpr != base_revision:
+                                    failed_revision_changes.append(
+                                        "remove-project path %s mismatch base "
+                                        "%s vs revision %s"
+                                        % (
+                                            p.relpath,
+                                            base_revision,
+                                            p.revisionExpr,
+                                        )
+                                    )
                             self._projects[projname].remove(p)
                             del self._paths[p.relpath]
                             removed_project = p.name
@@ -1597,6 +1625,13 @@ def recursively_add_projects(project):
                         "project: %s" % node.toxml()
                     )
 
+        if failed_revision_changes:
+            raise ManifestParseError(
+                "revision base check failed, rebase patches and update "
+                "base revs for: ",
+                failed_revision_changes,
+            )
+
         # Store repo hooks project information.
         if repo_hooks_project:
             # Store a reference to the Project.

tests/test_manifest_xml.py

@@ -1049,6 +1049,91 @@ def test_remove_using_path_attrib(self):
         self.assertTrue(found_proj1_path1)
         self.assertTrue(found_proj2)
 
+    def test_base_revision_checks_on_patching(self):
+        manifest_fail_wrong_tag = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="tag.002" />
+  <project name="project1" path="tests/path1" />
+  <extend-project name="project1" revision="new_hash" base-rev="tag.001" />
+</manifest>
+"""
+        )
+        with self.assertRaises(error.ManifestParseError):
+            manifest_fail_wrong_tag.ToXml()
+
+        manifest_fail_remove = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="project1" path="tests/path1" revision="hash1" />
+  <remove-project name="project1" base-rev="wrong_hash" />
+</manifest>
+"""
+        )
+        with self.assertRaises(error.ManifestParseError):
+            manifest_fail_remove.ToXml()
+
+        manifest_fail_extend = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="project1" path="tests/path1" revision="hash1" />
+  <extend-project name="project1" revision="new_hash" base-rev="wrong_hash" />
+</manifest>
+"""
+        )
+        with self.assertRaises(error.ManifestParseError):
+            manifest_fail_extend.ToXml()
+
+        manifest_fail_unknown = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="project1" path="tests/path1" />
+  <extend-project name="project1" revision="new_hash" base-rev="any_hash" />
+</manifest>
+"""
+        )
+        with self.assertRaises(error.ManifestParseError):
+            manifest_fail_unknown.ToXml()
+
+        manifest_ok = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="project1" path="tests/path1" revision="hash1" />
+  <project name="project2" path="tests/path2" revision="hash2" />
+  <project name="project3" path="tests/path3" revision="hash3" />
+  <project name="project4" path="tests/path4" revision="hash4" />
+
+  <remove-project name="project1" />
+  <remove-project name="project2" base-rev="hash2" />
+  <project name="project2" path="tests/path2" revision="new_hash2" />
+  <extend-project name="project3" base-rev="hash3" revision="new_hash3" />
+  <extend-project name="project3" base-rev="new_hash3" revision="newer_hash3" />
+  <remove-project path="tests/path4" base-rev="hash4" />
+</manifest>
+"""
+        )
+        found_proj2 = False
+        found_proj3 = False
+        for proj in manifest_ok.projects:
+            if proj.name == "project2":
+                found_proj2 = True
+            if proj.name == "project3":
+                found_proj3 = True
+            self.assertNotEqual(proj.name, "project1")
+            self.assertNotEqual(proj.name, "project4")
+        self.assertTrue(found_proj2)
+        self.assertTrue(found_proj3)
+        self.assertTrue(len(manifest_ok.projects) == 2)
+
 
 class ExtendProjectElementTests(ManifestParseTestCase):
     """Tests for <extend-project>."""