Merge branch 'samthursfield/morph-update-improvements' of git://git.baserock.org/baserock/baserock/morph

3 files changed, 258 insertions, 148 deletions

diff --git a/morphlib/morph2.py b/morphlib/morph2.py
index 6e24765e..488b7d3b 100644
--- a/morphlib/morph2.py
+++ b/morphlib/morph2.py
@@ -66,14 +66,7 @@ class Morphology(object):
         f.write('\n')
 
     def __init__(self, text):
-        # Load as JSON first, then try YAML, so morphologies
-        # that read as JSON are dumped as JSON, likewise with YAML.
-        try:
-            self._dict = self._load_json(text)
-            self._dumper = self._dump_json
-        except Exception, e: # pragma: no cover
-            self._dict = morphlib.yamlparse.load(text)
-            self._dumper = morphlib.yamlparse.dump
+        self._dict, self._dumper = self._load_morphology_dict(text)
         self._set_defaults()
         self._validate_children()
 
@@ -86,6 +79,23 @@ class Morphology(object):
     def keys(self):
         return self._dict.keys()
 
+    def _load_morphology_dict(self, text):
+        '''Load morphology, identifying whether it is JSON or YAML'''
+
+        try:
+            data = self._load_json(text)
+            dumper = self._dump_json
+        except ValueError as e:  # pragma: no cover
+            data = morphlib.yamlparse.load(text)
+            dumper = morphlib.yamlparse.dump
+
+        if data is None:
+            raise morphlib.YAMLError("Morphology is empty")
+        if type(data) not in [dict, OrderedDict]:
+            raise morphlib.YAMLError("Morphology did not parse as a dict")
+
+        return data, dumper
+
     def _validate_children(self):
         if self['kind'] == 'system':
             names = set()
@@ -102,37 +112,30 @@ class Morphology(object):
                    raise ValueError('Duplicate chunk "%s"' % name)
                 names.add(name)
 
-    def lookup_child_by_name(self, name):
-        '''Find child reference by its name.
+    def _set_default_value(self, target_dict, key, value):
+        '''Change a value in the in-memory representation of the morphology
 
