repo: properly handle remote annotations in manifest_xml

BUG=b:192664812
TEST=tests/

Change-Id: I1aa50260f4a00d3cebbd531141e1626825e70127
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/312643
Tested-by: Jack Neus <jackneus@google.com>
Reviewed-by: Mike Frysinger <vapier@google.com>

docs/manifest-format.md

@@ -36,7 +36,7 @@ following DTD:
 
   <!ELEMENT notice (#PCDATA)>
 
-  <!ELEMENT remote EMPTY>
+  <!ELEMENT remote (annotation*)>
   <!ATTLIST remote name         ID    #REQUIRED>
   <!ATTLIST remote alias        CDATA #IMPLIED>
   <!ATTLIST remote fetch        CDATA #REQUIRED>
@@ -348,12 +348,12 @@ project.  Same syntax as the corresponding element of `project`.
 ### Element annotation
 
 Zero or more annotation elements may be specified as children of a
-project element. Each element describes a name-value pair that will be
+project or remote element. Each element describes a name-value pair.
-exported into each project's environment during a 'forall' command,
+For projects, this name-value pair will be exported into each project's
-prefixed with REPO__.  In addition, there is an optional attribute
+environment during a 'forall' command, prefixed with `REPO__`.  In addition,
-"keep" which accepts the case insensitive values "true" (default) or
+there is an optional attribute "keep" which accepts the case insensitive values
-"false".  This attribute determines whether or not the annotation will
+"true" (default) or "false".  This attribute determines whether or not the
-be kept when exported with the manifest subcommand.
+annotation will be kept when exported with the manifest subcommand.
 
 ### Element copyfile
 

manifest_xml.py

@@ -25,7 +25,7 @@ import gitc_utils
 from git_config import GitConfig, IsId
 from git_refs import R_HEADS, HEAD
 import platform_utils
-from project import RemoteSpec, Project, MetaProject
+from project import Annotation, RemoteSpec, Project, MetaProject
 from error import (ManifestParseError, ManifestInvalidPathError,
                    ManifestInvalidRevisionError)
 from wrapper import Wrapper
@@ -149,16 +149,18 @@ class _XmlRemote(object):
     self.reviewUrl = review
     self.revision = revision
     self.resolvedFetchUrl = self._resolveFetchUrl()
+    self.annotations = []
 
   def __eq__(self, other):
     if not isinstance(other, _XmlRemote):
       return False
-    return self.__dict__ == other.__dict__
+    return (sorted(self.annotations) == sorted(other.annotations) and
+      self.name == other.name and self.fetchUrl == other.fetchUrl and
+      self.pushUrl == other.pushUrl and self.remoteAlias == other.remoteAlias
+      and self.reviewUrl == other.reviewUrl and self.revision == other.revision)
 
   def __ne__(self, other):
-    if not isinstance(other, _XmlRemote):
+    return not self.__eq__(other)
-      return True
-    return self.__dict__ != other.__dict__
 
   def _resolveFetchUrl(self):
     if self.fetchUrl is None:
@@ -191,6 +193,9 @@ class _XmlRemote(object):
                       orig_name=self.name,
                       fetchUrl=self.fetchUrl)
 
+  def AddAnnotation(self, name, value, keep):
+    self.annotations.append(Annotation(name, value, keep))
+
 
 class XmlManifest(object):
   """manages the repo configuration file"""
@@ -300,6 +305,13 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if r.revision is not None:
       e.setAttribute('revision', r.revision)
 
+    for a in r.annotations:
+      if a.keep == 'true':
+        ae = doc.createElement('annotation')
+        ae.setAttribute('name', a.name)
+        ae.setAttribute('value', a.value)
+        e.appendChild(ae)
+
   def _ParseList(self, field):
     """Parse fields that contain flattened lists.
 
@@ -995,7 +1007,14 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if revision == '':
       revision = None
     manifestUrl = self.manifestProject.config.GetString('remote.origin.url')
-    return _XmlRemote(name, alias, fetch, pushUrl, manifestUrl, review, revision)
+
+    remote = _XmlRemote(name, alias, fetch, pushUrl, manifestUrl, review, revision)
+
+    for n in node.childNodes:
+      if n.nodeName == 'annotation':
+        self._ParseAnnotation(remote, n)
+
+    return remote
 
   def _ParseDefault(self, node):
     """
