Merge branch 'baserock/richardmaw/S8847/cleanup-petrify-v3'

Reviewed-by: Lars Wirzenius
Reviewed-by: Daniel Silverstone

4 files changed, 215 insertions, 69 deletions

diff --git a/morphlib/morphset.py b/morphlib/morphset.py
index c256760e..3c07d58e 100644
--- a/morphlib/morphset.py
+++ b/morphlib/morphset.py
@@ -122,6 +122,66 @@ class MorphologySet(object):
             raise ChunkNotInStratumError(stratum_morph['name'], chunk_name)
         return repo_url, ref, morph
 
+    def _traverse_specs(self, cb_process, cb_filter=lambda s: True):
+        '''Higher-order function for processing every spec.
+        
+        This traverses every spec in all the morphologies, so all chunk,
+        stratum and stratum-build-depend specs are visited.
+
+        It is to be passed one or two callbacks. `cb_process` is given
+        a spec, which it may alter, but if it does, it must return True.
+
+        `cb_filter` is given the morphology, the kind of spec it is
+        working on in addition to the spec itself.
+
+        `cb_filter` is expected to decide whether to run `cb_process`
+        on the spec.
+
+        Arguably this could be checked in `cb_process`, but it can be less
+        logic over all since `cb_process` need not conditionally return.
+
+        If any specs have been altered, at the end of iteration, any
+        morphologies in the MorphologySet that are referred to by an
+        altered spec are also changed.
+
+        This requires a full iteration of the MorphologySet, so it is not a
+        cheap operation.
+
+        A coroutine was attempted, but it required the same amount of
+        code at the call site as doing it by hand.
+        
+        '''
+
+        altered_references = {}
+
+        def process_spec_list(m, kind):
+            specs = m[kind]
+            for spec in specs:
+                if cb_filter(m, kind, spec):
+                    orig_spec = (spec['repo'], spec['ref'], spec['morph'])
+                    dirtied = cb_process(m, kind, spec)
+                    if dirtied:
+                        m.dirty = True
+                        altered_references[orig_spec] = spec
+
+        for m in self.morphologies:
+            if m['kind'] == 'system':
+                process_spec_list(m, 'strata')
+            elif m['kind'] == 'stratum':
+                process_spec_list(m, 'build-depends')
+                process_spec_list(m, 'chunks')
+
+        for m in self.morphologies:
+            tup = (m.repo_url, m.ref, m.filename[:-len('.morph')])
+            if tup in altered_references:
+                spec = altered_references[tup]
+                if m.ref != spec['ref']:
+                    m.ref = spec['ref']
+                    m.dirty = True
+                assert (m.filename == spec['morph'] + '.morph'
+                        or m.repo_url == spec['repo']), \
+                       'Moving morphologies is not supported.'
+
     def change_ref(self, repo_url, orig_ref, morph_filename, new_ref):
         '''Change a triplet's ref to a new one in all morphologies in a ref.
 
@@ -130,30 +190,98 @@ class MorphologySet(object):
 
         '''
 
-        def wanted_spec(spec):
+        def wanted_spec(m, kind, spec):
             return (spec['repo'] == repo_url and
                     spec['ref'] == orig_ref and
                     spec['morph'] + '.morph' == morph_filename)
 
-        def change_specs(specs, m):
-            for spec in specs:
-                if wanted_spec(spec):
-                    spec['unpetrify-ref'] = spec['ref']
-                    spec['ref'] = new_ref
-                    m.dirty = True
+        def process_spec(m, kind, spec):
+            spec['unpetrify-ref'] = spec['ref']
+            spec['ref'] = new_ref
+            return True
 
-        def change(m):
-            if m['kind'] == 'system':
-                change_specs(m['strata'], m)
-            elif m['kind'] == 'stratum':
-                change_specs(m['chunks'], m)
-                change_specs(m['build-depends'], m)
+        self._traverse_specs(process_spec, wanted_spec)
 