-        This lookup honors aliases.
+        Record the default value separately, so that when writing out the
+        morphology we can determine whether the change from the on-disk value
+        was done at load time, or later on (we want to only write back out
+        the later, deliberate changes).
 
         '''
-
-        if self['kind'] == 'system':
-            for info in self['strata']:
-                source_name = info.get('alias', info['morph'])
-                if source_name == name:
-                    return info
-        elif self['kind'] == 'stratum':
-            for info in self['chunks']:
-                source_name = info.get('alias', info['morph'])
-                if source_name == name:
-                    return info
-        raise KeyError('"%s" not found' % name)
+        target_dict[key] = value
+        target_dict['_orig_' + key] = value
 
     def _set_defaults(self):
         if 'max-jobs' in self:
-            self._dict['max-jobs'] = int(self['max-jobs'])
+            self._set_default_value(self._dict, 'max-jobs',
+                                    int(self['max-jobs']))
 
         if 'disk-size' in self:
-            size = self['disk-size']
-            self._dict['_disk-size'] = size
-            self._dict['disk-size'] = self._parse_size(size)
+            self._set_default_value(self._dict, 'disk-size',
+                                    self._parse_size(self['disk-size']))
 
         for name, value in self.static_defaults[self['kind']]:
             if name not in self._dict:
-                self._dict[name] = value
+                self._set_default_value(self._dict, name, value)
 
         if self['kind'] == 'stratum':
             self._set_stratum_defaults()
@@ -140,11 +143,11 @@ class Morphology(object):
     def _set_stratum_defaults(self):
         for source in self['chunks']:
             if 'repo' not in source:
-                source['repo'] = source['name']
+                self._set_default_value(source, 'repo', source['name'])
             if 'morph' not in source:
-                source['morph'] = source['name']
+                self._set_default_value(source, 'morph', source['name'])
             if 'build-depends' not in source:
-                source['build-depends'] = None
+                self._set_default_value(source, 'build-depends', None)
 
     def _parse_size(self, size):
         if isinstance(size, basestring):
@@ -157,23 +160,102 @@ class Morphology(object):
                 return int(size[:-1]) * 1024
         return int(size) # pragma: no cover
 
-    def write_to_file(self, f): # pragma: no cover
-        # Recreate dict without the empty default values, with a few kind
-        # specific hacks to try and edit standard morphologies as
-        # non-destructively as possible
-        as_dict = OrderedDict()
-        for key in self.keys():
-            if self['kind'] == 'stratum' and key == 'chunks':
-                value = copy.copy(self[key])
-                for chunk in value:
-                    if chunk["morph"] == chunk["name"]:
-                        del chunk["morph"]
-            if self['kind'] == 'system' and key == 'disk-size':
-                # Use human-readable value (assumes we never programmatically
-                # change this value within morph)
-                value = self['_disk-size']
+    def lookup_child_by_name(self, name):
+        '''Find child reference by its name.
+
+        This lookup honors aliases.
+
+        '''
+
+        if self['kind'] == 'system':
+            for info in self['strata']:
+                source_name = info.get('alias', info['morph'])
+                if source_name == name:
+                    return info
+        elif self['kind'] == 'stratum':
+            for info in self['chunks']:
+                source_name = info.get('alias', info['morph'])
+                if source_name == name:
+                    return info
+        raise KeyError('"%s" not found' % name)
+
+    def _apply_changes(self, live_dict, original_dict):
+        '''Returns a new dict updated with changes from the in-memory object
+
+        This allows us to write out a morphology including only the changes
+        that were done after the morphology was loaded -- not the changes done
+        to set default values during construction.
+
+        '''
+        output_dict = OrderedDict()
+
+        for key in live_dict.keys():
+            if key.startswith('_orig_'):
+                continue
+
+            value = self._apply_changes_for_key(key, live_dict, original_dict)
+            if value is not None:
+                output_dict[key] = value
+        return output_dict
+
+    def _apply_changes_for_key(self, key, live_dict, original_dict):
+        '''Return value to write out for one key, recursing if necessary'''
+
+        live_value = live_dict.get(key, None)
+        orig_value = original_dict.get(key, None)
+
+        if type(live_value) in [dict, OrderedDict] and orig_value is not None:
+            # Recursively apply changes for dict
+            result = self._apply_changes(live_value, orig_value)
+        elif type(live_value) is list and orig_value is not None:
+            # Recursively apply changes for list (existing, then new items).
+            result = []
+            for i in range(0, min(len(orig_value), len(live_value))):
+                if type(live_value[i]) in [dict, OrderedDict]:
+                    item = self._apply_changes(live_value[i], orig_value[i])
+                else:
+                    item = live_value[i]
+                result.append(item)
+            for i in range(len(orig_value), len(live_value)):
+                if type(live_value[i]) in [dict, OrderedDict]:
+                    item = self._apply_changes(live_value[i], {})
+                else:
+                    item = live_value[i]
+                result.append(item)
+        else:
+            # Simple values. Use original value unless it has been changed from
+            # the default in memmory.
+            if live_dict[key] == live_dict.get('_orig_' + key, None):
+                if key in original_dict:
+                    result = original_dict[key]
+                else:
+                    result = None
             else:
-                value = self[key]
-            if value and key[0] != '_':
-                as_dict[key] = value
-        self._dumper(as_dict, f)
+                result = live_dict[key]
+        return result
+
+    def update_text(self, text, output_fd):
+        '''Write out in-memory changes to loaded morphology text
+
+        Similar in function to update_file().
+
+        '''
+        original_dict, dumper = self._load_morphology_dict(text)
+
+        output_dict = self._apply_changes(self._dict, original_dict)
+
+        dumper(output_dict, output_fd)
+
+    def update_file(self, filename, output_fd=None): # pragma: no cover
+        '''Write out in-memory changes to on-disk morphology file
+
+        This function reads the original morphology text from 'filename', so
+        that it can avoid writing out properties that are set in memory
+        to their default value but weren't specified by the user at all.
+
+        '''
+        with open(filename, 'r') as f:
+            text = f.read()
+
+        with output_fd or morphlib.savefile.SaveFile(filename, 'w') as f:
+            self.update_text(text, f)
diff --git a/morphlib/morph2_tests.py b/morphlib/morph2_tests.py
index 7a819556..d2973d5c 100644
--- a/morphlib/morph2_tests.py
+++ b/morphlib/morph2_tests.py
@@ -14,6 +14,8 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 
+import copy
+import json
 import StringIO
 import unittest
 
@@ -164,6 +166,12 @@ class MorphologyTests(unittest.TestCase):
 
     ## Validation tests
 
+    def test_not_empty(self):
+        self.assertRaises(morphlib.YAMLError, Morphology, '')
+
+    def test_is_dict(self):
+        self.assertRaises(morphlib.YAMLError, Morphology, 'foo')
+
     def test_makes_max_jobs_be_an_integer(self):
         m = Morphology('''
             {
@@ -218,38 +226,9 @@ class MorphologyTests(unittest.TestCase):
                           Morphology,
                           text)
 
