Merge remote-tracking branch 'remotes/origin/samthursfield/ambiguous-refs' into baserock/merge-queue

7 files changed, 79 insertions, 56 deletions

diff --git a/morphlib/cachedrepo.py b/morphlib/cachedrepo.py
index 0b5ce60f..0a085bb2 100644
--- a/morphlib/cachedrepo.py
+++ b/morphlib/cachedrepo.py
@@ -104,6 +104,15 @@ class CachedRepo(object):
         self.path = path
         self.is_mirror = not url.startswith('file://')
 
+    def ref_exists(self, ref):
+        '''Returns True if the given ref exists in the repo'''
+
+        try:
+            self._rev_parse(ref)
+        except cliapp.AppException:
+            return False
+        return True
+
     def resolve_ref(self, ref):
         '''Attempts to resolve a ref into its SHA1 and tree SHA1.
 
@@ -112,18 +121,10 @@ class CachedRepo(object):
 
         '''
 
-        if not morphlib.git.is_valid_sha1(ref):
-            try:
-                refs = self._show_ref(ref).split('\n')
-                refs = [x.split() for x in refs]
-                absref = refs[0][0]
-            except cliapp.AppException:
-                raise InvalidReferenceError(self, ref)
-        else:
-            try:
-                absref = self._rev_list(ref).strip()
-            except cliapp.AppException:
-                raise InvalidReferenceError(self, ref)
+        try:
+            absref = self._rev_parse(ref)
+        except cliapp.AppException:
+            raise InvalidReferenceError(self, ref)
 
         tree = self._show_tree_hash(absref)
         return absref, tree
@@ -141,7 +142,7 @@ class CachedRepo(object):
         if not morphlib.git.is_valid_sha1(ref):
             raise UnresolvedNamedReferenceError(self, ref)
         try:
-            sha1 = self._rev_list(ref).strip()
+            sha1 = self._rev_parse(ref)
         except cliapp.AppException:
             raise InvalidReferenceError(self, ref)
 
@@ -193,7 +194,7 @@ class CachedRepo(object):
         '''Loads a morphology from a given ref'''
 
         if not morphlib.git.is_valid_sha1(ref):
-            ref = self._rev_list(ref).strip()
+            ref = self._rev_parse(ref)
         text = self.cat(ref, '%s.morph' % name)
         morphology = morphlib.morph2.Morphology(text)
         return morphology
@@ -210,7 +211,7 @@ class CachedRepo(object):
         if not morphlib.git.is_valid_sha1(ref):
             raise UnresolvedNamedReferenceError(self, ref)
         try:
-            sha1 = self._rev_list(ref).strip()
+            sha1 = self._rev_parse(ref)
         except cliapp.AppException:
             raise InvalidReferenceError(self, ref)
 
@@ -237,16 +238,13 @@ class CachedRepo(object):
             kwargs['cwd'] = self.path
         return self.app.runcmd(*args, **kwargs)
 
-    def _show_ref(self, ref):  # pragma: no cover
-        return self._runcmd(['git', 'show-ref', ref])
+    def _rev_parse(self, ref):  # pragma: no cover
+        return self._runcmd(['git', 'rev-parse', '--verify', ref])[0:40]
 
     def _show_tree_hash(self, absref):  # pragma: no cover
         return self._runcmd(
                 ['git', 'log', '-1', '--format=format:%T', absref]).strip()
 
-    def _rev_list(self, ref):  # pragma: no cover
-        return self._runcmd(['git', 'rev-list', '--no-walk', ref])
-
     def _ls_tree(self, ref):  # pragma: no cover
         result = self._runcmd(['git', 'ls-tree', '--name-only', ref])
         return result.split('\n')