-        for m in self.morphologies:
-            change(m)
+    def list_refs(self):
+        '''Return a set of all the (repo, ref) pairs in the MorphologySet.
+
+        This does not dirty the morphologies so they do not need to be
+        written back to the disk.
+
+        '''
+        known = set()
+
+        def wanted_spec(m, kind, spec):
+            return (spec['repo'], spec['ref']) not in known
+
+        def process_spec(m, kind, spec):
+            known.add((spec['repo'], spec['ref']))
+            return False
+
+        self._traverse_specs(process_spec, wanted_spec)
+
+        return known
+
+    def repoint_refs(self, repo_url, new_ref):
+        '''Change all specs which refer to (repo, *) to (repo, new_ref).
+        
+        This is stunningly similar to change_ref, with the exception of
+        ignoring the morphology name and ref fields.
+
+        It is intended to be used before chunks are petrified
+
+        '''
+        def wanted_spec(m, kind, spec):
+            return spec['repo'] == repo_url
+
+        def process_spec(m, kind, spec):
+            if 'unpetrify-ref' not in spec:
+                spec['unpetrify-ref'] = spec['ref']
+            spec['ref'] = new_ref
+            return True
+
+        self._traverse_specs(process_spec, wanted_spec)
+
+    def petrify_chunks(self, resolutions):
+        '''Update _every_ chunk's ref to the value resolved in resolutions.
+
+        `resolutions` must be a {(repo, ref): resolved_ref}
+
+        This is subtly different to change_ref, since that works on
+        changing a single spec including its filename, and the morphology
+        those specs refer to, while petrify_chunks is interested in changing
+        _all_ the refs.
+
+        '''
+
+        def wanted_chunk_spec(m, kind, spec):
+            # Do not attempt to petrify non-chunk specs.
+            # This is not handled by previous implementations, and
+            # the details are tricky.
+            if not (m['kind'] == 'stratum' and kind == 'chunks'):
+                return
+            ref = spec['ref']
+            return (not morphlib.git.is_valid_sha1(ref)
+                    and (spec['repo'], ref) in resolutions)
+
+        def process_chunk_spec(m, kind, spec):
+            tup = (spec['repo'], spec['ref'])
+            spec['unpetrify-ref'] = spec['ref']
+            spec['ref'] = resolutions[tup]
+            return True
+
+        self._traverse_specs(process_chunk_spec, wanted_chunk_spec)
+
+    def unpetrify_all(self):
+        '''If a spec is petrified, unpetrify it.
+
+        '''
 
-        m = self._get_morphology(repo_url, orig_ref, morph_filename)
-        if m and m.ref != new_ref:
-            m.ref = new_ref
-            m.dirty = True
+        def wanted_spec(m, kind, spec):
+            return ('unpetrify-ref' in spec and
+                    morphlib.git.is_valid_sha1(spec['ref']))
+        def process_spec(m, kind, spec):
+            spec['ref'] = spec.pop('unpetrify-ref')
+            return True
 
+        self._traverse_specs(process_spec, wanted_spec)
diff --git a/morphlib/morphset_tests.py b/morphlib/morphset_tests.py
index 65fe2058..d6908844 100644
--- a/morphlib/morphset_tests.py
+++ b/morphlib/morphset_tests.py
@@ -145,6 +145,9 @@ class MorphologySetTests(unittest.TestCase):
                 },
             ]
         })
+        other_stratum.repo_url = 'test:morphs'
+        other_stratum.ref = 'master'
+        other_stratum.filename = 'other-stratum.morph'
 
         self.morphs.add_morphology(self.system)
         self.morphs.add_morphology(self.stratum)
@@ -182,3 +185,54 @@ class MorphologySetTests(unittest.TestCase):
                 }
             ])
 
+    def test_list_refs(self):
+        self.morphs.add_morphology(self.system)
+        self.morphs.add_morphology(self.stratum)
+        self.assertEqual(sorted(self.morphs.list_refs()),
+                         [('test:foo-chunk', 'master'),
+                          ('test:morphs', 'master')])
+
+    def test_repoint_refs(self):
+        self.morphs.add_morphology(self.system)
+        self.morphs.add_morphology(self.stratum)
+        self.morphs.repoint_refs('test:morphs', 'test')
+        self.assertEqual(self.system['strata'],
+                         [
+                             {
+                                 'morph': 'foo-stratum',
+                                 'ref': 'test',
+                                 'repo': 'test:morphs',
+                                 'unpetrify-ref': 'master',
+                             }
+                         ])
+
+    def test_petrify_chunks(self):
+        # TODO: test petrifying a larger morphset
+        self.morphs.add_morphology(self.system)
+        self.morphs.add_morphology(self.stratum)
+        self.morphs.petrify_chunks({('test:foo-chunk', 'master'): '0'*40})
+        self.assertEqual(
+            self.stratum['chunks'],
+            [
+                {
+                    'repo': 'test:foo-chunk',
+                    'ref': '0'*40,
+                    'morph': 'foo-chunk',
+                    'unpetrify-ref': 'master',
+                }
+            ])
+
+    def test_unpetrify_all(self):
+        self.morphs.add_morphology(self.system)
+        self.morphs.add_morphology(self.stratum)
+        self.morphs.petrify_chunks({('test:foo-chunk', 'master'): '0'*40})
+        self.morphs.unpetrify_all()
+        self.assertEqual(
+            self.stratum['chunks'],
+            [
+                {
+                    'repo': 'test:foo-chunk',
+                    'ref': 'master',
+                    'morph': 'foo-chunk',
+                }
+            ])
diff --git a/morphlib/plugins/branch_and_merge_new_plugin.py b/morphlib/plugins/branch_and_merge_new_plugin.py
index 7710f309..39552ef0 100644
--- a/morphlib/plugins/branch_and_merge_new_plugin.py
+++ b/morphlib/plugins/branch_and_merge_new_plugin.py
@@ -673,29 +673,24 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
         loader = morphlib.morphloader.MorphologyLoader()
         lrc, rrc = morphlib.util.new_repo_caches(self.app)
         update_repos = not self.app.settings['no-git-update']