@@ -1362,7 +1381,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
       self._ValidateFilePaths('linkfile', src, dest)
       project.AddLinkFile(src, dest, self.topdir)
 
-  def _ParseAnnotation(self, project, node):
+  def _ParseAnnotation(self, element, node):
     name = self._reqatt(node, 'name')
     value = self._reqatt(node, 'value')
     try:
@@ -1372,7 +1391,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if keep != "true" and keep != "false":
       raise ManifestParseError('optional "keep" attribute must be '
                                '"true" or "false"')
-    project.AddAnnotation(name, value, keep)
+    element.AddAnnotation(name, value, keep)
 
   def _get_remote(self, node):
     name = node.getAttribute('remote')

project.py

@@ -251,13 +251,29 @@ class DiffColoring(Coloring):
     self.fail = self.printer('fail', fg='red')
 
 
-class _Annotation(object):
+class Annotation(object):
 
   def __init__(self, name, value, keep):
     self.name = name
     self.value = value
     self.keep = keep
 
+  def __eq__(self, other):
+    if not isinstance(other, Annotation):
+      return False
+    return self.__dict__ == other.__dict__
+
+  def __lt__(self, other):
+    # This exists just so that lists of Annotation objects can be sorted, for
+    # use in comparisons.
+    if not isinstance(other, Annotation):
+      raise ValueError('comparison is not between two Annotation objects')
+    if self.name == other.name:
+      if self.value == other.value:
+        return self.keep < other.keep
+      return self.value < other.value
+    return self.name < other.name
+
 
 def _SafeExpandPath(base, subpath, skipfinal=False):
   """Make sure |subpath| is completely safe under |base|.
@@ -1448,7 +1464,7 @@ class Project(object):
     self.linkfiles.append(_LinkFile(self.worktree, src, topdir, dest))
 
   def AddAnnotation(self, name, value, keep):
-    self.annotations.append(_Annotation(name, value, keep))
+    self.annotations.append(Annotation(name, value, keep))
 
   def DownloadPatchSet(self, change_id, patch_id):
     """Download a single patch set of a single change to FETCH_HEAD.

tests/test_manifest_xml.py

@@ -286,6 +286,25 @@ class XmlManifestTests(ManifestParseTestCase):
         '<superproject name="superproject"/>'
         '</manifest>')
 
+  def test_remote_annotations(self):
+    """Check remote settings."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost">
+    <annotation name="foo" value="bar"/>
+  </remote>
+</manifest>
+""")
+    self.assertEqual(manifest.remotes['test-remote'].annotations[0].name, 'foo')
+    self.assertEqual(manifest.remotes['test-remote'].annotations[0].value, 'bar')
+    self.assertEqual(
+        sort_attributes(manifest.ToXml().toxml()),
+        '<?xml version="1.0" ?><manifest>'
+        '<remote fetch="http://localhost" name="test-remote">'
+        '<annotation name="foo" value="bar"/>'
+        '</remote>'
+        '</manifest>')
+
 
 class IncludeElementTests(ManifestParseTestCase):
   """Tests for <include>."""
@@ -632,9 +651,17 @@ class RemoteElementTests(ManifestParseTestCase):
   def test_remote(self):
     """Check remote settings."""
     a = manifest_xml._XmlRemote(name='foo')
-    b = manifest_xml._XmlRemote(name='bar')
+    a.AddAnnotation('key1', 'value1', 'true')
+    b = manifest_xml._XmlRemote(name='foo')
+    b.AddAnnotation('key2', 'value1', 'true')
+    c = manifest_xml._XmlRemote(name='foo')
+    c.AddAnnotation('key1', 'value2', 'true')
+    d = manifest_xml._XmlRemote(name='foo')
+    d.AddAnnotation('key1', 'value1', 'false')
     self.assertEqual(a, a)
     self.assertNotEqual(a, b)
+    self.assertNotEqual(a, c)
+    self.assertNotEqual(a, d)
     self.assertNotEqual(a, manifest_xml._Default())
     self.assertNotEqual(a, 123)
     self.assertNotEqual(a, None)