Re: [StGit PATCH] Check for local changes with "goto"

[Catalin Marinas <catalin.marinas@gmail.com>]

2009/1/30 Catalin Marinas <catalin.marinas@gmail.com>:
> Now, should we add the check_clean argument to Transaction.__init__()
> rather than run() as we do for the allow_bad_head case?

It looks like this may be a better option. The previous patch fails if
"goto" pushes a patch with standard git-apply followed by another
patch with a three-way merge. When Transaction.run() is called, even
if the patch pushing succeeded, the function complains about local
changes because of the "iw.index.is_clean(self.stack.head)" check.

It is also a bit weird to push/pop patches and only complain at the
end of local changes. Below is an updated patch which does the
checking in Transaction.__init__ (only the relevant parts of the
patch):

diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index e2b4266..07079b8 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -706,9 +706,11 @@ class Index(RunWithEnv):
                     ).output_one_line())
         except run.RunException:
             raise MergeException('Conflicting merge')
-    def is_clean(self):
+    def is_clean(self, tree):
+        """Check whether the index is clean relative to the given treeish."""
         try:
-            self.run(['git', 'update-index', '--refresh']).discard_output()
+            self.run(['git', 'diff-index', '--quiet', '--cached', tree.sha1]
+                    ).discard_output()
         except run.RunException:
             return False
         else:
@@ -858,6 +860,14 @@ class IndexAndWorktree(RunWithEnvCwd):
         cmd = ['git', 'update-index', '--remove']
         self.run(cmd + ['-z', '--stdin']
                  ).input_nulterm(paths).discard_output()
+    def worktree_clean(self):
+        """Check whether the worktree is clean relative to index."""
+        try:
+            self.run(['git', 'update-index', '--refresh']).discard_output()
+        except run.RunException:
+            return False
+        else:
+            return True

 class Branch(object):
     """Represents a Git branch."""
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 54de127..e1bd38d 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -75,7 +75,8 @@ class StackTransaction(object):
       your refs and index+worktree, or fail without having done
       anything."""
     def __init__(self, stack, msg, discard_changes = False,
-                 allow_conflicts = False, allow_bad_head = False):
+                 allow_conflicts = False, allow_bad_head = False,
+                 check_clean = False):
         """Create a new L{StackTransaction}.

         @param discard_changes: Discard any changes in index+worktree
@@ -102,6 +103,8 @@ class StackTransaction(object):
         self.__temp_index = self.temp_index_tree = None
         if not allow_bad_head:
             self.__assert_head_top_equal()
+        if check_clean:
+            self.__assert_index_worktree_clean()
     stack = property(lambda self: self.__stack)
     patches = property(lambda self: self.__patches)
     def __set_applied(self, val):
@@ -147,6 +150,12 @@ class StackTransaction(object):
                 'This can happen if you modify a branch with git.',
                 '"stg repair --help" explains more about what to do next.')
             self.__abort()
+    def __assert_index_worktree_clean(self):
+        iw = self.__stack.repository.default_iw
+        if not iw.worktree_clean() or \
+           not iw.index.is_clean(self.stack.head):
+            self.__halt('Repository not clean. Use "refresh" or '
+                        '"status --reset"')
     def __checkout(self, tree, iw, allow_bad_head):
         if not allow_bad_head:
             self.__assert_head_top_equal()


-- 
Catalin