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

2 files changed, 33 insertions, 21 deletions

diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index 26bdd54e..3e3eed78 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -458,23 +458,24 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             # 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 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             # 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 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             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 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         # 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
diff --git a/git/test/test_submodule.py b/git/test/test_submodule.py
index dc6c8a1a..7fe882e1 100644
--- a/git/test/test_submodule.py
+++ b/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.util import to_native_path_linux, join_path_native
 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 @@ prog = TestRootProgress()
 
 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 @@ class TestSubmodule(TestBase):
 
         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 @@ class TestSubmodule(TestBase):
             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 @@ class TestSubmodule(TestBase):
 
             # 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 @@ class TestSubmodule(TestBase):
 
             # 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 @@ class TestSubmodule(TestBase):
 
     @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)