-    def test_writing_preserves_field_order(self):
-        text = '''{
-    "kind": "system",
-    "disk-size": 1073741824,
-    "description": "Some text",
-    "arch": "x86_64",
-    "system-kind": "syslinux-disk",
-    "strata": [
-        {
-            "morph": "foundation",
-            "repo": "morphs",
-            "ref": "ref"
-        },
-        {
-            "morph": "devel",
-            "repo": "morphs",
-            "ref": "ref"
-        }
-    ]
-}'''
-        morphology = Morphology(text)
-        output = StringIO.StringIO()
-        morphology.write_to_file(output)
-
-        text_lines = text.splitlines()
-        output_lines = output.getvalue().splitlines()
-
-        # Verify that input and output are equal.
-        self.assertEqual(text_lines, output_lines)
+    ## Writing tests
 
-    def test_writing_stratum_morphology_preserves_chunk_order(self):
-        text = '''{
+    stratum_text = '''{
     "kind": "stratum",
     "chunks": [
         {
@@ -262,33 +241,90 @@ class MorphologyTests(unittest.TestCase):
             "name": "bar",
             "repo": "morphs",
             "ref": "ref",
-            "build-depends": []
+            "build-depends": [
+                "foo"
+            ]
         }
     ]
 }'''
-        morphology = Morphology(text)
+
+    def test_writing_preserves_chunk_order(self):
+        text_lines = self.stratum_text.splitlines()
+        text_lines[6] = '            "ref": "new-ref",'
+
+        # Change one of the fields
+        morphology = Morphology(self.stratum_text)
+        morphology['chunks'][0]['ref'] = 'new-ref'
+
         output = StringIO.StringIO()
-        morphology.write_to_file(output)
+        morphology.update_text(self.stratum_text, output)
+        output_lines = output.getvalue().splitlines()
+        self.assertEqual(text_lines, output_lines)
+
+    def test_writing_handles_added_chunks(self):
+        text_lines = self.stratum_text.splitlines()
+        text_lines = text_lines[0:16] + text_lines[8:17] + text_lines[17:]
+        text_lines[18] = '            "name": "baz",'
+
+        # Add a new chunk to the list
+        morphology = Morphology(self.stratum_text)
+        morphology['chunks'].append(copy.copy(morphology['chunks'][1]))
+        morphology['chunks'][2]['name'] = 'baz'
 
-        text_lines = text.splitlines()
+        output = StringIO.StringIO()
+        morphology.update_text(self.stratum_text, output)
         output_lines = output.getvalue().splitlines()
+        self.assertEqual(text_lines, output_lines)
+
+    def test_writing_handles_deleted_chunks(self):
+        text_lines = self.stratum_text.splitlines()
+        text_lines = text_lines[0:3] + text_lines[9:]
 
-        # Verify that input and output are equal.
+        # Delete a chunk
+        morphology = Morphology(self.stratum_text)
+        del morphology['chunks'][0]
+
+        output = StringIO.StringIO()
+        morphology.update_text(self.stratum_text, output)
+        output_lines = output.getvalue().splitlines()
         self.assertEqual(text_lines, output_lines)
 
-    def test_writing_preserves_disk_size(self):
-        text = '''{
+    system_text = '''{
     "kind": "system",
     "disk-size": "1g",
     "arch": "x86_64",
     "system-kind": "syslinux-disk"
 }'''
-        morphology = Morphology(text)
-        output = StringIO.StringIO()
-        morphology.write_to_file(output)
 
-        text_lines = text.splitlines()
+    def test_writing_preserves_disk_size(self):
+        text_lines = self.system_text.splitlines()
+        morphology = Morphology(self.system_text)
+
+        output = StringIO.StringIO()
+        morphology.update_text(self.system_text, output)
         output_lines = output.getvalue().splitlines()
+        self.assertEqual(text_lines, output_lines)
+
+    def test_writing_updates_disk_size(self):
+        text_lines = self.system_text.splitlines()
+        text_lines[2] = '    "disk-size": 512,'
+
+        morphology = Morphology(self.system_text)
+        morphology._dict['disk-size'] = 512
 
-        # Verify that in- and output are the same.
+        output = StringIO.StringIO()
+        morphology.update_text(self.system_text, output)
+        output_lines = output.getvalue().splitlines()
         self.assertEqual(text_lines, output_lines)
+
+    def test_nested_dict(self):
+        # Real morphologies don't trigger this code path, so we test manually
+        original_dict = {
+            'dict': { '1': 'fee', '2': 'fie', '3': 'foe', '4': 'foo' }
+        }
+        live_dict = copy.deepcopy(original_dict)
+        live_dict['_orig_dict'] = live_dict['dict']
+
+        dummy = Morphology(self.stratum_text)
+        output_dict = dummy._apply_changes(live_dict, original_dict)
+        self.assertEqual(original_dict, output_dict)
diff --git a/morphlib/plugins/branch_and_merge_plugin.py b/morphlib/plugins/branch_and_merge_plugin.py
index e17ef740..f9595f98 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -361,7 +361,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
             
         known = required[kind] + also_known[kind]
         for field in morphology.keys():
-            if field not in known:
+            if field not in known and not field.startswith('_orig_'):
                 msg = 'Unknown field "%s" in %s' % (field, basename)
                 logging.warning(msg)
                 self.app.status(msg=msg)
@@ -378,18 +378,13 @@ class BranchAndMergePlugin(cliapp.Plugin):
                              (repo_dir, error))
 
     @staticmethod
