Merge pull request #195 from johnpaulalex/check_repo

test_sys_checkout: check that desired repo was actually checked out.

Also change GitRepository._determine_remote_name() into a classmethod _remote_name_for_url() that takes in a url, to make it more self-evident how the remote name is being picked.

User interface changes?: No

Fixes: None

Testing:
test removed: none
unit tests: none
system tests: 'make stest' passes
manual testing: none

manic/repository_git.py

@@ -194,7 +194,7 @@ def compare_refs(current_ref, expected_ref):
             if self._url == LOCAL_PATH_INDICATOR:
                 expected_ref = self._branch
             else:
-                remote_name = self._determine_remote_name()
+                remote_name = self._remote_name_for_url(self._url)
                 if not remote_name:
                     # git doesn't know about this remote. by definition
                     # this is a modified state.
@@ -232,27 +232,23 @@ def compare_refs(current_ref, expected_ref):
 
         os.chdir(cwd)
 
-    def _determine_remote_name(self):
-        """Return the remote name.
-
-        Note that this is for the *future* repo url and branch, not
-        the current working copy!
+    @classmethod
+    def _remote_name_for_url(cls, remote_url, dir=None):
+        """Return the remote name matching remote_url (or None)
 
         """
-        git_output = self._git_remote_verbose()
+        git_output = cls._git_remote_verbose(dir)
         git_output = git_output.splitlines()
-        remote_name = ''
         for line in git_output:
             data = line.strip()
             if not data:
                 continue
             data = data.split()
             name = data[0].strip()
             url = data[1].strip()
-            if self._url == url:
-                remote_name = name
-                break
-        return remote_name
+            if remote_url == url:
+                return name
+        return None
 
     def _create_remote_name(self):
         """The url specified in the externals description file was not known
@@ -353,7 +349,7 @@ def _checkout_external_ref(self, verbosity, submodules):
         else:
             ref = self._hash
 
-        remote_name = self._determine_remote_name()
+        remote_name = self._remote_name_for_url(self._url)
         if not remote_name:
             remote_name = self._create_remote_name()
             self._git_remote_add(remote_name, self._url)
@@ -612,7 +608,7 @@ def _status_v1z_is_dirty(git_output):
     def _git_current_hash(dir=None):
         """Return the full hash of the currently checked-out version.
 
-        if dir is None, uses the cwd.
+        If dir is None, uses the cwd.
 
         Returns a tuple, (hash_found, hash), where hash_found is a
         logical specifying whether a hash was found for HEAD (False
@@ -787,11 +783,21 @@ def _git_status_verbose():
         return git_output
 
     @staticmethod
-    def _git_remote_verbose():
+    def _git_remote_verbose(dir=None):
         """Run the git remote command to obtain repository information.
