Greatly improved possible safety of Submodule.update(), which is used by default.

Previously, the implementation would gladly reset new commits in submodules,
and/or reset a dirty working tree.

Now the new force_reset/force flag has to be specified explicitly to get back
to the old behaviour.

All submodule tests except for one are working.

Author: Byron

git/config.py

@@ -546,10 +546,12 @@ def set_value(self, section, option, value):
         :param option: Name of the options whose value to set
 
         :param value: Value to set the option to. It must be a string or convertible
-            to a string"""
+            to a string
+        :return: this instance"""
         if not self.has_section(section):
             self.add_section(section)
         self.set(section, option, self._value_to_string(value))
+        return self
 
     def rename_section(self, section, new_name):
         """rename the given section to new_name

git/exc.py

@@ -84,3 +84,14 @@ def __init__(self, command, status, stdout, stderr):
     def __str__(self):
         return ("'%s' hook returned with exit code %i\nstdout: '%s'\nstderr: '%s'"
                 % (self.command, self.status, self.stdout, self.stderr))
+
+
+class RepositoryDirtyError(Exception):
+    """Thrown whenever an operation on a repository fails as it has uncommited changes that would be overwritten"""
+
+    def __init__(self, repo, message):
+        self.repo = repo
+        self.message = message
+
+    def __str__(self):
+        return "Operation cannot be performed on %r: %s" % (self.repo, self.message)

git/objects/submodule/base.py

@@ -24,7 +24,8 @@
 )
 from git.exc import (
     InvalidGitRepositoryError,
-    NoSuchPathError
+    NoSuchPathError,
+    RepositoryDirtyError
 )
 from git.compat import (
     string_types,
@@ -36,6 +37,7 @@
 
 import os
 import logging
+import tempfile
 
 __all__ = ["Submodule", "UpdateProgress"]
 
@@ -424,8 +426,8 @@ def add(cls, repo, name, path, url=None, branch=None, no_checkout=False):
 
         return sm
 
-    def update(self, recursive=False, init=True, to_latest_revision=False, progress=None,
-               dry_run=False):
+    def update(self, recursive=False, init=True, to_latest_revision=False, progress=None, dry_run=False,
+                     force=False):
         """Update the repository of this submodule to point to the checkout
         we point at with the binsha of this instance.
 
@@ -440,6 +442,12 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress=
         :param progress: UpdateProgress instance or None of no progress should be shown
         :param dry_run: if True, the operation will only be simulated, but not performed.
             All performed operations are read-only