-    def save_morphology(repo_dir, name, morphology):
+    def update_morphology(repo_dir, name, morphology, output_fd=None):
         if not name.endswith('.morph'):
             name = '%s.morph' % name
-        if os.path.isabs(name):
-            filename = name
-        else:
-            filename = os.path.join(repo_dir, name)
         filename = os.path.join(repo_dir, '%s' % name)
-        with morphlib.savefile.SaveFile(filename, 'w') as f:
-            morphology.write_to_file(f)
+        morphology.update_file(filename, output_fd=output_fd)
 
-        if name != morphology['name']:
+        if name != morphology['name'] + '.morph':
             logging.warning('%s: morphology "name" should match filename' %
                             filename)
 
@@ -657,7 +652,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                 # Bring the morphology forward from its ref to the current HEAD
                 repo = self.lrc.get_repo(root_repo)
                 m = repo.load_morphology(spec['ref'], spec['morph'])
-                self.save_morphology(root_repo_dir, spec['morph'], m)
+                self.update_morphology(root_repo_dir, spec['morph'], m)
                 self.log_change(spec['repo'],
                     '"%s" copied from "%s" to "%s"' %
                     (spec['morph'], spec['ref'], branch))
@@ -731,8 +726,8 @@ class BranchAndMergePlugin(cliapp.Plugin):
                                          stratum_spec['morph']))
             # Correct the System Morphology's reference
             stratum_spec['ref'] = branch
