Merge remote-tracking branch 'origin/baserock/richardmaw/S11416/no-unnecessary-temp-branches'

Reviewed-by: Daniel Silverstone

7 files changed, 157 insertions, 126 deletions

diff --git a/morphlib/app.py b/morphlib/app.py
index a543443e..88eb58a4 100644
--- a/morphlib/app.py
+++ b/morphlib/app.py
@@ -518,16 +518,15 @@ class Morph(cliapp.Application):
         self.output.write(text)
 
     def _help_topic(self, topic):
-        build_ref_prefix = self.settings['build-ref-prefix']
         if topic in self.subcommands:
             usage = self._format_usage_for(topic)
             description = self._format_subcommand_help(topic)
             text = '%s\n\n%s' % (usage, description)
             self.output.write(text)
-        elif topic in extensions.list_extensions(build_ref_prefix):
+        elif topic in extensions.list_extensions():
             name, kind = os.path.splitext(topic)
             try:
-                with extensions.get_extension_filename(build_ref_prefix,
+                with extensions.get_extension_filename(
                         name,
                         kind + '.help', executable=False) as fname:
                     with open(fname, 'r') as f:
diff --git a/morphlib/buildbranch.py b/morphlib/buildbranch.py
index d415e7e1..885f5cf8 100644
--- a/morphlib/buildbranch.py
+++ b/morphlib/buildbranch.py
@@ -15,6 +15,7 @@
 
 
 import collections
+import contextlib
 import os
 import urlparse
 
@@ -48,10 +49,9 @@ class BuildBranch(object):
     # would be better to not use local repositories and temporary refs,
     # so building from a workspace appears to be identical to using
     # `morph build-morphology`
-    def __init__(self, sb, build_ref_prefix, push_temporary):
+    def __init__(self, sb, build_ref_prefix):
 
         self._sb = sb
-        self._push_temporary = push_temporary
 
         self._cleanup = collections.deque()
         self._to_push = {}
@@ -84,15 +84,18 @@ class BuildBranch(object):
     def _register_cleanup(self, func, *args, **kwargs):
         self._cleanup.append((func, args, kwargs))
 
-    def add_uncommitted_changes(self):
+    def add_uncommitted_changes(self, add_cb=lambda **kwargs: None):
         '''Add any uncommitted changes to temporary build GitIndexes'''
+        changes_made = False
         for gd, (build_ref, index) in self._to_push.iteritems():
             changed = [to_path for code, to_path, from_path
                        in index.get_uncommitted_changes()]
             if not changed:
                 continue
-            yield gd, build_ref
+            add_cb(gd=gd, build_ref=gd, changed=changed)
+            changes_made = True
             index.add_files_from_working_tree(changed)
+        return changes_made
 
     @staticmethod
     def _hash_morphologies(gd, morphologies, loader):
@@ -102,7 +105,8 @@ class BuildBranch(object):
             sha1 = gd.store_blob(loader.save_to_string(morphology))
             yield 0100644, sha1, morphology.filename
 
-    def inject_build_refs(self, loader):
+    def inject_build_refs(self, loader, use_local_repos,
+                          inject_cb=lambda **kwargs: None):
         '''Update system and stratum morphologies to point to our branch.
 
         For all edited repositories, this alter the temporary GitIndex
@@ -133,7 +137,7 @@ class BuildBranch(object):
                 spec['repo'] = None
                 spec['ref'] = None
                 return True
-            if not self._push_temporary:
+            if use_local_repos:
                 spec['repo'] = urlparse.urljoin('file://', gd.dirname)
             spec['ref'] = build_ref
             return True
@@ -141,12 +145,15 @@ class BuildBranch(object):
         morphs.traverse_specs(process, filter)
 
         if any(m.dirty for m in morphs.morphologies):
-            yield self._root
+            inject_cb(gd=self._root)
 
+        # TODO: Prevent it hashing unchanged morphologies, while still
+        # hashing uncommitted ones.
         self._root_index.add_files_from_index_info(
             self._hash_morphologies(self._root, morphs.morphologies, loader))
 
-    def update_build_refs(self, name, email, uuid):
+    def update_build_refs(self, name, email, uuid,
+                          commit_cb=lambda **kwargs: None):
         '''Commit changes in temporary GitIndexes to temporary branches.
 
         `name` and `email` are required to construct the commit author info.
@@ -176,12 +183,18 @@ class BuildBranch(object):
 
         with morphlib.branchmanager.LocalRefManager() as lrm:
             for gd, (build_ref, index) in self._to_push.iteritems():
-                yield gd, build_ref
                 tree = index.write_tree()
                 try:
                     parent = gd.resolve_ref_to_commit(build_ref)
                 except morphlib.gitdir.InvalidRefError:
                     parent = gd.resolve_ref_to_commit(gd.HEAD)
+                else:
+                    # Skip updating ref if we already have a temporary
+                    # build branch and have this tree on the branch
+                    if tree == gd.resolve_ref_to_tree(build_ref):
+                        continue
+
+                commit_cb(gd=gd, build_ref=build_ref)
 
                 commit = gd.commit_tree(tree, parent=parent,
                                         committer_name=committer_name,
@@ -201,46 +214,55 @@ class BuildBranch(object):
                     #       a problem.
                     lrm.update(gd, build_ref, commit, old_commit)
 
-    def push_build_branches(self):
-        '''Push all temporary build branches to the remote repositories.
+    def get_unpushed_branches(self):
+        '''Work out which, if any, local branches need to be pushed to build
 
-        This is a no-op if the BuildBranch was constructed with
-        `push_temporary` as False, so that the code flow for the user of
-        the BuildBranch can be the same when it can be pushed as when
-        it can't.
+        NOTE: This assumes that the refs in the morphologies and the
+        refs in the local checkouts match.
 
         '''
-        # TODO: When BuildBranches become more context aware, if there
-        # are no uncommitted changes and the local versions are pushed
-        # we can skip pushing even if push_temporary is set.
-        # No uncommitted changes isn't sufficient reason to push the
-        # current HEAD
-        if self._push_temporary:
-            with morphlib.branchmanager.RemoteRefManager(False) as rrm:
-                for gd, (build_ref, index) in self._to_push.iteritems():
-                    remote = gd.get_remote('origin')
-                    yield gd, build_ref, remote
-                    refspec = morphlib.gitdir.RefSpec(build_ref)
-                    rrm.push(remote, refspec)
-            self._register_cleanup(rrm.close)
+        for gd, (build_ref, index) in self._to_push.iteritems():
+            remote = gd.get_remote('origin')
+            head_ref = gd.disambiguate_ref(gd.HEAD)
+            head_sha1 = gd.resolve_ref_to_commit(head_ref)
+            pushed_refs = sorted(
+                    (remote_ref
+                     for remote_sha1, remote_ref in remote.ls()
+                     # substring match of refs, since ref may be a tag,
+                     # in which case it would end with ^{}
+                     if remote_sha1 == head_sha1 and head_ref in remote_ref),
+                    key=len)
+            if not pushed_refs:
+                yield gd
+
+    def push_build_branches(self, push_cb=lambda **kwargs: None):
+        '''Push all temporary build branches to the remote repositories.
+        '''
+        with morphlib.branchmanager.RemoteRefManager(False) as rrm:
+            for gd, (build_ref, index) in self._to_push.iteritems():
+                remote = gd.get_remote('origin')
+                refspec = morphlib.gitdir.RefSpec(build_ref)
+                push_cb(gd=gd, build_ref=build_ref,
+                        remote=remote, refspec=refspec)
+                rrm.push(remote, refspec)
+        self._register_cleanup(rrm.close)
 
     @property
     def root_repo_url(self):
         '''URI of the repository that systems may be found in.'''
-        # TODO: When BuildBranches become more context aware, we only
-        # have to use the file:// URI when there's uncommitted changes
-        # and we can't push; or HEAD is not pushed and we can't push.
-        # All other times we can use the pushed branch
-        return (self._sb.get_config('branch.root') if self._push_temporary
-                else urlparse.urljoin('file://', self._root.dirname))
+        return self._sb.get_config('branch.root')
 
     @property
     def root_ref(self):
+        return self._sb.get_config('branch.name')
+
+    @property
+    def root_local_repo_url(self):
+        return urlparse.urljoin('file://', self._root.dirname)
+
+    @property
+    def root_build_ref(self):
         '''Name of the ref of the repository that systems may be found in.'''
-        # TODO: When BuildBranches become more context aware, this can be
-        # HEAD when there's no uncommitted changes and we're not pushing;
-        # or we are pushing and there's no uncommitted changes and HEAD
-        # has been pushed.
         build_ref, index = self._to_push[self._root]
         return build_ref
 
@@ -258,3 +280,47 @@ class BuildBranch(object):
                 exceptions.append(e)
         if exceptions:
             raise BuildBranchCleanupError(self, exceptions)
+
+
+@contextlib.contextmanager
+def pushed_build_branch(bb, loader, changes_need_pushing, name, email,
+                        build_uuid, status):
+    with contextlib.closing(bb) as bb:
+        def report_add(gd, build_ref, changed):
+            status(msg='Adding uncommitted changes '\
+                           'in %(dirname)s to %(ref)s',
+                       dirname=gd.dirname, ref=build_ref, chatty=True)
+        changes_made = bb.add_uncommitted_changes(add_cb=report_add)
+        unpushed = any(bb.get_unpushed_branches())
+
+        if not changes_made and not unpushed:
+            yield bb.root_repo_url, bb.root_ref
+            return
+
+        def report_inject(gd):
+            status(msg='Injecting temporary build refs '\
+                           'into morphologies in %(dirname)s',
+                       dirname=gd.dirname, chatty=True)
+        bb.inject_build_refs(loader=loader,
+                             use_local_repos=not changes_need_pushing,
+                             inject_cb=report_inject)
+
+        def report_commit(gd, build_ref):
+            status(msg='Committing changes in %(dirname)s '\
+                           'to %(ref)s',
+                       dirname=gd.dirname, ref=build_ref,
+                       chatty=True)
+        bb.update_build_refs(name, email, build_uuid,
+                             commit_cb=report_commit)
+
+        if changes_need_pushing:
+            def report_push(gd, build_ref, remote, refspec):
+                status(msg='Pushing %(ref)s in %(dirname)s '\
+                               'to %(remote)s',
+                           ref=build_ref, dirname=gd.dirname,
+                           remote=remote.get_push_url(), chatty=True)
+            bb.push_build_branches(push_cb=report_push)
+
+            yield bb.root_repo_url, bb.root_build_ref
+        else:
+            yield bb.root_local_repo_url, bb.root_build_ref
diff --git a/morphlib/extensions.py b/morphlib/extensions.py
index be551fdd..55478418 100644
--- a/morphlib/extensions.py
+++ b/morphlib/extensions.py
@@ -30,26 +30,20 @@ class ExtensionNotFoundError(ExtensionError):
 class ExtensionNotExecutableError(ExtensionError):
     pass
 
-def _get_root_repo(build_ref_prefix):
+def _get_root_repo():
     system_branch = morphlib.sysbranchdir.open_from_within('.')
     root_repo_dir = morphlib.gitdir.GitDirectory(
             system_branch.get_git_directory_name(
                 system_branch.root_repository_url))
-    build_branch = morphlib.buildbranch.BuildBranch(system_branch,
-                                                    build_ref_prefix,
-                                                    push_temporary=False)
-    ref = build_branch.root_ref
-
-    return (build_branch.root_ref, root_repo_dir)
+    return root_repo_dir
 
 def _get_morph_extension_directory():
     code_dir = os.path.dirname(morphlib.__file__)
     return os.path.join(code_dir, 'exts')
 
-def _list_repo_extension_filenames(build_ref_prefix,
-                                   kind): #pragma: no cover
-    (ref, repo_dir) = _get_root_repo(build_ref_prefix)
-    files = repo_dir.list_files(ref)
+def _list_repo_extension_filenames(kind): #pragma: no cover
+    repo_dir = _get_root_repo()
+    files = repo_dir.list_files()
     return (f for f in files if os.path.splitext(f)[1] == kind)
 
 def _list_morph_extension_filenames(kind):
@@ -59,9 +53,9 @@ def _list_morph_extension_filenames(kind):
 def _get_extension_name(filename):
     return os.path.basename(filename)
 
-def _get_repo_extension_contents(build_ref_prefix, name, kind):
-    (ref, repo_dir) = _get_root_repo(build_ref_prefix)
-    return repo_dir.get_file_from_ref(ref, name + kind)
+def _get_repo_extension_contents(name, kind):
+    repo_dir = _get_root_repo()
+    return repo_dir.read_file(name + kind)
 
 def _get_morph_extension_filename(name, kind):
     return os.path.join(_get_morph_extension_directory(), name + kind)
@@ -71,11 +65,11 @@ def _is_executable(filename):
     mask = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
     return (stat.S_IMODE(st.st_mode) & mask) != 0
 
-def _list_extensions(build_ref_prefix, kind):
+def _list_extensions(kind):
     repo_extension_filenames = []
     try:
         repo_extension_filenames = \
-                _list_repo_extension_filenames(build_ref_prefix, kind)
+                _list_repo_extension_filenames(kind)
     except (sysbranchdir.NotInSystemBranch):
         # Squash this and just return no system branch extensions
         pass
@@ -90,7 +84,7 @@ def _list_extensions(build_ref_prefix, kind):
     extension_names.update(set(morph_extension_names))
     return list(extension_names)
 
-def list_extensions(build_ref_prefix, kind=None):
+def list_extensions(kind=None):
     """
     List all available extensions by 'kind'.
 
@@ -102,10 +96,10 @@ def list_extensions(build_ref_prefix, kind=None):
     be associated with a '.write' extension of the same name.
     """
     if kind:
-        return _list_extensions(build_ref_prefix, kind)
+        return _list_extensions(kind)
     else:
-        configure_extensions = _list_extensions(build_ref_prefix, '.configure')
-        write_extensions = _list_extensions(build_ref_prefix, '.write')
+        configure_extensions = _list_extensions('.configure')
+        write_extensions = _list_extensions('.write')
 
         return configure_extensions + write_extensions
 
@@ -121,8 +115,7 @@ class get_extension_filename():
     If the extension is in the build repository then a temporary
     file will be created, which will be deleted on exting the with block.
     """
-    def __init__(self, build_ref_prefix, name, kind, executable=True):
-        self.build_ref_prefix = build_ref_prefix
+    def __init__(self, name, kind, executable=True):
         self.name = name
         self.kind = kind
         self.executable = executable
@@ -131,10 +124,9 @@ class get_extension_filename():
     def __enter__(self):
         ext_filename = None
         try:
-            ext_contents = _get_repo_extension_contents(self.build_ref_prefix,
-                                                        self.name,
+            ext_contents = _get_repo_extension_contents(self.name,
                                                         self.kind)
-        except cliapp.AppException, sysbranchdir.NotInSystemBranch:
+        except (IOError, cliapp.AppException, sysbranchdir.NotInSystemBranch):
             # Not found: look for it in the Morph code.
             ext_filename = _get_morph_extension_filename(self.name, self.kind)
             if not os.path.exists(ext_filename):
diff --git a/morphlib/gitdir.py b/morphlib/gitdir.py
index 5b0693cb..3966a0f0 100644
--- a/morphlib/gitdir.py
+++ b/morphlib/gitdir.py
@@ -282,6 +282,16 @@ class Remote(object):
         return self._get_remote_url(self.name, 'push')
 
     @staticmethod
+    def _parse_ls_remote_output(output): # pragma: no cover
+        for line in output.splitlines():
+            sha1, refname = line.split(None, 1)
+            yield sha1, refname
+
+    def ls(self): # pragma: no cover
+        out = self.gd._runcmd(['git', 'ls-remote', self.get_fetch_url()])
+        return self._parse_ls_remote_output(out)
+
+    @staticmethod
     def _parse_push_output(output):
         for line in output.splitlines():
             m = PUSH_FORMAT.match(line)
@@ -484,6 +494,10 @@ class GitDirectory(object):
         except cliapp.AppException as e:
             raise InvalidRefError(self, ref)
 
+    def disambiguate_ref(self, ref): # pragma: no cover
+        out = self._runcmd(['git', 'rev-parse', '--symbolic-full-name', ref])
+        return out.strip()
+
     def resolve_ref_to_commit(self, ref):
         return self._rev_parse('%s^{commit}' % ref)
 
diff --git a/morphlib/gitindex.py b/morphlib/gitindex.py
index 978ea0e2..6be4aacb 100644
--- a/morphlib/gitindex.py
+++ b/morphlib/gitindex.py
@@ -1,4 +1,4 @@
-# Copyright (C) 2013  Codethink Limited
+# Copyright (C) 2013-2014  Codethink Limited
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -97,7 +97,8 @@ class GitIndex(object):
 
     def get_uncommitted_changes(self):
         for code, to_path, from_path in self._get_status():
-            if code not in (STATUS_UNTRACKED, STATUS_IGNORED):
+            if (code not in (STATUS_UNTRACKED, STATUS_IGNORED)
+            or  code == (STATUS_UNTRACKED) and to_path.endswith('.morph')):
                 yield code, to_path, from_path
 
     def set_to_tree(self, treeish):
diff --git a/morphlib/plugins/build_plugin.py b/morphlib/plugins/build_plugin.py
index 1a4fb573..64630c2b 100644
--- a/morphlib/plugins/build_plugin.py
+++ b/morphlib/plugins/build_plugin.py
@@ -184,31 +184,10 @@ class BuildPlugin(cliapp.Plugin):
                             system=system_filename,
                             branch=sb.system_branch_name)
 
-        bb = morphlib.buildbranch.BuildBranch(sb, build_ref_prefix,
-                                              push_temporary=push)
-        with contextlib.closing(bb) as bb:
-
-            for gd, build_ref in bb.add_uncommitted_changes():
-                self.app.status(msg='Adding uncommitted changes '\
-                                    'in %(dirname)s to %(ref)s',
-                                dirname=gd.dirname, ref=build_ref, chatty=True)
-
-            for gd in bb.inject_build_refs(loader):
-                self.app.status(msg='Injecting temporary build refs '\
-                                    'into morphologies in %(dirname)s',
-                                dirname=gd.dirname, chatty=True)
-
-            for gd, build_ref in bb.update_build_refs(name, email, build_uuid):
-                self.app.status(msg='Committing changes in %(dirname)s '\
-                                    'to %(ref)s',
-                                dirname=gd.dirname, ref=build_ref, chatty=True)
-
-            for gd, build_ref, remote in bb.push_build_branches():
-                self.app.status(msg='Pushing %(ref)s in %(dirname)s '\
-                                    'to %(remote)s',
-                                ref=build_ref, dirname=gd.dirname,
-                                remote=remote.get_push_url(), chatty=True)
-
-            build_command.build([bb.root_repo_url,
-                                 bb.root_ref,
-                                 system_filename])
+        bb = morphlib.buildbranch.BuildBranch(sb, build_ref_prefix)
+        pbb = morphlib.buildbranch.pushed_build_branch(
+                bb, loader=loader, changes_need_pushing=push,
+                name=name, email=email, build_uuid=build_uuid,
+                status=self.app.status)
+        with pbb as (repo, ref):
+            build_command.build([repo, ref, system_filename])
diff --git a/morphlib/plugins/deploy_plugin.py b/morphlib/plugins/deploy_plugin.py
index 38c17bc2..61b8145e 100644
--- a/morphlib/plugins/deploy_plugin.py
+++ b/morphlib/plugins/deploy_plugin.py
@@ -319,40 +319,21 @@ class DeployPlugin(cliapp.Plugin):
         self.validate_deployment_options(
             env_vars, all_deployments, all_subsystems)
 
-        bb = morphlib.buildbranch.BuildBranch(sb, build_ref_prefix,
-                                              push_temporary=False)
-        with contextlib.closing(bb) as bb:
-
-            for gd, build_ref in bb.add_uncommitted_changes():
-                self.app.status(msg='Adding uncommitted changes '\
-                                    'in %(dirname)s to %(ref)s',
-                                dirname=gd.dirname, ref=build_ref, chatty=True)
-
-            for gd in bb.inject_build_refs(loader):
-                self.app.status(msg='Injecting temporary build refs '\
-                                    'into morphologies in %(dirname)s',
-                                dirname=gd.dirname, chatty=True)
-
-            for gd, build_ref in bb.update_build_refs(name, email, build_uuid):
-                self.app.status(msg='Committing changes in %(dirname)s '\
-                                    'to %(ref)s',
-                                dirname=gd.dirname, ref=build_ref, chatty=True)
-
-            for gd, build_ref, remote in bb.push_build_branches():
-                self.app.status(msg='Pushing %(ref)s in %(dirname)s '\
-                                    'to %(remote)s',
-                                ref=build_ref, dirname=gd.dirname,
-                                remote=remote.get_push_url(), chatty=True)
-
+        bb = morphlib.buildbranch.BuildBranch(sb, build_ref_prefix)
+        pbb = morphlib.buildbranch.pushed_build_branch(
+                bb, loader=loader, changes_need_pushing=False,
+                name=name, email=email, build_uuid=build_uuid,
+                status=self.app.status)
+        with pbb as (repo, ref):
             # Create a tempdir for this deployment to work in
             deploy_tempdir = tempfile.mkdtemp(
                 dir=os.path.join(self.app.settings['tempdir'], 'deployments'))
             try:
                 for system in cluster_morphology['systems']:
                     self.deploy_system(build_command, deploy_tempdir,
-                                       root_repo_dir, bb.root_repo_url,
-                                       bb.root_ref, system, env_vars,
-                                       deployments, parent_location='')
+                                       root_repo_dir, repo, ref, system,
+                                       env_vars, deployments,
+                                       parent_location='')
             finally:
                 shutil.rmtree(deploy_tempdir)
 
@@ -547,9 +528,8 @@ class DeployPlugin(cliapp.Plugin):
         system morphology (repo, ref), or with the Morph code.
         
         '''
-        build_ref_prefix = self.app.settings['build-ref-prefix']
         with morphlib.extensions.get_extension_filename(
-                build_ref_prefix, name, kind) as ext_filename:
+                name, kind) as ext_filename:
             self.app.status(msg='Running extension %(name)s%(kind)s',
                             name=name, kind=kind)
             self.app.runcmd(