+        :param force: 
+            If True, we may reset heads even if the repository in question is dirty. Additinoally we will be allowed
+            to set a tracking branch which is ahead of its remote branch back into the past or the location of the 
+            remote branch. This will essentially 'forget' commits.
+            If False, local tracking branches that are in the future of their respective remote branches will simply 
+            not be moved.
         :note: does nothing in bare repositories
         :note: method is definitely not atomic if recurisve is True
         :return: self"""
@@ -565,23 +573,45 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress=
         # update the working tree
         # handles dry_run
         if mrepo is not None and mrepo.head.commit.binsha != binsha:
+            # We must assure that our destination sha (the one to point to) is in the future of our current head.
+            # Otherwise, we will reset changes that might have been done on the submodule, but were not yet pushed
+            # We also handle the case that history has been rewritten, leaving no merge-base. In that case
+            # we behave conservatively, protecting possible changes the user had done
+            may_reset = True
+            if mrepo.head.commit.binsha != self.NULL_BIN_SHA:
+                base_commit = mrepo.merge_base(mrepo.head.commit, hexsha)
+                if len(base_commit) == 0 or base_commit[0].hexsha == hexsha:
+                    if force:
+                        log.debug("Will force checkout or reset on local branch that is possibly in the future of" +
+                                  "the commit it will be checked out to, effectively 'forgetting' new commits")
+                    else:
+                        log.info("Skipping %s on branch '%s' of submodule repo '%s' as it contains un-pushed commits",
+                                 is_detached and "checkout" or "reset", mrepo.head, mrepo)
+                        may_reset = False
+                    # end handle force
+                # end handle if we are in the future
+
+                if may_reset and not force and mrepo.is_dirty(index=True, working_tree=True, untracked_files=True):
+                    raise RepositoryDirtyError(mrepo, "Cannot reset a dirty repository")
+                # end handle force and dirty state
+            # end handle empty repo
+
+            # end verify future/past
             progress.update(BEGIN | UPDWKTREE, 0, 1, prefix +
                             "Updating working tree at %s for submodule %r to revision %s"
                             % (self.path, self.name, hexsha))
-            if not dry_run:
+
+            if not dry_run and may_reset:
                 if is_detached:
                     # NOTE: for now we force, the user is no supposed to change detached
                     # submodules anyway. Maybe at some point this becomes an option, to
                     # properly handle user modifications - see below for future options
                     # regarding rebase and merge.
-                    mrepo.git.checkout(hexsha, force=True)
+                    mrepo.git.checkout(hexsha, force=force)
                 else:
-                    # TODO: allow to specify a rebase, merge, or reset
-                    # TODO: Warn if the hexsha forces the tracking branch off the remote
-                    # branch - this should be prevented when setting the branch option
                     mrepo.head.reset(hexsha, index=True, working_tree=True)
                 # END handle checkout
-            # END handle dry_run
+            # if we may reset/checkout
             progress.update(END | UPDWKTREE, 0, 1, prefix + "Done updating working tree for submodule %r" % self.name)
         # END update to new commit only if needed
 
@@ -591,7 +621,8 @@ def update(self, recursive=False, init=True, to_latest_revision=False, progress=
             # in dry_run mode, the module might not exist
             if mrepo is not None:
                 for submodule in self.iter_items(self.module()):
-                    submodule.update(recursive, init, to_latest_revision, progress=progress, dry_run=dry_run)
+                    submodule.update(recursive, init, to_latest_revision, progress=progress, dry_run=dry_run, 
+                                     force=force)
                 # END handle recursive update
             # END handle dry run
         # END for each submodule
@@ -748,7 +779,7 @@ def remove(self, module=True, force=False, configuration=True, dry_run=False):
             csm.remove(module, force, configuration, dry_run)
             del(csm)
         # end
-        if nc > 0:
+        if not dry_run and 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)
@@ -854,8 +885,7 @@ def remove(self, module=True, force=False, configuration=True, dry_run=False):
     def set_parent_commit(self, commit, check=True):
         """Set this instance to use the given commit whose tree is supposed to
         contain the .gitmodules blob.
-
-        :param commit: Commit'ish reference pointing at the root_tree
+        :param commit: Commit'ish reference pointing at the root_tree, or None always point to the most recent commit
         :param check: if True, relatively expensive checks will be performed to verify
             validity of the submodule.
         :raise ValueError: if the commit's tree didn't contain the .gitmodules blob.
@@ -939,7 +969,7 @@ def rename(self, new_name):
         pw.release()
 
         # .gitmodules
-        cw = self.config_writer().config
+        cw = self.config_writer(write=False).config
         cw.rename_section(sm_section(self.name), sm_section(new_name))
         cw.release()
 
@@ -948,9 +978,16 @@ def rename(self, new_name):
         # .git/modules
         mod = self.module()
         if mod.has_separate_working_tree():
-            module_abspath = self._module_abspath(self.repo, self.path, new_name)
-            os.renames(mod.git_dir, module_abspath)
-            self._write_git_file_and_module_config(mod.working_tree_dir, module_abspath)
+            destination_module_abspath = self._module_abspath(self.repo, self.path, new_name)
+            source_dir = mod.git_dir
+            # Let's be sure the submodule name is not so obviously tied to a directory
+            if destination_module_abspath.startswith(mod.git_dir):
+                tmp_dir = self._module_abspath(self.repo, self.path, os.path.basename(tempfile.mkdtemp()))
+                os.renames(source_dir, tmp_dir)
+                source_dir = tmp_dir
+            # end handle self-containment
+            os.renames(source_dir, destination_module_abspath)
+            self._write_git_file_and_module_config(mod.working_tree_dir, destination_module_abspath)
         # end move separate git repository
 
         return self