-            self.save_morphology(stratum_repo_dir, stratum_spec['morph'],
-                                 stratum_morphology)
+            self.update_morphology(stratum_repo_dir, stratum_spec['morph'],
+                                   stratum_morphology)
             self.log_change(root_repo,
                 '"%s" now includes "%s" from "%s"' %
                 (system_name, stratum_name, branch))
@@ -767,14 +762,14 @@ class BranchAndMergePlugin(cliapp.Plugin):
                         # Update the System morphology to use
                         # the modified version of the Stratum
                         stratum_spec['ref'] = branch
-                        self.save_morphology(stratum_repo_dir,
-                                             stratum_spec['morph'],
-                                             stratum_morphology)
+                        self.update_morphology(stratum_repo_dir,
+                                               stratum_spec['morph'],
+                                               stratum_morphology)
                         self.log_change(root_repo,
                             '"%s" now includes "%s" from "%s"' %
                             (system_name, stratum_name, branch))
 
-        self.save_morphology(root_repo_dir, system_name, system_morphology)
+        self.update_morphology(root_repo_dir, system_name, system_morphology)
 
         self.print_changelog('The following changes were made but have not '
                              'been committed')
@@ -921,7 +916,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                 else:
                     strata[key] = stratum_info['ref']
                     stratum_info['ref'] = branch
-            self.save_morphology(root_repo_dir, name, morphology)
+            self.update_morphology(root_repo_dir, name, morphology)
 
         for (repo, morph), ref in strata.iteritems():
             repo_dir = self.make_available(
@@ -939,7 +934,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                         update=not self.app.settings['no-git-update'])
                     chunk_info['unpetrify-ref'] = chunk_info['ref']
                     chunk_info['ref'] = commit_sha1
-            self.save_morphology(repo_dir, morph, stratum)
+            self.update_morphology(repo_dir, morph, stratum)
 
         self.print_changelog('The following changes were made but have not '
                              'been committed')
@@ -978,9 +973,10 @@ class BranchAndMergePlugin(cliapp.Plugin):
                     if 'unpetrify-ref' in chunk_info:
                         chunk_info['ref'] = chunk_info['unpetrify-ref']
                         del chunk_info['unpetrify-ref']
-                self.save_morphology(repo_dir, stratum_info['morph'], stratum)
+                self.update_morphology(repo_dir, stratum_info['morph'],
+                                       stratum)
 
-            self.save_morphology(root_repo_dir, name, morphology)
+            self.update_morphology(root_repo_dir, name, morphology)
 
         self.print_changelog('The following changes were made but have not '
                              'been committed')
@@ -1149,27 +1145,25 @@ class BranchAndMergePlugin(cliapp.Plugin):
 
         # Write the petrified morphology to a temporary file in the
         # branch root repository for inclusion in the tag commit.