+
+        If dir is None, uses the cwd.
+        Returned string is of the form:
+        myfork  git@github.com:johnpaulalex/manage_externals_jp.git (fetch)
+        myfork  git@github.com:johnpaulalex/manage_externals_jp.git (push)
         """
+        if dir:
+            cwd = os.getcwd()
+            os.chdir(dir)
         cmd = ['git', 'remote', '--verbose']
         git_output = execute_subprocess(cmd, output_to_caller=True)
+        if dir:
+            os.chdir(cwd)
         return git_output
 
     @staticmethod

test/test_sys_checkout.py

@@ -269,6 +269,12 @@ def create_metadata(self):
         self._config.set(DESCRIPTION_SECTION, VERSION_ITEM,
                          self._schema_version)
 
+    def url_for_repo_path(self, repo_path, repo_path_abs=None):
+        if repo_path_abs is not None:
+            return repo_path_abs
+        else:
+            return os.path.join(self._bare_root, repo_path)
+
     def create_section(self, repo_path, name, tag='', branch='',
                        ref_hash='', required=True, path=EXTERNALS_PATH,
                        sub_externals='', repo_path_abs=None, from_submodule=False,
@@ -304,10 +310,7 @@ def create_section(self, repo_path, name, tag='', branch='',
             ref_hash = ''
             branch = ''
 
-        if repo_path_abs is not None:
-            repo_url = repo_path_abs
-        else:
-            repo_url = os.path.join(self._bare_root, repo_path)
+        repo_url = self.url_for_repo_path(repo_path, repo_path_abs)
 
         if not from_submodule:
             self._config.set(name, ExternalsDescription.REPO_URL, repo_url)
@@ -639,22 +642,29 @@ def test_required_bytag(self):
         # externals start out 'empty' aka not checked out.
         tree = self.execute_checkout_in_dir(cloned_repo_dir,
                                             self.status_args)
-        local_path = self._external_path(TAG_SECTION)
-        self._check_sync_clean(tree[local_path],
+        local_path_rel = self._external_path(TAG_SECTION)
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.EMPTY,
                                ExternalStatus.DEFAULT)
-        tag_full_path = os.path.join(cloned_repo_dir, local_path)
-        self.assertFalse(os.path.exists(tag_full_path))
+        local_path_abs = os.path.join(cloned_repo_dir, local_path_rel)
+        self.assertFalse(os.path.exists(local_path_abs))
 
         # after checkout, the external is 'clean' aka at the correct version.
         tree = self.execute_checkout_with_status(cloned_repo_dir,
                                                  self.checkout_args)
-        self._check_sync_clean(tree[local_path],
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.STATUS_OK,
                                ExternalStatus.STATUS_OK)
 
+        # Actually checked out the desired repo.
+        self.assertEqual('origin', GitRepository._remote_name_for_url(
+            # Which url to look up
+            self._generator.url_for_repo_path(SIMPLE_REPO),
+            # Which directory has the local checked-out repo.
+            dir=local_path_abs))
+
         # Actually checked out the desired tag.
-        (tag_found, tag_name) = GitRepository._git_current_tag(tag_full_path)
+        (tag_found, tag_name) = GitRepository._git_current_tag(local_path_abs)
         self.assertEqual(tag_name, 'tag1')
 
         # Check existence of some simp_tag files
@@ -677,22 +687,31 @@ def test_required_bybranch(self):
         # externals start out 'empty' aka not checked out.
         tree = self.execute_checkout_in_dir(cloned_repo_dir,
                                             self.status_args)
-        local_path = self._external_path(BRANCH_SECTION)
-        self._check_sync_clean(tree[local_path],
+        local_path_rel = self._external_path(BRANCH_SECTION)
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.EMPTY,
                                ExternalStatus.DEFAULT)
-        branch_path = os.path.join(cloned_repo_dir, local_path)
-        self.assertFalse(os.path.exists(branch_path))
+        local_path_abs = os.path.join(cloned_repo_dir, local_path_rel)
+        self.assertFalse(os.path.exists(local_path_abs))
 
         # after checkout, the external is 'clean' aka at the correct version.
         tree = self.execute_checkout_with_status(cloned_repo_dir,
                                                  self.checkout_args)
-        self._check_sync_clean(tree[local_path],
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.STATUS_OK,
                                ExternalStatus.STATUS_OK)
-        self.assertTrue(os.path.exists(branch_path))
+        self.assertTrue(os.path.exists(local_path_abs))
+
+        # Actually checked out the desired repo.
+        self.assertEqual('origin', GitRepository._remote_name_for_url(
+            # Which url to look up
+            self._generator.url_for_repo_path(SIMPLE_REPO),
+            # Which directory has the local checked-out repo.
+            dir=local_path_abs))
+
+        # Actually checked out the desired branch. 
         (branch_found, branch_name) = GitRepository._git_current_remote_branch(
-            branch_path)
+            local_path_abs)
         self.assertEquals(branch_name, 'origin/' + REMOTE_BRANCH_FEATURE2)
 
     def test_required_byhash(self):
@@ -706,20 +725,30 @@ def test_required_byhash(self):
         # externals start out 'empty' aka not checked out.
         tree = self.execute_checkout_in_dir(cloned_repo_dir,
                                             self.status_args)
-        local_path = self._external_path(HASH_SECTION)
-        self._check_sync_clean(tree[local_path],
+        local_path_rel = self._external_path(HASH_SECTION)
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.EMPTY,
                                ExternalStatus.DEFAULT)
-        hash_path = os.path.join(cloned_repo_dir, local_path)
-        self.assertFalse(os.path.exists(hash_path))
+        local_path_abs = os.path.join(cloned_repo_dir, local_path_rel)
+        self.assertFalse(os.path.exists(local_path_abs))
 
         # after checkout, the externals are 'clean' aka at their correct version.
         tree = self.execute_checkout_with_status(cloned_repo_dir,
                                                  self.checkout_args)
-        self._check_sync_clean(tree[local_path],
+        self._check_sync_clean(tree[local_path_rel],
                                ExternalStatus.STATUS_OK,
                                ExternalStatus.STATUS_OK)
-        (hash_found, hash_name) = GitRepository._git_current_hash(hash_path)
+
+        # Actually checked out the desired repo.
+        self.assertEqual('origin', GitRepository._remote_name_for_url(
+            # Which url to look up
+            self._generator.url_for_repo_path(SIMPLE_REPO),
+            # Which directory has the local checked-out repo.
+            dir=local_path_abs))
+
+        # Actually checked out the desired hash.
+        (hash_found, hash_name) = GitRepository._git_current_hash(
+            local_path_abs)
         self.assertTrue(hash_name.startswith('60b1cc1a38d63'),
                         msg=hash_name)
 

test/test_unit_repository_git.py

@@ -206,11 +206,19 @@ def setUp(self):
         self._repo._current_ref = self._current_ref_empty
         self._create_tmp_git_dir()
 
+        # We have to override this class method rather than the self._repo
+        # instance method because it is called via
+        # GitRepository._remote_name_for_url, which is itself a @classmethod
+        # calls cls._git_remote_verbose().
+        self._orignal_git_remote_verbose = GitRepository._git_remote_verbose
+        GitRepository._git_remote_verbose = self._git_remote_origin_upstream        
     def tearDown(self):
         """Cleanup tmp stuff on the file system
         """
         self._remove_tmp_git_dir()
 
+        GitRepository._git_remote_verbose = self._orignal_git_remote_verbose
+
     def _create_tmp_git_dir(self):
         """Create a temporary fake git directory for testing purposes.
         """
@@ -229,20 +237,18 @@ def _remove_tmp_git_dir(self):
     @staticmethod
     def _current_ref_empty():
         """Return an empty string.
