Basic submodule tests are working once again !

After all, it was easier than expected. It seems that previous assertions
the test made should have never been true to begin with. Thus we might
have improved the test thanks to our improved implementation.

Fixes #233

Author: Byron

git/objects/submodule/base.py

@@ -458,23 +458,24 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress=
             # END early abort if init is not allowed
 
             # there is no git-repository yet - but delete empty paths
-            module_path = self._module_abspath(self.repo, self.path, self.name)
-            if not dry_run and os.path.isdir(module_path):
+            checkout_module_abspath = self.abspath
+            if not dry_run and os.path.isdir(checkout_module_abspath):
                 try:
-                    os.rmdir(module_path)
+                    os.rmdir(checkout_module_abspath)
                 except OSError:
-                    raise OSError("Module directory at %r does already exist and is non-empty" % module_path)
+                    raise OSError("Module directory at %r does already exist and is non-empty"
+                                  % checkout_module_abspath)
                 # END handle OSError
             # END handle directory removal
 
             # don't check it out at first - nonetheless it will create a local
             # branch according to the remote-HEAD if possible
             progress.update(BEGIN | CLONE, 0, 1, prefix + "Cloning %s to %s in submodule %r" %
-                            (self.url, module_path, self.name))
+                            (self.url, checkout_module_abspath, self.name))
             if not dry_run:
                 mrepo = self._clone_repo(self.repo, self.url, self.path, self.name, n=True)
             # END handle dry-run
-            progress.update(END | CLONE, 0, 1, prefix + "Done cloning to %s" % module_path)
+            progress.update(END | CLONE, 0, 1, prefix + "Done cloning to %s" % checkout_module_abspath)
 
             if not dry_run:
                 # see whether we have a valid branch to checkout
@@ -784,6 +785,10 @@ def remove(self, module=True, force=False, configuration=True, dry_run=False):
             # end handle separate bare repository
         # END handle module deletion
 
+        # void our data not to delay invalid access
+        if not dry_run:
+            self._clear_cache()
+
         # DELETE CONFIGURATION
         ######################
         if configuration and not dry_run:
@@ -807,8 +812,6 @@ def remove(self, module=True, force=False, configuration=True, dry_run=False):
             writer.release()
         # END delete configuration
 
-        # void our data not to delay invalid access
-        self._clear_cache()
         return self
 
     def set_parent_commit(self, commit, check=True):
@@ -840,10 +843,15 @@ def set_parent_commit(self, commit, check=True):
         # END handle checking mode
 
         # update our sha, it could have changed
-        self.binsha = pctree[self.path].binsha
+        # If check is False, we might see a parent-commit that doens't even contain the submodule anymore.
+        # in that case, mark our sha as being NULL
+        try:
+            self.binsha = pctree[self.path].binsha
+        except KeyError:
+            self.binsha = self.NULL_BIN_SHA
+        # end
 
         self._clear_cache()
-
         return self
 
     @unbare_repo

git/test/test_submodule.py

@@ -1,6 +1,5 @@
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
-import shutil
 import sys
 import os
 
@@ -18,8 +17,6 @@
 from git.compat import string_types
 from git.repo.fun import find_git_dir
 
-from nose import SkipTest
-
 # Change the configuration if possible to prevent the underlying memory manager
 # to keep file handles open. On windows we get problems as they are not properly
 # closed due to mmap bugs on windows (as it appears)
@@ -46,7 +43,7 @@ def update(self, op, index, max_count, message=''):
 
 class TestSubmodule(TestBase):
 
-    k_subm_current = "468cad66ff1f80ddaeee4123c24e4d53a032c00d"
+    k_subm_current = "c15a6e1923a14bc760851913858a3942a4193cdb"
     k_subm_changed = "394ed7006ee5dc8bddfd132b64001d5dfc0ffdd3"
     k_no_subm_tag = "0.1.6"
 
@@ -67,7 +64,7 @@ def _do_base_tests(self, rwrepo):
 
         assert sm.path == 'git/ext/gitdb'
         assert sm.path != sm.name                   # in our case, we have ids there, which don't equal the path
-        assert sm.url == 'git://github.com/gitpython-developers/gitdb.git'
+        assert sm.url.endswith('github.com/gitpython-developers/gitdb.git')
         assert sm.branch_path == 'refs/heads/master'            # the default ...
         assert sm.branch_name == 'master'
         assert sm.parent_commit == rwrepo.head.commit
@@ -184,7 +181,9 @@ def _do_base_tests(self, rwrepo):
             assert sm.module().head.ref.tracking_branch() is not None
 
             # delete the whole directory and re-initialize
-            shutil.rmtree(sm.abspath)
+            assert len(sm.children()) != 0
+            # shutil.rmtree(sm.abspath)
+            sm.remove(force=True, configuration=False)
             assert len(sm.children()) == 0
             # dry-run does nothing
             sm.update(dry_run=True, recursive=False, progress=prog)
@@ -277,9 +276,12 @@ def _do_base_tests(self, rwrepo):
 
             # enforce the submodule to be checked out at the right spot as well.
             csm.update()
+            assert csm.module_exists()
+            assert csm.exists()
+            assert os.path.isdir(csm.module().working_tree_dir)
 
             # this would work
-            assert sm.remove(dry_run=True) is sm
+            assert sm.remove(force=True, dry_run=True) is sm
             assert sm.module_exists()
             sm.remove(force=True, dry_run=True)
             assert sm.module_exists()
@@ -291,17 +293,20 @@ def _do_base_tests(self, rwrepo):
 
             # forcibly delete the child repository
             prev_count = len(sm.children())
-            assert csm.remove(force=True) is csm
+            self.failUnlessRaises(ValueError, csm.remove, force=True)
+            # We removed sm, which removed all submodules. Howver, the instance we have
+            # still points to the commit prior to that, where it still existed
+            csm.set_parent_commit(csm.repo.commit(), check=False)
             assert not csm.exists()
             assert not csm.module_exists()
-            assert len(sm.children()) == prev_count - 1
+            assert len(sm.children()) == prev_count
             # now we have a changed index, as configuration was altered.
             # fix this
             sm.module().index.reset(working_tree=True)
 
             # now delete only the module of the main submodule
             assert sm.module_exists()
-            sm.remove(configuration=False)
+            sm.remove(configuration=False, force=True)
             assert sm.exists()
             assert not sm.module_exists()
             assert sm.config_reader().get_value('url')
@@ -391,7 +396,6 @@ def _do_base_tests(self, rwrepo):
 
     @with_rw_repo(k_subm_current)
     def test_base_rw(self, rwrepo):
-        raise SkipTest("Disabled as long as it fails and submodule support wasn't overhauled")
         self._do_base_tests(rwrepo)
 
     @with_rw_repo(k_subm_current, bare=True)