Submodule.remove() now seems to work properly, nearly all tests are back.

This also means that now we seem to be able to properly handle .git files in submodules

Related to #233

3 files changed, 35 insertions, 24 deletions

diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py
index 15ba8a0d..26bdd54e 100644
--- a/git/objects/submodule/base.py
+++ b/git/objects/submodule/base.py
@@ -572,14 +572,14 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         the repository at our current path, changing the configuration, as well as
         adjusting our index entry accordingly.
 
-        :param module_path: the path to which to move our module in the parent repostory's working tree, 
+        :param module_path: the path to which to move our module in the parent repostory's working tree,
             given as repository-relative or absolute path. Intermediate directories will be created
             accordingly. If the path already exists, it must be empty.
             Trailing (back)slashes are removed automatically
         :param configuration: if True, the configuration will be adjusted to let
             the submodule point to the given path.
         :param module: if True, the repository managed by this submodule
-            will be moved as well. If False, we don't move the submodule's checkout, which may leave 
+            will be moved as well. If False, we don't move the submodule's checkout, which may leave
             the parent repository in an inconsistent state.
         :return: self
         :raise ValueError: if the module path existed and was not empty, or was a file
@@ -672,7 +672,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         return self
 
     @unbare_repo
-    def remove(self, module=True, force=False, configuration=True, dry_run=False, _is_recursive=False):
+    def remove(self, module=True, force=False, configuration=True, dry_run=False):
         """Remove this submodule from the repository. This will remove our entry
         from the .gitmodules file and the entry in the .git/config file.
 
@@ -695,7 +695,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             we would usually throw
         :return: self
         :note: doesn't work in bare repositories
-        :note: doesn't work atomically, as failure to remove any part of the submodule will leave 
+        :note: doesn't work atomically, as failure to remove any part of the submodule will leave
             an inconsistent state
         :raise InvalidGitRepositoryError: thrown if the repository cannot be deleted
         :raise OSError: if directories or files could not be removed"""
@@ -704,10 +704,17 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         # END handle parameters
 
         # Recursively remove children of this submodule
+        nc = 0
         for csm in self.children():
-            csm.remove(module, force, configuration, dry_run, _is_recursive=True)
+            nc += 1
+            csm.remove(module, force, configuration, dry_run)
             del(csm)
-        # end 
+        # end
+        if nc > 0:
+            # Assure we don't leave the parent repository in a dirty state, and commit our changes
+            # It's important for recursive, unforced, deletions to work as expected
+            self.module().index.commit("Removed submodule '%s'" % self.name)
+        # end handle recursion
 
         # DELETE REPOSITORY WORKING TREE
         ################################
@@ -733,7 +740,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
                 # END apply deletion method
             else:
                 # verify we may delete our module
-                if mod.is_dirty(untracked_files=True):
+                if mod.is_dirty(index=True, working_tree=True, untracked_files=True):
                     raise InvalidGitRepositoryError(
                         "Cannot delete module at %s with any modifications, unless force is specified"
                         % mod.working_tree_dir)
@@ -743,7 +750,7 @@ class Submodule(util.IndexObject, Iterable, Traversable):
                 # NOTE: If the user pulled all the time, the remote heads might
                 # not have been updated, so commits coming from the remote look
                 # as if they come from us. But we stay strictly read-only and
-                # don't fetch beforhand.
+                # don't fetch beforehand.
                 for remote in mod.remotes:
                     num_branches_with_new_commits = 0
                     rrefs = remote.refs
@@ -794,15 +801,10 @@ class Submodule(util.IndexObject, Iterable, Traversable):
             writer = self.repo.config_writer()
             writer.remove_section(sm_section(self.name))
             writer.release()
+
             writer = self.config_writer()
             writer.remove_section()
             writer.release()
-
-            # Assure we don't leave the parent repository in a dirty state, and commit our changes
-            # It's important for recursive, unforced, deletions to work as expected
-            if _is_recursive:
-                self.module().index.commit("Removed submodule '%s'" % self.name)
-            # end
         # END delete configuration
 
         # void our data not to delay invalid access
@@ -875,16 +877,16 @@ class Submodule(util.IndexObject, Iterable, Traversable):
         :raise InvalidGitRepositoryError: if a repository was not available. This could
             also mean that it was not yet initialized"""
         # late import to workaround circular dependencies
-        module_path = self.abspath
+        module_checkout_abspath = self.abspath
         try:
-            repo = git.Repo(module_path)
+            repo = git.Repo(module_checkout_abspath)
             if repo != self.repo:
                 return repo
             # END handle repo uninitialized
         except (InvalidGitRepositoryError, NoSuchPathError):
-            raise InvalidGitRepositoryError("No valid repository at %s" % self.path)
+            raise InvalidGitRepositoryError("No valid repository at %s" % module_checkout_abspath)
         else:
-            raise InvalidGitRepositoryError("Repository at %r was not yet checked out" % module_path)
+            raise InvalidGitRepositoryError("Repository at %r was not yet checked out" % module_checkout_abspath)
         # END handle exceptions
 
     def module_exists(self):
diff --git a/git/repo/base.py b/git/repo/base.py
index 74e72aa5..f3dd05b3 100644
--- a/git/repo/base.py
+++ b/git/repo/base.py
@@ -132,8 +132,8 @@ class Repo(object):
         # walk up the path to find the .git dir
         while curpath:
             # ABOUT os.path.NORMPATH
-            # It's important to normalize the paths, as submodules will otherwise initialize their 
-            # repo instances with paths that depend on path-portions that will not exist after being 
+            # It's important to normalize the paths, as submodules will otherwise initialize their
+            # repo instances with paths that depend on path-portions that will not exist after being
             # removed. It's just cleaner.
             if is_git_dir(curpath):
                 self.git_dir = os.path.normpath(curpath)
diff --git a/git/test/test_submodule.py b/git/test/test_submodule.py
index ace7ab07..dc6c8a1a 100644
--- a/git/test/test_submodule.py
+++ b/git/test/test_submodule.py
@@ -508,7 +508,11 @@ class TestSubmodule(TestBase):
         rm.update(recursive=False, dry_run=True)
         assert os.path.isdir(smp)
 
-        rm.update(recursive=False)
+        # when removing submodules, we may get new commits as nested submodules are auto-committing changes
+        # to allow deletions without force, as the index would be dirty otherwise.
+        # QUESTION: Why does this seem to work in test_git_submodule_compatibility() ?
+        self.failUnlessRaises(InvalidGitRepositoryError, rm.update, recursive=False, force_remove=False)
+        rm.update(recursive=False, force_remove=True)
         assert not os.path.isdir(smp)
 
         # change url
@@ -664,14 +668,19 @@ class TestSubmodule(TestBase):
         assert sm.exists()
         assert sm.module_exists()
 
+        # Add additional submodule level
+        sm.module().create_submodule('nested-submodule', 'nested-submodule/working-tree',
+                                     url=self._submodule_url())
+        sm.module().index.commit("added nested submodule")
+        # Fails because there are new commits, compared to the remote we cloned from
+        self.failUnlessRaises(InvalidGitRepositoryError, sm.remove, dry_run=True)
+
         # remove
         sm_module_path = sm.module().git_dir
 
         for dry_run in (True, False):
-            sm.remove(dry_run=dry_run)
+            sm.remove(dry_run=dry_run, force=True)
             assert sm.exists() == dry_run
             assert sm.module_exists() == dry_run
             assert os.path.isdir(sm_module_path) == dry_run
         # end for each dry-run mode
-
-