+
+        Drop-in for GitRepository._current_ref
         """
         return EMPTY_STR
 
     @staticmethod
-    def _git_remote_origin_upstream():
-        """Return an info string that is a checkout hash
-        """
-        return GIT_REMOTE_OUTPUT_ORIGIN_UPSTREAM
+    def _git_remote_origin_upstream(dir=None):
+        """Return an info string that is a checkout hash.
 
-    @staticmethod
-    def _git_remote_none():
-        """Return an info string that is a checkout hash
+        Drop-in for GitRepository._git_remote_verbose.
         """
-        return EMPTY_STR
+        return GIT_REMOTE_OUTPUT_ORIGIN_UPSTREAM
 
     @staticmethod
     def _git_current_hash(myhash):
@@ -291,9 +297,6 @@ def test_sync_dir_exist_no_git_info(self):
         """Test that a non-existent git repo returns an unknown status
         """
         stat = ExternalStatus()
-        # Now we over-ride the _git_remote_verbose method on the repo to return
-        # a known value without requiring access to git.
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = 'tag1'
         self._repo._git_current_hash = self._git_current_hash('')
         self._repo._git_revparse_commit = self._git_revparse_commit(
@@ -313,7 +316,6 @@ def test_sync_invalid_reference(self):
         """Test that an invalid reference returns out-of-sync
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = 'tag1'
         self._repo._git_current_hash = self._git_current_hash('abc123')
         self._repo._git_revparse_commit = self._git_revparse_commit(
@@ -333,7 +335,6 @@ def test_sync_tag_on_same_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = 'tag1'
         self._repo._git_current_hash = self._git_current_hash('abc123')
         self._repo._git_revparse_commit = self._git_revparse_commit(
@@ -348,7 +349,6 @@ def test_sync_tag_on_different_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = 'tag1'
         self._repo._git_current_hash = self._git_current_hash('def456')
         self._repo._git_revparse_commit = self._git_revparse_commit(
@@ -368,7 +368,6 @@ def test_sync_hash_on_same_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = ''
         self._repo._hash = 'abc'
         self._repo._git_current_hash = self._git_current_hash('abc123')
@@ -384,7 +383,6 @@ def test_sync_hash_on_different_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._tag = ''
         self._repo._hash = 'abc'
         self._repo._git_current_hash = self._git_current_hash('def456')
@@ -405,7 +403,6 @@ def test_sync_branch_on_same_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature-2'
         self._repo._tag = ''
         self._repo._git_current_hash = self._git_current_hash('abc123')
@@ -421,7 +418,6 @@ def test_sync_branch_on_diff_hash(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature-2'
         self._repo._tag = ''
         self._repo._git_current_hash = self._git_current_hash('abc123')
@@ -433,11 +429,10 @@ def test_sync_branch_on_diff_hash(self):
         self.assertEqual(stat.clean_state, ExternalStatus.DEFAULT)
 
     def test_sync_branch_diff_remote(self):
-        """Test _determine_remote_name with a different remote
+        """Test _remote_name_for_url with a different remote
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature-2'
         self._repo._tag = ''
         self._repo._url = '/path/to/other/repo'
@@ -449,11 +444,10 @@ def test_sync_branch_diff_remote(self):
         # expected argument
 
     def test_sync_branch_diff_remote2(self):
-        """Test _determine_remote_name with a different remote
+        """Test _remote_name_for_url with a different remote
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature-2'
         self._repo._tag = ''
         self._repo._url = '/path/to/local/repo2'
@@ -469,7 +463,6 @@ def test_sync_branch_on_unknown_remote(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature-2'
         self._repo._tag = ''
         self._repo._url = '/path/to/unknown/repo'
@@ -491,7 +484,6 @@ def test_sync_branch_on_untracked_local(self):
 
         """
         stat = ExternalStatus()
-        self._repo._git_remote_verbose = self._git_remote_origin_upstream
         self._repo._branch = 'feature3'
         self._repo._tag = ''
         self._repo._url = '.'