-        done = set()
 
         morphs = self._load_all_sysbranch_morphologies(sb, loader)
 
-        # Petrify the ref to each stratum and chunk.
-        def petrify_specs(specs):
-            for spec in specs:
-                ref = spec['ref']
-                # Do not double petrify refs
-                if morphlib.git.is_valid_sha1(ref):
+        #TODO: Stop using app.resolve_ref
+        def resolve_refs(morphs):
+            for repo, ref in morphs.list_refs():
+                # TODO: Handle refs that are only in workspace in general
+                if (repo == sb.root_repository_url
+                    and ref == sb.system_branch_name):
                     continue
                 commit_sha1, tree_sha1 = self.app.resolve_ref(
-                    lrc, rrc, spec['repo'], ref, update=update_repos)
-                assert 'name' in spec or 'morph' in spec
-                filename = '%s.morph' % spec.get('morph', spec.get('name'))
-                morphs.change_ref(spec['repo'], ref, filename, commit_sha1)
+                    lrc, rrc, repo, ref, update=update_repos)
+                yield ((repo, ref), commit_sha1)
 
-        for m in morphs.morphologies:
-            if m['kind'] == 'system':
-                petrify_specs(m['strata'])
-            elif m['kind'] == 'stratum':
-                petrify_specs(m['build-depends'])
-                petrify_specs(m['chunks'])
+        morphs.repoint_refs(sb.root_repository_url,
+                            sb.system_branch_name)
+
+        morphs.petrify_chunks(dict(resolve_refs(morphs)))
 
         # Write morphologies back out again.
         self._save_dirty_morphologies(loader, sb, morphs.morphologies)
@@ -713,33 +708,11 @@ class SimpleBranchAndMergePlugin(cliapp.Plugin):
         ws = morphlib.workspace.open('.')
         sb = morphlib.sysbranchdir.open_from_within('.')
         loader = morphlib.morphloader.MorphologyLoader()
-        lrc, rrc = morphlib.util.new_repo_caches(self.app)
-        update_repos = not self.app.settings['no-git-update']
-        done = set()
 
         morphs = self._load_all_sysbranch_morphologies(sb, loader)
 
         # Restore the ref for each stratum and chunk
-        def unpetrify_specs(specs):
-            dirty = False
-            for spec in specs:
-                ref = spec['ref']
-                # Don't attempt to unpetrify refs which aren't petrified
-                if not ('unpetrify-ref' in spec
-                        and morphlib.git.is_valid_sha1(ref)):
-                    continue
-                spec['ref'] = spec.pop('unpetrify-ref')
-                dirty = True
-            return dirty
-
-        for m in morphs.morphologies:
-            dirty = False
-            if m['kind'] == 'system':
-                dirty = dirty or unpetrify_specs(m['strata'])
-            elif m['kind'] == 'stratum':
-                dirty = dirty or unpetrify_specs(m['build-depends'])
-                dirty = dirty or unpetrify_specs(m['chunks'])
-            m.dirty = True
+        morphs.unpetrify_all()
 
         # Write morphologies back out again.
         self._save_dirty_morphologies(loader, sb, morphs.morphologies)
diff --git a/yarns/branches-workspaces.yarn b/yarns/branches-workspaces.yarn
index 3af362a1..5273f396 100644
--- a/yarns/branches-workspaces.yarn
+++ b/yarns/branches-workspaces.yarn
@@ -87,15 +87,6 @@ to check for that locally.
     AND         creating system branch bar, based on foo
     THEN        the system branch bar is checked out
 
-Similarly, attempting to branch a system branch should fail if the
-repository does not contain any system morphologies.
-
-    SCENARIO    checking out a system branch with no systems
-    GIVEN       a workspace
-    AND         a git server
-    WHEN        morph attempts to branch a repository with no systems
-    THEN        morph failed
-
 Query commands in workspaces
 ----------------------------
 
@@ -177,7 +168,7 @@ somewhere outside a checkout, where exactly one checkout exists below.
 However, it fails if run outside a checkout and there's no system
 branches checked out.
 
-    SCENARIO    morph fails to report system branch with two checked out
+    SCENARIO    morph fails to report system branch with none checked out
     GIVEN       a workspace
     AND         a git server
     WHEN        attempting to report system branch root repository in .