manifest_xml: unify bool & int parsing

We've been overly lenient with boolean parsing by ignoring invalid
values as "false" even if the user didn't intend that.  Turn all
unknown values into warnings to avoid breaking existing manifests,
and unify the parsing logic in a helper to simplify.

We've been stricter about numbers, but still copying & pasting
inconsistent code.  Add a helper for this too.  For out of range
sync-j numbers (i.e. less than 1), throw a warning for now, but
mark it for future hard failures.

Change-Id: I924162b8036e6a5f1e31b6ebb24b6a26ed63712d
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/256457
Reviewed-by: Michael Mortensen <mmortensen@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

2 files changed, 127 insertions, 55 deletions

diff --git a/manifest_xml.py b/manifest_xml.py
index fe09f498..edcbadae 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -57,6 +57,60 @@ urllib.parse.uses_netloc.extend([
     'rpc'])
 
 
+def XmlBool(node, attr, default=None):
+  """Determine boolean value of |node|'s |attr|.
+
+  Invalid values will issue a non-fatal warning.
+
+  Args:
+    node: XML node whose attributes we access.
+    attr: The attribute to access.
+    default: If the attribute is not set (value is empty), then use this.
+
+  Returns:
+    True if the attribute is a valid string representing true.
+    False if the attribute is a valid string representing false.
+    |default| otherwise.
+  """
+  value = node.getAttribute(attr)
+  s = value.lower()
+  if s == '':
+    return default
+  elif s in {'yes', 'true', '1'}:
+    return True
+  elif s in {'no', 'false', '0'}:
+    return False
+  else:
+    print('warning: manifest: %s="%s": ignoring invalid XML boolean' %
+          (attr, value), file=sys.stderr)
+    return default
+
+
+def XmlInt(node, attr, default=None):
+  """Determine integer value of |node|'s |attr|.
+
+  Args:
+    node: XML node whose attributes we access.
+    attr: The attribute to access.
+    default: If the attribute is not set (value is empty), then use this.
+
+  Returns:
+    The number if the attribute is a valid number.
+
+  Raises:
+    ManifestParseError: The number is invalid.
+  """
+  value = node.getAttribute(attr)
+  if not value:
+    return default
+
+  try:
+    return int(value)
+  except ValueError:
+    raise ManifestParseError('manifest: invalid %s="%s" integer' %
+                             (attr, value))
+
+
 class _Default(object):
   """Project defaults within the manifest."""
 
@@ -757,29 +811,14 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     d.destBranchExpr = node.getAttribute('dest-branch') or None
     d.upstreamExpr = node.getAttribute('upstream') or None
 
-    sync_j = node.getAttribute('sync-j')
-    if sync_j == '' or sync_j is None:
-      d.sync_j = 1
-    else:
-      d.sync_j = int(sync_j)
-
-    sync_c = node.getAttribute('sync-c')
-    if not sync_c:
-      d.sync_c = False
-    else:
-      d.sync_c = sync_c.lower() in ("yes", "true", "1")
-
-    sync_s = node.getAttribute('sync-s')
-    if not sync_s:
-      d.sync_s = False
-    else:
-      d.sync_s = sync_s.lower() in ("yes", "true", "1")
+    d.sync_j = XmlInt(node, 'sync-j', 1)
+    if d.sync_j <= 0:
+      raise ManifestParseError('%s: sync-j must be greater than 0, not "%s"' %
+                               (self.manifestFile, d.sync_j))
 
-    sync_tags = node.getAttribute('sync-tags')
-    if not sync_tags:
-      d.sync_tags = True
-    else:
-      d.sync_tags = sync_tags.lower() in ("yes", "true", "1")
+    d.sync_c = XmlBool(node, 'sync-c', False)
+    d.sync_s = XmlBool(node, 'sync-s', False)
+    d.sync_tags = XmlBool(node, 'sync-tags', True)
     return d
 
   def _ParseNotice(self, node):
@@ -856,39 +895,15 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
       raise ManifestParseError("project %s path cannot be absolute in %s" %
                                (name, self.manifestFile))
 