diff --git a/morphlib/cachedrepo_tests.py b/morphlib/cachedrepo_tests.py
index 81673880..9251a473 100644
--- a/morphlib/cachedrepo_tests.py
+++ b/morphlib/cachedrepo_tests.py
@@ -31,19 +31,19 @@ class CachedRepoTests(unittest.TestCase):
         "kind": "chunk"
     }'''
 
-    def show_ref(self, ref):
+    def rev_parse(self, ref):
         output = {
-            'master':
-            'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9'
-            ' refs/heads/master',
-            'baserock/morph':
-            '8b780e2e6f102fcf400ff973396566d36d730501'
-            ' refs/heads/baserock/morph',
+            'a4da32f5a81c8bc6d660404724cedc3bc0914a75':
+                    'a4da32f5a81c8bc6d660404724cedc3bc0914a75',
+            'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9':
+                    'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9',
+            'master': 'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9',
+            'baserock/morph': '8b780e2e6f102fcf400ff973396566d36d730501'
         }
         try:
             return output[ref]
         except:
-            raise cliapp.AppException('git show-ref %s' % ref)
+            raise cliapp.AppException('git rev-parse --verify %s' % ref)
 
     def show_tree_hash(self, absref):
         output = {
@@ -60,19 +60,6 @@ class CachedRepoTests(unittest.TestCase):
             raise cliapp.AppException('git log -1 --format=format:%%T %s' %
                                       absref)
 
-    def rev_list(self, ref):
-        output = {
-            'master': 'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9',
-            'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9':
-            'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9',
-            'a4da32f5a81c8bc6d660404724cedc3bc0914a75':
-            'a4da32f5a81c8bc6d660404724cedc3bc0914a75',
-        }
-        try:
-            return output[ref]
-        except:
-            raise cliapp.AppException('git rev-list %s' % ref)
-
     def cat_file(self, ref, filename):
         output = {
             'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9:foo.morph':
@@ -125,9 +112,8 @@ class CachedRepoTests(unittest.TestCase):
         self.repo_path = '/tmp/foo'
         self.repo = cachedrepo.CachedRepo(
             object(), self.repo_name, self.repo_url, self.repo_path)
-        self.repo._show_ref = self.show_ref
+        self.repo._rev_parse = self.rev_parse
         self.repo._show_tree_hash = self.show_tree_hash
-        self.repo._rev_list = self.rev_list
         self.repo._cat_file = self.cat_file
         self.repo._copy_repository = self.copy_repository
         self.repo._checkout_ref = self.checkout_ref
@@ -143,6 +129,12 @@ class CachedRepoTests(unittest.TestCase):
         self.assertEqual(self.repo.url, self.repo_url)
         self.assertEqual(self.repo.path, self.repo_path)
 
+    def test_ref_exists(self):
+        self.assertEqual(self.repo.ref_exists('master'), True)
+
+    def test_ref_does_not_exist(self):
+        self.assertEqual(self.repo.ref_exists('non-existant-ref'), False)
+
     def test_resolve_named_ref_master(self):
         sha1, tree = self.repo.resolve_ref('master')
         self.assertEqual(sha1, 'e28a23812eadf2fce6583b8819b9c5dbd36b9fb9')
diff --git a/morphlib/git.py b/morphlib/git.py
index a37b6675..9ac9e292 100644
--- a/morphlib/git.py
+++ b/morphlib/git.py
@@ -269,4 +269,4 @@ def is_valid_sha1(ref):
 
 def rev_parse(runcmd, gitdir, ref):
     '''Find the sha1 for the given ref'''
-    return runcmd(['git', 'rev-parse', ref], cwd=gitdir)[0:40]
+    return runcmd(['git', 'rev-parse', '--verify', ref], cwd=gitdir)[0:40]
diff --git a/morphlib/plugins/branch_and_merge_plugin.py b/morphlib/plugins/branch_and_merge_plugin.py
index a65b28ed..0c6209da 100644
--- a/morphlib/plugins/branch_and_merge_plugin.py
+++ b/morphlib/plugins/branch_and_merge_plugin.py
@@ -195,8 +195,8 @@ class BranchAndMergePlugin(cliapp.Plugin):
 
     def resolve_ref(self, repodir, ref):
         try:
-            return self.app.runcmd(['git', 'show-ref', ref],
-                                   cwd=repodir).split()[0]
+            return self.app.runcmd(['git', 'rev-parse', '--verify', ref],
+                                   cwd=repodir)[0:40]
         except:
             return None
 
@@ -517,11 +517,15 @@ class BranchAndMergePlugin(cliapp.Plugin):
         new_branch = args[1]
         commit = 'master' if len(args) == 2 else args[2]
 
+        self.lrc, self.rrc = morphlib.util.new_repo_caches(self.app)
+        if self.lrc.get_repo(repo).ref_exists(new_branch):
+            raise cliapp.AppException('branch %s already exists in '
+                                      'repository %s' % (new_branch, repo))
+
         # Create the system branch directory.
         workspace = self.deduce_workspace()
         branch_dir = os.path.join(workspace, new_branch)
         os.makedirs(branch_dir)
-        self.lrc, self.rrc = morphlib.util.new_repo_caches(self.app)
 
         try:
             # Create a .morph-system-branch directory to clearly identify
@@ -541,11 +545,6 @@ class BranchAndMergePlugin(cliapp.Plugin):
             repo_dir = os.path.join(branch_dir, self.convert_uri_to_path(repo))
             self.clone_to_directory(repo_dir, repo, commit)
 
-            # Check if branch already exists locally or in a remote
-            if self.resolve_ref(repo_dir, new_branch) is not None:
-                raise cliapp.AppException('branch %s already exists in '
-                                          'repository %s' % (new_branch, repo))
-
             # Create a new branch in the local morphs repository.
             self.app.runcmd(['git', 'checkout', '-b', new_branch, commit],
                             cwd=repo_dir)
diff --git a/tests.branching/ambiguous-refs.script b/tests.branching/ambiguous-refs.script
index a93c4b66..0ea8a55f 100755
--- a/tests.branching/ambiguous-refs.script
+++ b/tests.branching/ambiguous-refs.script
@@ -16,8 +16,10 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 
-## 'git show-ref master' sorts its output alphabetically, so we can't rely on
-## it to resolve 'master': we might get 'alpha/master' instead.
+## Guard against a bug that occurs if 'git show-ref' is used to resolve refs
+## instead of 'git rev-parse --verify': show-ref returns a list of partial
+## matches sorted alphabetically, so any code using it may resolve refs
+## incorrectly.
 
 set -eu
 
diff --git a/tests/ambiguous-refs.script b/tests/ambiguous-refs.script
new file mode 100755
index 00000000..58d09d26
--- /dev/null
+++ b/tests/ambiguous-refs.script
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Copyright (C) 2011, 2012  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
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+
+## Guard against a bug that occurs if 'git show-ref' is used to resolve refs
+## instead of 'git rev-parse --verify': show-ref returns a list of partial
+## matches sorted alphabetically, so any code using it may resolve refs
+
+set -eu
+
+# Create a ref that will show up in 'git show-ref' before the real master ref
+cd "$DATADIR/morphs-repo"
+git checkout -q -b alpha/master
+git rm -q hello-stratum.morph
+git commit -q -m "This ref will not build correctly"
+
+"$SRCDIR/scripts/test-morph" build-morphology \
+    test:morphs-repo master hello-stratum
diff --git a/tests/update-gits-chunk.script b/tests/update-gits-chunk.script
index c2ad08d0..288438ec 100755
--- a/tests/update-gits-chunk.script
+++ b/tests/update-gits-chunk.script
@@ -31,7 +31,7 @@ REPO_ALIAS='test=git://localhost:9419/%s#git://localhost:9419/%s'
 cd "$DATADIR/chunk-repo"
 git checkout --quiet farrokh
 git commit --quiet --allow-empty --allow-empty-message -m ""
-NEWREF="$(git show-ref --hash farrokh)"
+NEWREF="$(git rev-parse --verify farrokh)"
 
 "$SRCDIR/scripts/test-morph" --repo-alias=$REPO_ALIAS update-gits \
     test:chunk-repo farrokh hello
@@ -40,4 +40,4 @@ kill $(cat "$DATADIR/git.pid")
 
 # Check the top commit of the cached repo's farrokh branch
 cd "$DATADIR/cache/gits/"*chunk?repo*
-test "$(git show-ref --hash refs/heads/farrokh)" = "$NEWREF"
+test "$(git rev-parse --verify refs/heads/farrokh)" = "$NEWREF"