From 8c1e9cbef161f2ff12dadbacf26affd23876fde9 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Sun, 6 Sep 2020 14:53:18 -0400 Subject: [PATCH] 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 Tested-by: Mike Frysinger --- main.py | 13 +++--- manifest_xml.py | 80 +++++++++++++++++++++++----------- project.py | 6 +-- subcmds/diffmanifests.py | 8 ++-- subcmds/help.py | 4 +- subcmds/info.py | 2 +- subcmds/init.py | 4 +- subcmds/status.py | 2 +- tests/test_manifest_xml.py | 89 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 164 insertions(+), 44 deletions(-) diff --git a/main.py b/main.py index cd62793d..e152de4f 100755 --- a/main.py +++ b/main.py @@ -63,7 +63,7 @@ from error import NoManifestException 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 @@ class _Repo(object): 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 @@ class _Repo(object): 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 e1ef330f..95c67d73 100644 --- a/manifest_xml.py +++ b/manifest_xml.py @@ -187,12 +187,24 @@ class _XmlRemote(object): 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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md 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 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md 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 @@ class GitcManifest(XmlManifest): """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 50bb53c3..29f6b1e8 100644 --- a/project.py +++ b/project.py @@ -510,7 +510,7 @@ class Project(object): 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 @@ class Project(object): 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 @@ class Project(object): 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 72c441ac..409bbdac 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 @@ synced and their revisions won't be found. 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 @@ synced and their revisions won't be found. 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 1e16019a..c219a763 100644 --- a/subcmds/help.py +++ b/subcmds/help.py @@ -65,7 +65,7 @@ Displays detailed usage information about a command. 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 @@ Displays detailed usage information about a command. 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 f4d6f98c..60149975 100644 --- a/subcmds/info.py +++ b/subcmds/info.py @@ -44,7 +44,7 @@ class Info(PagedCommand): 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 5ba0d074..f46babfe 100644 --- a/subcmds/init.py +++ b/subcmds/init.py @@ -365,7 +365,7 @@ to update the working directory files. 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 @@ to update the working directory files. 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 c380dba3..dfa974e9 100644 --- a/subcmds/status.py +++ b/subcmds/status.py @@ -165,7 +165,7 @@ the following meanings: 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 aa6cb7df..40385cce 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 @@ class ValueTests(unittest.TestCase): with self.assertRaises(error.ManifestParseError): node = self._get_node('') 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( + '' + '') + 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.Link('foo.xml') + with open(self.manifest_file) as fp: + self.assertIn('', fp.read()) + + def test_toxml_empty(self): + """Verify the ToXml() helper.""" + manifest = self.getXmlManifest( + '' + '') + self.assertEqual(manifest.ToXml().toxml(), '') + + def test_todict_empty(self): + """Verify the ToDict() helper.""" + manifest = self.getXmlManifest( + '' + '') + self.assertEqual(manifest.ToDict(), {}) + + def test_project_group(self): + """Check project group settings.""" + manifest = self.getXmlManifest(""" + + + + + + +""") + 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'])