git/objects/submodule/root.py

@@ -61,7 +61,7 @@ def _clear_cache(self):
     #{ Interface
 
     def update(self, previous_commit=None, recursive=True, force_remove=False, init=True,
-               to_latest_revision=False, progress=None, dry_run=False):
+               to_latest_revision=False, progress=None, dry_run=False, force_reset=False):
         """Update the submodules of this repository to the current HEAD commit.
         This method behaves smartly by determining changes of the path of a submodules
         repository, next to changes to the to-be-checked-out commit or the branch to be
@@ -80,10 +80,16 @@ def update(self, previous_commit=None, recursive=True, force_remove=False, init=
         :param init: If we encounter a new module which would need to be initialized, then do it.
         :param to_latest_revision: If True, instead of checking out the revision pointed to
             by this submodule's sha, the checked out tracking branch will be merged with the
-            newest remote branch fetched from the repository's origin
+            latest remote branch fetched from the repository's origin.
+            Unless force_reset is specified, a local tracking branch will never be reset into its past, therefore
+            the remote branch must be in the future for this to have an effect.
+        :param force_reset: if True, submodules may checkout or reset their branch even if the repository has
+            pending changes that would be overwritten, or if the local tracking branch is in the future of the 
+            remote tracking branch and would be reset into its past.
         :param progress: RootUpdateProgress instance or None if no progress should be sent
         :param dry_run: if True, operations will not actually be performed. Progress messages
-            will change accordingly to indicate the WOULD DO state of the operation."""
+            will change accordingly to indicate the WOULD DO state of the operation.
+        :return: self"""
         if self.repo.bare:
             raise InvalidGitRepositoryError("Cannot update submodules in bare repositories")
         # END handle bare
@@ -134,9 +140,7 @@ def update(self, previous_commit=None, recursive=True, force_remove=False, init=
             # of previous module. Trigger the cache to be updated before that
             progress.update(op, i, len_rrsm, prefix + "Removing submodule %r at %s" % (rsm.name, rsm.abspath))
             rsm._parent_commit = repo.head.commit
-            if not dry_run:
-                rsm.remove(configuration=False, module=True, force=force_remove)
-            # END handle dry-run
+            rsm.remove(configuration=False, module=True, force=force_remove, dry_run=dry_run)
 
             if i == len_rrsm - 1:
                 op |= END
@@ -311,7 +315,7 @@ def update(self, previous_commit=None, recursive=True, force_remove=False, init=
         for sm in sms:
             # update the submodule using the default method
             sm.update(recursive=False, init=init, to_latest_revision=to_latest_revision,
-                      progress=progress, dry_run=dry_run)
+                      progress=progress, dry_run=dry_run, force=force_reset)
 
             # update recursively depth first - question is which inconsitent
             # state will be better in case it fails somewhere. Defective branch
@@ -322,11 +326,13 @@ def update(self, previous_commit=None, recursive=True, force_remove=False, init=
                 if sm.module_exists():
                     type(self)(sm.module()).update(recursive=True, force_remove=force_remove,
                                                    init=init, to_latest_revision=to_latest_revision,
-                                                   progress=progress, dry_run=dry_run)
+                                                   progress=progress, dry_run=dry_run, force_reset=force_reset)
                 # END handle dry_run
             # END handle recursive
         # END for each submodule to update
 
+        return self
+
     def module(self):
         """:return: the actual repository containing the submodules"""
         return self.repo

git/repo/fun.py

@@ -296,7 +296,7 @@ def rev_parse(repo, rev):
                 raise ValueError("Invalid token: %r" % token)
             # END end handle tag
         except (IndexError, AttributeError):
-            raise BadObject("Invalid Revision in %s" % rev)
+            raise BadName("Invalid revision spec '%s' - not enough parent commits to reach '%s%i'" % (rev, token, num))
         # END exception handling
     # END parse loop