manifest_xml: refactor manifest parsing from client management
We conflate the manifest & parsing logic with the management of the
repo client checkout in a single class. This makes testing just one
part (the manifest parsing) hard as it requires a full checkout too.
Start splitting the two apart into separate classes to make it easy
to reason about & test.
Change-Id: Iaf897c93db9c724baba6044bfe7a589c024523b2
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/288682
Reviewed-by: Michael Mortensen <mmortensen@google.com>
Tested-by: Mike Frysinger <vapier@google.com>
diff --git a/main.py b/main.py
index cd62793..e152de4 100755
--- a/main.py
+++ b/main.py
@@ -63,7 +63,7 @@
from error import NoSuchProjectError
from error import RepoChangedException
import gitc_utils
-from manifest_xml import GitcManifest, XmlManifest
+from manifest_xml import GitcClient, RepoClient
from pager import RunPager, TerminatePager
from wrapper import WrapperPath, Wrapper
@@ -212,14 +212,15 @@
return 1
cmd.repodir = self.repodir
- cmd.manifest = XmlManifest(cmd.repodir)
+ cmd.client = RepoClient(cmd.repodir)
+ cmd.manifest = cmd.client.manifest
cmd.gitc_manifest = None
gitc_client_name = gitc_utils.parse_clientdir(os.getcwd())
if gitc_client_name:
- cmd.gitc_manifest = GitcManifest(cmd.repodir, gitc_client_name)
- cmd.manifest.isGitcClient = True
+ cmd.gitc_manifest = GitcClient(cmd.repodir, gitc_client_name)
+ cmd.client.isGitcClient = True
- Editor.globalConfig = cmd.manifest.globalConfig
+ Editor.globalConfig = cmd.client.globalConfig
if not isinstance(cmd, MirrorSafeCommand) and cmd.manifest.IsMirror:
print("fatal: '%s' requires a working directory" % name,
@@ -247,7 +248,7 @@
return 1
if gopts.pager is not False and not isinstance(cmd, InteractiveCommand):
- config = cmd.manifest.globalConfig
+ config = cmd.client.globalConfig
if gopts.pager:
use_pager = True
else:
diff --git a/manifest_xml.py b/manifest_xml.py
index e1ef330..95c67d7 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -187,12 +187,24 @@
class XmlManifest(object):
"""manages the repo configuration file"""
- def __init__(self, repodir):
+ def __init__(self, repodir, manifest_file, local_manifests=None):
+ """Initialize.
+
+ Args:
+ repodir: Path to the .repo/ dir for holding all internal checkout state.
+ It must be in the top directory of the repo client checkout.
+ manifest_file: Full path to the manifest file to parse. This will usually
+ be |repodir|/|MANIFEST_FILE_NAME|.
+ local_manifests: Full path to the directory of local override manifests.
+ This will usually be |repodir|/|LOCAL_MANIFESTS_DIR_NAME|.
+ """
+ # TODO(vapier): Move this out of this class.
+ self.globalConfig = GitConfig.ForUser()
+
self.repodir = os.path.abspath(repodir)
self.topdir = os.path.dirname(self.repodir)
- self.manifestFile = os.path.join(self.repodir, MANIFEST_FILE_NAME)
- self.globalConfig = GitConfig.ForUser()
- self.isGitcClient = False
+ self.manifestFile = manifest_file
+ self.local_manifests = local_manifests
self._load_local_manifests = True
self.repoProject = MetaProject(self, 'repo',
@@ -602,20 +614,11 @@
nodes.append(self._ParseManifestXml(self.manifestFile,
self.manifestProject.worktree))
- if self._load_local_manifests:
- if os.path.exists(os.path.join(self.repodir, LOCAL_MANIFEST_NAME)):
- print('error: %s is not supported; put local manifests in `%s`'
- 'instead' % (LOCAL_MANIFEST_NAME,
- os.path.join(self.repodir, LOCAL_MANIFESTS_DIR_NAME)),
- file=sys.stderr)
- sys.exit(1)
-
- local_dir = os.path.abspath(os.path.join(self.repodir,
- LOCAL_MANIFESTS_DIR_NAME))
+ if self._load_local_manifests and self.local_manifests:
try:
- for local_file in sorted(platform_utils.listdir(local_dir)):
+ for local_file in sorted(platform_utils.listdir(self.local_manifests)):
if local_file.endswith('.xml'):
- local = os.path.join(local_dir, local_file)
+ local = os.path.join(self.local_manifests, local_file)
nodes.append(self._ParseManifestXml(local, self.repodir))
except OSError:
pass
@@ -1253,15 +1256,7 @@
class GitcManifest(XmlManifest):
-
- def __init__(self, repodir, gitc_client_name):
- """Initialize the GitcManifest object."""
- super(GitcManifest, self).__init__(repodir)
- self.isGitcClient = True
- self.gitc_client_name = gitc_client_name
- self.gitc_client_dir = os.path.join(gitc_utils.get_gitc_manifest_dir(),
- gitc_client_name)
- self.manifestFile = os.path.join(self.gitc_client_dir, '.manifest')
+ """Parser for GitC (git-in-the-cloud) manifests."""
def _ParseProject(self, node, parent=None):
"""Override _ParseProject and add support for GITC specific attributes."""
@@ -1272,3 +1267,38 @@
"""Output GITC Specific Project attributes"""
if p.old_revision:
e.setAttribute('old-revision', str(p.old_revision))
+
+
+class RepoClient(XmlManifest):
+ """Manages a repo client checkout."""
+
+ def __init__(self, repodir, manifest_file=None):
+ self.isGitcClient = False
+
+ if os.path.exists(os.path.join(repodir, LOCAL_MANIFEST_NAME)):
+ print('error: %s is not supported; put local manifests in `%s` instead' %
+ (LOCAL_MANIFEST_NAME, os.path.join(repodir, LOCAL_MANIFESTS_DIR_NAME)),
+ file=sys.stderr)
+ sys.exit(1)
+
+ if manifest_file is None:
+ manifest_file = os.path.join(repodir, MANIFEST_FILE_NAME)
+ local_manifests = os.path.abspath(os.path.join(repodir, LOCAL_MANIFESTS_DIR_NAME))
+ super(RepoClient, self).__init__(repodir, manifest_file, local_manifests)
+
+ # TODO: Completely separate manifest logic out of the client.
+ self.manifest = self
+
+
+class GitcClient(RepoClient, GitcManifest):
+ """Manages a GitC client checkout."""
+
+ def __init__(self, repodir, gitc_client_name):
+ """Initialize the GitcManifest object."""
+ self.gitc_client_name = gitc_client_name
+ self.gitc_client_dir = os.path.join(gitc_utils.get_gitc_manifest_dir(),
+ gitc_client_name)
+
+ super(GitcManifest, self).__init__(
+ repodir, os.path.join(self.gitc_client_dir, '.manifest'))
+ self.isGitcClient = True
diff --git a/project.py b/project.py
index 50bb53c..29f6b1e 100644
--- a/project.py
+++ b/project.py
@@ -510,7 +510,7 @@
with exponential backoff and jitter.
old_revision: saved git commit id for open GITC projects.
"""
- self.manifest = manifest
+ self.client = self.manifest = manifest
self.name = name
self.remote = remote
self.gitdir = gitdir.replace('\\', '/')
@@ -551,7 +551,7 @@
self.linkfiles = []
self.annotations = []
self.config = GitConfig.ForRepository(gitdir=self.gitdir,
- defaults=self.manifest.globalConfig)
+ defaults=self.client.globalConfig)
if self.worktree:
self.work_git = self._GitGetByExec(self, bare=False, gitdir=gitdir)
@@ -1168,7 +1168,7 @@
self._InitHooks()
def _CopyAndLinkFiles(self):
- if self.manifest.isGitcClient:
+ if self.client.isGitcClient:
return
for copyfile in self.copyfiles:
copyfile._Copy()
diff --git a/subcmds/diffmanifests.py b/subcmds/diffmanifests.py
index 72c441a..409bbda 100644
--- a/subcmds/diffmanifests.py
+++ b/subcmds/diffmanifests.py
@@ -16,7 +16,7 @@
from color import Coloring
from command import PagedCommand
-from manifest_xml import XmlManifest
+from manifest_xml import RepoClient
class _Coloring(Coloring):
@@ -183,7 +183,7 @@
self.OptionParser.error('missing manifests to diff')
def Execute(self, opt, args):
- self.out = _Coloring(self.manifest.globalConfig)
+ self.out = _Coloring(self.client.globalConfig)
self.printText = self.out.nofmt_printer('text')
if opt.color:
self.printProject = self.out.nofmt_printer('project', attr='bold')
@@ -193,12 +193,12 @@
else:
self.printProject = self.printAdded = self.printRemoved = self.printRevision = self.printText
- manifest1 = XmlManifest(self.manifest.repodir)
+ manifest1 = RepoClient(self.manifest.repodir)
manifest1.Override(args[0], load_local_manifests=False)
if len(args) == 1:
manifest2 = self.manifest
else:
- manifest2 = XmlManifest(self.manifest.repodir)
+ manifest2 = RepoClient(self.manifest.repodir)
manifest2.Override(args[1], load_local_manifests=False)
diff = manifest1.projectsDiff(manifest2)
diff --git a/subcmds/help.py b/subcmds/help.py
index 1e16019..c219a76 100644
--- a/subcmds/help.py
+++ b/subcmds/help.py
@@ -65,7 +65,7 @@
def gitc_supported(cmd):
if not isinstance(cmd, GitcAvailableCommand) and not isinstance(cmd, GitcClientCommand):
return True
- if self.manifest.isGitcClient:
+ if self.client.isGitcClient:
return True
if isinstance(cmd, GitcClientCommand):
return False
@@ -127,7 +127,7 @@
self.wrap.end_paragraph(1)
self.wrap.end_paragraph(0)
- out = _Out(self.manifest.globalConfig)
+ out = _Out(self.client.globalConfig)
out._PrintSection('Summary', 'helpSummary')
cmd.OptionParser.print_help()
out._PrintSection('Description', 'helpDescription')
diff --git a/subcmds/info.py b/subcmds/info.py
index f4d6f98..6014997 100644
--- a/subcmds/info.py
+++ b/subcmds/info.py
@@ -44,7 +44,7 @@
help="Disable all remote operations")
def Execute(self, opt, args):
- self.out = _Coloring(self.manifest.globalConfig)
+ self.out = _Coloring(self.client.globalConfig)
self.heading = self.out.printer('heading', attr='bold')
self.headtext = self.out.nofmt_printer('headtext', fg='yellow')
self.redtext = self.out.printer('redtext', fg='red')
diff --git a/subcmds/init.py b/subcmds/init.py
index 5ba0d07..f46babf 100644
--- a/subcmds/init.py
+++ b/subcmds/init.py
@@ -365,7 +365,7 @@
return a
def _ShouldConfigureUser(self, opt):
- gc = self.manifest.globalConfig
+ gc = self.client.globalConfig
mp = self.manifest.manifestProject
# If we don't have local settings, get from global.
@@ -414,7 +414,7 @@
return False
def _ConfigureColor(self):
- gc = self.manifest.globalConfig
+ gc = self.client.globalConfig
if self._HasColorSet(gc):
return
diff --git a/subcmds/status.py b/subcmds/status.py
index c380dba..dfa974e 100644
--- a/subcmds/status.py
+++ b/subcmds/status.py
@@ -165,7 +165,7 @@
proj_dirs, proj_dirs_parents, outstring)
if outstring:
- output = StatusColoring(self.manifest.globalConfig)
+ output = StatusColoring(self.client.globalConfig)
output.project('Objects not within a project (orphans)')
output.nl()
for entry in outstring:
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index aa6cb7d..40385cc 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -19,6 +19,8 @@
from __future__ import print_function
import os
+import shutil
+import tempfile
import unittest
import xml.dom.minidom
@@ -146,3 +148,90 @@
with self.assertRaises(error.ManifestParseError):
node = self._get_node('<node a="xx"/>')
manifest_xml.XmlInt(node, 'a')
+
+
+class XmlManifestTests(unittest.TestCase):
+ """Check manifest processing."""
+
+ def setUp(self):
+ self.tempdir = tempfile.mkdtemp(prefix='repo_tests')
+ self.repodir = os.path.join(self.tempdir, '.repo')
+ self.manifest_dir = os.path.join(self.repodir, 'manifests')
+ self.manifest_file = os.path.join(
+ self.repodir, manifest_xml.MANIFEST_FILE_NAME)
+ self.local_manifest_dir = os.path.join(
+ self.repodir, manifest_xml.LOCAL_MANIFESTS_DIR_NAME)
+ os.mkdir(self.repodir)
+ os.mkdir(self.manifest_dir)
+
+ # The manifest parsing really wants a git repo currently.
+ gitdir = os.path.join(self.repodir, 'manifests.git')
+ os.mkdir(gitdir)
+ with open(os.path.join(gitdir, 'config'), 'w') as fp:
+ fp.write("""[remote "origin"]
+ url = https://localhost:0/manifest
+""")
+
+ def tearDown(self):
+ shutil.rmtree(self.tempdir, ignore_errors=True)
+
+ def getXmlManifest(self, data):
+ """Helper to initialize a manifest for testing."""
+ with open(self.manifest_file, 'w') as fp:
+ fp.write(data)
+ return manifest_xml.XmlManifest(self.repodir, self.manifest_file)
+
+ def test_empty(self):
+ """Parse an 'empty' manifest file."""
+ manifest = self.getXmlManifest(
+ '<?xml version="1.0" encoding="UTF-8"?>'
+ '<manifest></manifest>')
+ self.assertEqual(manifest.remotes, {})
+ self.assertEqual(manifest.projects, [])
+
+ def test_link(self):
+ """Verify Link handling with new names."""
+ manifest = manifest_xml.XmlManifest(self.repodir, self.manifest_file)
+ with open(os.path.join(self.manifest_dir, 'foo.xml'), 'w') as fp:
+ fp.write('<manifest></manifest>')
+ manifest.Link('foo.xml')
+ with open(self.manifest_file) as fp:
+ self.assertIn('<include name="foo.xml" />', fp.read())
+
+ def test_toxml_empty(self):
+ """Verify the ToXml() helper."""
+ manifest = self.getXmlManifest(
+ '<?xml version="1.0" encoding="UTF-8"?>'
+ '<manifest></manifest>')
+ self.assertEqual(manifest.ToXml().toxml(), '<?xml version="1.0" ?><manifest/>')
+
+ def test_todict_empty(self):
+ """Verify the ToDict() helper."""
+ manifest = self.getXmlManifest(
+ '<?xml version="1.0" encoding="UTF-8"?>'
+ '<manifest></manifest>')
+ self.assertEqual(manifest.ToDict(), {})
+
+ def test_project_group(self):
+ """Check project group settings."""
+ manifest = self.getXmlManifest("""
+<manifest>
+ <remote name="test-remote" fetch="http://localhost" />
+ <default remote="test-remote" revision="refs/heads/main" />
+ <project name="test-name" path="test-path"/>
+ <project name="extras" path="path" groups="g1,g2,g1"/>
+</manifest>
+""")
+ self.assertEqual(len(manifest.projects), 2)
+ # Ordering isn't guaranteed.
+ result = {
+ manifest.projects[0].name: manifest.projects[0].groups,
+ manifest.projects[1].name: manifest.projects[1].groups,
+ }
+ project = manifest.projects[0]
+ self.assertCountEqual(
+ result['test-name'],
+ ['name:test-name', 'all', 'path:test-path'])
+ self.assertCountEqual(
+ result['extras'],
+ ['g1', 'g2', 'g1', 'name:extras', 'all', 'path:path'])