-        handle, tmpfile = tempfile.mkstemp(suffix='.morph')
-        self.save_morphology(branch_root_dir, tmpfile, morphology)
-
-        # Hash the petrified morphology and add it to the index
-        # for the tag commit.
-        sha1 = self.app.runcmd(
-                ['git', 'hash-object', '-t', 'blob', '-w', tmpfile],
-                cwd=branch_root_dir, env=env)
-        self.app.runcmd(
-                ['git', 'update-index', '--add', '--cacheinfo',
-                 '100644', sha1, '%s.morph' % name],
-                cwd=branch_root_dir, env=env)
-
-        # Update the working tree if requested. This can be done with
-        # git-checkout-index, but we still have the file, so use that
-        if update_working_tree:
-            shutil.copy(tmpfile,
-                        os.path.join(branch_root_dir, '%s.morph' % name))
-
-        # Delete the temporary file again.
-        os.remove(tmpfile)
+        with tempfile.NamedTemporaryFile(suffix='.morph') as f:
+            self.update_morphology(
+                    repo_dir, name, morphology, output_fd=f.file)
+
+            # Hash the petrified morphology and add it to the index
+            # for the tag commit.
+            sha1 = self.app.runcmd(
+                    ['git', 'hash-object', '-t', 'blob', '-w', f.name],
+                    cwd=branch_root_dir, env=env)
+            self.app.runcmd(
+                    ['git', 'update-index', '--add', '--cacheinfo',
+                     '100644', sha1, '%s.morph' % name],
+                    cwd=branch_root_dir, env=env)
+
+            # Update the working tree if requested. This can be done with
+            # git-checkout-index, but we still have the file, so use that
+            if update_working_tree:
+                shutil.copy(f.name,
+                            os.path.join(branch_root_dir, '%s.morph' % name))
 
     def resolve_info(self, info, resolved_refs):
         '''Takes a morphology info and resolves its ref with cache support.'''
@@ -1414,7 +1408,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                 ci['ref'] = old_ci['ref']
                 merge_chunk(path, old_ci, ci)
             if changed:
-                self.save_morphology(to_repo_dir, si['morph'], to_morph)
+                self.update_morphology(to_repo_dir, si['morph'], to_morph)
                 self.app.runcmd(['git', 'add', si['morph'] + '.morph'],
                                 cwd=to_repo_dir)
 
@@ -1447,7 +1441,7 @@ class BranchAndMergePlugin(cliapp.Plugin):
                 si['ref'] = old_si['ref']
                 merge_stratum(name, old_si, si)
             if changed:
-                self.save_morphology(to_root_dir, name, to_morph)
+                self.update_morphology(to_root_dir, name, to_morph)
                 self.app.runcmd(['git', 'add', f], cwd=to_root_dir)
 
         merged_repos = {}
@@ -1684,20 +1678,18 @@ class BranchAndMergePlugin(cliapp.Plugin):
                 # Inject temporary refs in the right places in each morphology.
                 morphology = self.load_morphology(repo_dir, filename)
                 self.inject_build_refs(morphology, build_repos, will_push)
-                handle, tmpfile = tempfile.mkstemp(suffix='.morph')
-                self.save_morphology(repo_dir, tmpfile, morphology)
-
-                morphology_sha1 = self.app.runcmd(
-                        ['git', 'hash-object', '-t', 'blob', '-w', tmpfile],
-                        cwd=repo_dir, env=env)
+                with tempfile.NamedTemporaryFile(suffix='.morph') as f:
+                    self.update_morphology(
+                            repo_dir, filename, morphology, output_fd=f.file)
 
-                self.app.runcmd(
-                        ['git', 'update-index', '--cacheinfo',
-                         '100644', morphology_sha1, '%s.morph' % filename],
-                        cwd=repo_dir, env=env)
+                    morphology_sha1 = self.app.runcmd(
+                            ['git', 'hash-object', '-t', 'blob', '-w', f.name],
+                            cwd=repo_dir, env=env)
 
-                # Remove the temporary morphology file.
-                os.remove(tmpfile)
+                    self.app.runcmd(
+                            ['git', 'update-index', '--cacheinfo',
+                             '100644', morphology_sha1, '%s.morph' % filename],
+                            cwd=repo_dir, env=env)
 
             # Create a commit message including the build UUID. This allows us
             # to collect all commits of a build across repositories and thereby