-    rebase = node.getAttribute('rebase')
-    if not rebase:
-      rebase = True
-    else:
-      rebase = rebase.lower() in ("yes", "true", "1")
-
-    sync_c = node.getAttribute('sync-c')
-    if not sync_c:
-      sync_c = False
-    else:
-      sync_c = sync_c.lower() in ("yes", "true", "1")
-
-    sync_s = node.getAttribute('sync-s')
-    if not sync_s:
-      sync_s = self._default.sync_s
-    else:
-      sync_s = sync_s.lower() in ("yes", "true", "1")
-
-    sync_tags = node.getAttribute('sync-tags')
-    if not sync_tags:
-      sync_tags = self._default.sync_tags
-    else:
-      sync_tags = sync_tags.lower() in ("yes", "true", "1")
+    rebase = XmlBool(node, 'rebase', True)
+    sync_c = XmlBool(node, 'sync-c', False)
+    sync_s = XmlBool(node, 'sync-s', self._default.sync_s)
+    sync_tags = XmlBool(node, 'sync-tags', self._default.sync_tags)
 
-    clone_depth = node.getAttribute('clone-depth')
-    if clone_depth:
-      try:
-        clone_depth = int(clone_depth)
-        if clone_depth <= 0:
-          raise ValueError()
-      except ValueError:
-        raise ManifestParseError('invalid clone-depth %s in %s' %
-                                 (clone_depth, self.manifestFile))
+    clone_depth = XmlInt(node, 'clone-depth')
+    if clone_depth is not None and clone_depth <= 0:
+      raise ManifestParseError('%s: clone-depth must be greater than 0, not "%s"' %
+                               (self.manifestFile, clone_depth))
 
     dest_branch = node.getAttribute('dest-branch') or self._default.destBranchExpr
 
@@ -911,7 +926,7 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     groups.extend(set(default_groups).difference(groups))
 
     if self.IsMirror and node.hasAttribute('force-path'):
-      if node.getAttribute('force-path').lower() in ("yes", "true", "1"):
+      if XmlBool(node, 'force-path', False):
         gitdir = os.path.join(self.topdir, '%s.git' % path)
 
     project = Project(manifest=self,
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index 1cb72971..aa6cb7df 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -20,6 +20,7 @@ from __future__ import print_function
 
 import os
 import unittest
+import xml.dom.minidom
 
 import error
 import manifest_xml
@@ -89,3 +90,59 @@ class ManifestValidateFilePaths(unittest.TestCase):
           error.ManifestInvalidPathError, self.check_both, path, 'a')
       self.assertRaises(
           error.ManifestInvalidPathError, self.check_both, 'a', path)
+
+
+class ValueTests(unittest.TestCase):
+  """Check utility parsing code."""
+
+  def _get_node(self, text):
+    return xml.dom.minidom.parseString(text).firstChild
+
+  def test_bool_default(self):
+    """Check XmlBool default handling."""
+    node = self._get_node('<node/>')
+    self.assertIsNone(manifest_xml.XmlBool(node, 'a'))
+    self.assertIsNone(manifest_xml.XmlBool(node, 'a', None))
+    self.assertEqual(123, manifest_xml.XmlBool(node, 'a', 123))
+
+    node = self._get_node('<node a=""/>')
+    self.assertIsNone(manifest_xml.XmlBool(node, 'a'))
+
+  def test_bool_invalid(self):
+    """Check XmlBool invalid handling."""
+    node = self._get_node('<node a="moo"/>')
+    self.assertEqual(123, manifest_xml.XmlBool(node, 'a', 123))
+
+  def test_bool_true(self):
+    """Check XmlBool true values."""
+    for value in ('yes', 'true', '1'):
+      node = self._get_node('<node a="%s"/>' % (value,))
+      self.assertTrue(manifest_xml.XmlBool(node, 'a'))
+
+  def test_bool_false(self):
+    """Check XmlBool false values."""
+    for value in ('no', 'false', '0'):
+      node = self._get_node('<node a="%s"/>' % (value,))
+      self.assertFalse(manifest_xml.XmlBool(node, 'a'))
+
+  def test_int_default(self):
+    """Check XmlInt default handling."""
+    node = self._get_node('<node/>')
+    self.assertIsNone(manifest_xml.XmlInt(node, 'a'))
+    self.assertIsNone(manifest_xml.XmlInt(node, 'a', None))
+    self.assertEqual(123, manifest_xml.XmlInt(node, 'a', 123))
+
+    node = self._get_node('<node a=""/>')
+    self.assertIsNone(manifest_xml.XmlInt(node, 'a'))
+
+  def test_int_good(self):
+    """Check XmlInt numeric handling."""
+    for value in (-1, 0, 1, 50000):
+      node = self._get_node('<node a="%s"/>' % (value,))
+      self.assertEqual(value, manifest_xml.XmlInt(node, 'a'))
+
+  def test_int_invalid(self):
+    """Check XmlInt invalid handling."""
+    with self.assertRaises(error.ManifestParseError):
+      node = self._get_node('<node a="xx"/>')
+      manifest_xml.XmlInt(node, 'a')