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

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

[Catalin Marinas]

This is done by default, unless the --keep option is passed, for
consistency with the "pop" command. The index is checked in the
Transaction.run() function so that other commands could benefit from
this feature (off by default).

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/goto.py    |    8 ++++++--
 stgit/lib/transaction.py  |    7 ++++++-
 t/t2300-refresh-subdir.sh |    2 +-
 t/t2800-goto-subdir.sh    |    4 ++--
 t/t3000-dirty-merge.sh    |    2 +-
 5 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/stgit/commands/goto.py b/stgit/commands/goto.py
index 60a917e..7c5ad39 100644
--- a/stgit/commands/goto.py
+++ b/stgit/commands/goto.py
@@ -18,6 +18,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 from stgit.commands import common
 from stgit.lib import transaction
 from stgit import argparse
+from stgit.argparse import opt
 
 help = 'Push or pop patches to the given one'
 kind = 'stack'
@@ -27,7 +28,10 @@ Push/pop patches to/from the stack until the one given on the command
 line becomes current."""
 
 args = [argparse.other_applied_patches, argparse.unapplied_patches]
-options = []
+options = [
+    opt('-k', '--keep', action = 'store_true',
+        short = 'Keep the local changes')
+]
 
 directory = common.DirectoryHasRepositoryLib()
 
@@ -52,4 +56,4 @@ def func(parser, options, args):
         raise common.CmdException('Cannot goto a hidden patch')
     else:
         raise common.CmdException('Patch "%s" does not exist' % patch)
-    return trans.run(iw)
+    return trans.run(iw, check_clean = not options.keep)
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 54de127..2b8f2de 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -183,13 +183,18 @@ class StackTransaction(object):
             self.__checkout(self.__stack.head.data.tree, iw,
                             allow_bad_head = True)
     def run(self, iw = None, set_head = True, allow_bad_head = False,
-            print_current_patch = True):
+            print_current_patch = True, check_clean = False):
         """Execute the transaction. Will either succeed, or fail (with an
         exception) and do nothing."""
         self.__check_consistency()
         log.log_external_mods(self.__stack)
         new_head = self.head
 
+        # Check for not clean index
+        if check_clean and iw and not iw.index.is_clean():
+            self.__halt('Repository not clean. Use "refresh" or '
+                        '"status --reset"')
+
         # Set branch head.
         if set_head:
             if iw:
diff --git a/t/t2300-refresh-subdir.sh b/t/t2300-refresh-subdir.sh
index d731a11..89c95db 100755
--- a/t/t2300-refresh-subdir.sh
+++ b/t/t2300-refresh-subdir.sh
@@ -65,7 +65,7 @@ test_expect_success 'refresh -u -p <subdir>' '
 
 test_expect_success 'refresh an unapplied patch' '
     stg refresh -u &&
-    stg goto p0 &&
+    stg goto --keep p0 &&
     test "$(stg status)" = "M foo.txt" &&
     stg refresh -p p1 &&
     test "$(stg status)" = "" &&
diff --git a/t/t2800-goto-subdir.sh b/t/t2800-goto-subdir.sh
index 28b8292..855972b 100755
--- a/t/t2800-goto-subdir.sh
+++ b/t/t2800-goto-subdir.sh
@@ -25,7 +25,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (just pop)' '
-    (cd foo && stg goto p1) &&
+    (cd foo && stg goto --keep p1) &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
     ls foo > actual.txt &&
@@ -48,7 +48,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (conflicting push)' '
-    (cd foo && stg goto p3) ;
+    (cd foo && stg goto --keep p3) ;
     [ $? -eq 3 ] &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
diff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh
index f0f79d5..419d86e 100755
--- a/t/t3000-dirty-merge.sh
+++ b/t/t3000-dirty-merge.sh
@@ -26,7 +26,7 @@ test_expect_success 'Push with dirty worktree' '
     echo 4 > a &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
-    conflict stg goto p2 &&
+    conflict stg goto --keep p2 &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
     [ "$(echo $(cat a))" = "4" ]

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

[Karl Hasselström]

On 2009-01-28 23:13:05 +0000, Catalin Marinas wrote:

> This is done by default, unless the --keep option is passed, for
> consistency with the "pop" command. The index is checked in the
> Transaction.run() function so that other commands could benefit from
> this feature (off by default).

This looks good, except for ...

> +        # Check for not clean index
> +        if check_clean and iw and not iw.index.is_clean():
> +            self.__halt('Repository not clean. Use "refresh" or '
> +                        '"status --reset"')

... this, which doesn't do what I think you think it does.

Index.is_clean() calls "git update-index --refresh", which checks for
changes in the worktree relative to the index. It's bad design to have
it in Index rather than IndexAndWorktree, but that's my fault, not
yours. ;-) But the point that breaks your patch is that it doesn't
check for changes between index and HEAD -- try it and see.

The fix I'd suggest is to move the existing is_clean() method to
IndexAndWorktree, and call it maybe worktree_clean(). And create a
method in Index() called is_clean(tree) that checks whether the index
is clean with respect to the given Tree (I think this method should
just call "git diff-index --quiet --cached <tree>".). Then call both
of these methods.

Sorry if I just keep creating more work for you. :-/

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

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

[Catalin Marinas]

2009/1/29 Karl Hasselström <kha@treskal.com>:
> On 2009-01-28 23:13:05 +0000, Catalin Marinas wrote:
>> +        # Check for not clean index
>> +        if check_clean and iw and not iw.index.is_clean():
>> +            self.__halt('Repository not clean. Use "refresh" or '
>> +                        '"status --reset"')
>
> ... this, which doesn't do what I think you think it does.
>
> Index.is_clean() calls "git update-index --refresh", which checks for
> changes in the worktree relative to the index. It's bad design to have
> it in Index rather than IndexAndWorktree, but that's my fault, not
> yours. ;-) But the point that breaks your patch is that it doesn't
> check for changes between index and HEAD -- try it and see.
>
> The fix I'd suggest is to move the existing is_clean() method to
> IndexAndWorktree, and call it maybe worktree_clean(). And create a
> method in Index() called is_clean(tree) that checks whether the index
> is clean with respect to the given Tree (I think this method should
> just call "git diff-index --quiet --cached <tree>".). Then call both
> of these methods.

What about this (only pasting the relevant hunks, though they may be
wrapped by the web interface):

diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index e2b4266..7e1b9dd 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -706,9 +706,15 @@ class Index(RunWithEnv):
                     ).output_one_line())
         except run.RunException:
             raise MergeException('Conflicting merge')
-    def is_clean(self):
+    def is_clean(self, tree = None):
+        """Check whether the index is clean relative to the given tree."""
+        if tree:
+            sha1 = tree.sha1
+        else:
+            sha1 = 'HEAD'
         try:
-            self.run(['git', 'update-index', '--refresh']).discard_output()
+            self.run(['git', 'diff-index', '--quiet', '--cached', sha1]
+                    ).discard_output()
         except run.RunException:
             return False
         else:
@@ -858,6 +864,15 @@ 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 and no updates or
+        merges are needed."""
+        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..8abf296 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -183,13 +183,22 @@ class StackTransaction(object):
             self.__checkout(self.__stack.head.data.tree, iw,
                             allow_bad_head = True)
     def run(self, iw = None, set_head = True, allow_bad_head = False,
-            print_current_patch = True):
+            print_current_patch = True, check_clean = False):
         """Execute the transaction. Will either succeed, or fail (with an
         exception) and do nothing."""
         self.__check_consistency()
         log.log_external_mods(self.__stack)
         new_head = self.head

+        # Check for not clean index and worktree
+        if check_clean and iw:
+            if not iw.worktree_clean():
+                self.__halt('Repository not clean. Use "refresh" or '
+                            '"status --reset"')
+            elif not iw.index.is_clean()):
+                self.__halt('Index and HEAD different. Use "repair" to '
+                            'recover additional commits')
+
         # Set branch head.
         if set_head:
             if iw:


Thanks.

-- 
Catalin

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

[Karl Hasselström]

On 2009-01-30 14:01:20 +0000, Catalin Marinas wrote:

> @@ -706,9 +706,15 @@ class Index(RunWithEnv):
>                      ).output_one_line())
>          except run.RunException:
>              raise MergeException('Conflicting merge')
> -    def is_clean(self):
> +    def is_clean(self, tree = None):
> +        """Check whether the index is clean relative to the given tree."""
> +        if tree:
> +            sha1 = tree.sha1
> +        else:
> +            sha1 = 'HEAD'
>          try:
> -            self.run(['git', 'update-index', '--refresh']).discard_output()
> +            self.run(['git', 'diff-index', '--quiet', '--cached', sha1]
> +                    ).discard_output()
>          except run.RunException:
>              return False
>          else:

OK (though I personally would have allowed only Tree objects, with no
defaulting to the current HEAD).

The docstring should say s/tree/treeish/.

> @@ -858,6 +864,15 @@ 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 and no updates or
> +        merges are needed."""
> +        try:
> +            self.run(['git', 'update-index', '--refresh']).discard_output()
> +        except run.RunException:
> +            return False
> +        else:
> +            return True

Clean relative to the index.

And what do merges have to do with it?

> +        # Check for not clean index and worktree
> +        if check_clean and iw:
> +            if not iw.worktree_clean():
> +                self.__halt('Repository not clean. Use "refresh" or '
> +                            '"status --reset"')
> +            elif not iw.index.is_clean()):
> +                self.__halt('Index and HEAD different. Use "repair" to '
> +                            'recover additional commits')

The first message is good, but the second one is misleading -- you'd
be much better off just reusing the same message as for the first
case. (You could recommend refresh --index if you want to get fancy.)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

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

[Catalin Marinas]

2009/1/30 Karl Hasselström <kha@treskal.com>:
> On 2009-01-30 14:01:20 +0000, Catalin Marinas wrote:
>
>> @@ -706,9 +706,15 @@ class Index(RunWithEnv):
>>                      ).output_one_line())
>>          except run.RunException:
>>              raise MergeException('Conflicting merge')
>> -    def is_clean(self):
>> +    def is_clean(self, tree = None):
>> +        """Check whether the index is clean relative to the given tree."""
>> +        if tree:
>> +            sha1 = tree.sha1
>> +        else:
>> +            sha1 = 'HEAD'
>>          try:
>> -            self.run(['git', 'update-index', '--refresh']).discard_output()
>> +            self.run(['git', 'diff-index', '--quiet', '--cached', sha1]
>> +                    ).discard_output()
>>          except run.RunException:
>>              return False
>>          else:
>
> OK (though I personally would have allowed only Tree objects, with no
> defaulting to the current HEAD).

Done.

See below for an update. I added an __assert_index_worktree_clean()
function in Transaction.

Now, should we add the check_clean argument to Transaction.__init__()
rather than run() as we do for the allow_bad_head case? The check
would need to be done in run() where we have an iw. Just a thought,
I'm not convinced it is better.


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..c961222 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -147,6 +147,11 @@ 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):
+        if not iw.worktree_clean() or \
+           not iw.index.is_clean(self.stack.head.data.tree):
+            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()
@@ -183,13 +188,17 @@ class StackTransaction(object):
             self.__checkout(self.__stack.head.data.tree, iw,
                             allow_bad_head = True)
     def run(self, iw = None, set_head = True, allow_bad_head = False,
-            print_current_patch = True):
+            print_current_patch = True, check_clean = False):
         """Execute the transaction. Will either succeed, or fail (with an
         exception) and do nothing."""
         self.__check_consistency()
         log.log_external_mods(self.__stack)
         new_head = self.head

+        # Check for clean index and worktree
+        if check_clean and iw:
+            self.__assert_index_worktree_clean(iw)
+
         # Set branch head.
         if set_head:
             if iw:


-- 
Catalin

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

[Catalin Marinas]

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

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

[Karl Hasselström]

On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:

> 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.

Sorry for taking so long to respond, but ... I strongly advise against
using default_iw in transaction.py. It's library code, and it should
take stuff like index and worktree as input parameters from layers
that are higher up in the abstraction stack. Compare the kernel policy
of having policy in userspace and not in the kernel.

And if you accept that reasoning, the check has to go in run() rather
than __init__(), because we don't have an iw in __init__(). (Though we
could add such a parameter, I guess.)

> 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.

Hmm, so that would have to be worked around somehow ... I guess doing
the check in __init__() might make sense after all, since that's
before we start changing things. How about adding a
check_clean_relative_to paramter to __init__() that's not a boolean,
but an iw to check against? It would default to None, meaning no
check.

> It is also a bit weird to push/pop patches and only complain at the
> end of local changes.

You mean the behavior the new infrastructure currently gives you? It's
actually convenient in a number of cases. Assume for example that you
have patch A that changes file foo, patch B that changes file bar, and
then local changes to file bar. At this point you can pop A without
problem, even though a middle stage is to pop and push B which touches
the same file as your local changes -- the existing checks will only
compare the diff between the original and final tree with your local
changes, and that diff doesn't contain bar.

> Below is an updated patch which does the checking in
> Transaction.__init__ (only the relevant parts of the patch):

Looks good, except for the hard-coded default_iw as I mentioned above.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

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

[Catalin Marinas]

2009/2/6 Karl Hasselström <kha@treskal.com>:
> On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:
>
>> 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.
>
> Sorry for taking so long to respond, but ... I strongly advise against
> using default_iw in transaction.py. It's library code, and it should
> take stuff like index and worktree as input parameters from layers
> that are higher up in the abstraction stack.

OK, no problem with that.

>> 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.
>
> Hmm, so that would have to be worked around somehow ... I guess doing
> the check in __init__() might make sense after all, since that's
> before we start changing things. How about adding a
> check_clean_relative_to paramter to __init__() that's not a boolean,
> but an iw to check against? It would default to None, meaning no
> check.

OK, that's better.

>> It is also a bit weird to push/pop patches and only complain at the
>> end of local changes.
>
> You mean the behavior the new infrastructure currently gives you? It's
> actually convenient in a number of cases. Assume for example that you
> have patch A that changes file foo, patch B that changes file bar, and
> then local changes to file bar. At this point you can pop A without
> problem, even though a middle stage is to pop and push B which touches
> the same file as your local changes -- the existing checks will only
> compare the diff between the original and final tree with your local
> changes, and that diff doesn't contain bar.

Yes, I agree that's a nice feature and it would still be available
with the --keep option (I wrote in the past why I wouldn't leave the
current behaviour to be the default).

Above I was referring to the new behaviour which checks for local
changes by default (--keep not passed). If we do the test in run() you
may push or pop patches and only fail at the end when actually the
operation shouldn't have started. I plan to add the auto interactive
merging to the new infrastructure (by invoking mergetool if
IndexAndWorktree.merge() fails, based on the stgit.autoimerge option)
and pushing may become a more complex operation if enabled.

I'll repost the patch. Thanks.

-- 
Catalin

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

[Catalin Marinas]

This is done by default, unless the --keep option is passed, for
consistency with the "pop" command. The index is checked in the
Transaction.run() function so that other commands could benefit from
this feature (off by default).

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---

This is hopefully the final variant of what was discussed in a previous
thread. The next step is to have the --keep behaviour configurable via
.gitconfig.

 stgit/commands/goto.py    |   13 +++++++++++--
 stgit/lib/git.py          |   14 ++++++++++++--
 stgit/lib/transaction.py  |   10 +++++++++-
 t/t2300-refresh-subdir.sh |    2 +-
 t/t2800-goto-subdir.sh    |    4 ++--
 t/t3000-dirty-merge.sh    |    2 +-
 6 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/stgit/commands/goto.py b/stgit/commands/goto.py
index 60a917e..4f6274f 100644
--- a/stgit/commands/goto.py
+++ b/stgit/commands/goto.py
@@ -18,6 +18,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 from stgit.commands import common
 from stgit.lib import transaction
 from stgit import argparse
+from stgit.argparse import opt
 
 help = 'Push or pop patches to the given one'
 kind = 'stack'
@@ -27,7 +28,10 @@ Push/pop patches to/from the stack until the one given on the command
 line becomes current."""
 
 args = [argparse.other_applied_patches, argparse.unapplied_patches]
-options = []
+options = [
+    opt('-k', '--keep', action = 'store_true',
+        short = 'Keep the local changes')
+]
 
 directory = common.DirectoryHasRepositoryLib()
 
@@ -38,7 +42,12 @@ def func(parser, options, args):
 
     stack = directory.repository.current_stack
     iw = stack.repository.default_iw
-    trans = transaction.StackTransaction(stack, 'goto')
+    if not options.keep:
+        check_clean_iw = iw
+    else:
+        check_clean_iw = None
+    trans = transaction.StackTransaction(stack, 'goto',
+                                         check_clean_iw = check_clean_iw)
     if patch in trans.applied:
         to_pop = set(trans.applied[trans.applied.index(patch)+1:])
         assert not trans.pop_patches(lambda pn: pn in to_pop)
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..5a81f9d 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_iw = None):
         """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_iw:
+            self.__assert_index_worktree_clean(check_clean_iw)
     stack = property(lambda self: self.__stack)
     patches = property(lambda self: self.__patches)
     def __set_applied(self, val):
@@ -147,6 +150,11 @@ 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):
+        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()
diff --git a/t/t2300-refresh-subdir.sh b/t/t2300-refresh-subdir.sh
index d731a11..89c95db 100755
--- a/t/t2300-refresh-subdir.sh
+++ b/t/t2300-refresh-subdir.sh
@@ -65,7 +65,7 @@ test_expect_success 'refresh -u -p <subdir>' '
 
 test_expect_success 'refresh an unapplied patch' '
     stg refresh -u &&
-    stg goto p0 &&
+    stg goto --keep p0 &&
     test "$(stg status)" = "M foo.txt" &&
     stg refresh -p p1 &&
     test "$(stg status)" = "" &&
diff --git a/t/t2800-goto-subdir.sh b/t/t2800-goto-subdir.sh
index 28b8292..855972b 100755
--- a/t/t2800-goto-subdir.sh
+++ b/t/t2800-goto-subdir.sh
@@ -25,7 +25,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (just pop)' '
-    (cd foo && stg goto p1) &&
+    (cd foo && stg goto --keep p1) &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
     ls foo > actual.txt &&
@@ -48,7 +48,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (conflicting push)' '
-    (cd foo && stg goto p3) ;
+    (cd foo && stg goto --keep p3) ;
     [ $? -eq 3 ] &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
diff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh
index f0f79d5..419d86e 100755
--- a/t/t3000-dirty-merge.sh
+++ b/t/t3000-dirty-merge.sh
@@ -26,7 +26,7 @@ test_expect_success 'Push with dirty worktree' '
     echo 4 > a &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
-    conflict stg goto p2 &&
+    conflict stg goto --keep p2 &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
     [ "$(echo $(cat a))" = "4" ]

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

[Karl Hasselström]

On 2009-02-10 14:11:51 +0000, Catalin Marinas wrote:

> This is hopefully the final variant of what was discussed in a
> previous thread. The next step is to have the --keep behaviour
> configurable via .gitconfig.

Yeah, I can't find anything more to complain about. :-)

Acked-by: Karl Hasselström <kha@treskal.com>

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

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

[Catalin Marinas]

This is done by default, unless the --keep option is passed, for
consistency with the "pop" command.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---
 stgit/commands/common.py  |    7 +++++++
 stgit/commands/goto.py    |    8 +++++++-
 t/t2300-refresh-subdir.sh |    2 +-
 t/t2800-goto-subdir.sh    |    4 ++--
 t/t3000-dirty-merge.sh    |    2 +-
 5 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 6bb3685..8ae43ff 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -100,6 +100,13 @@ def check_conflicts():
                            ' then use "resolve <files>" or revert the'
                            ' changes with "status --reset".')
 
+def check_clean(repository):
+    """Check whether the index is up to date.
+    """
+    if not repository.default_index.is_clean():
+        raise CmdException('Repository not clean. Use "refresh" or '
+                           '"status --reset"')
+
 def print_crt_patch(crt_series, branch = None):
     if not branch:
         patch = crt_series.get_current()
diff --git a/stgit/commands/goto.py b/stgit/commands/goto.py
index 60a917e..6483011 100644
--- a/stgit/commands/goto.py
+++ b/stgit/commands/goto.py
@@ -18,6 +18,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 from stgit.commands import common
 from stgit.lib import transaction
 from stgit import argparse
+from stgit.argparse import opt
 
 help = 'Push or pop patches to the given one'
 kind = 'stack'
@@ -27,7 +28,10 @@ Push/pop patches to/from the stack until the one given on the command
 line becomes current."""
 
 args = [argparse.other_applied_patches, argparse.unapplied_patches]
-options = []
+options = [
+    opt('-k', '--keep', action = 'store_true',
+        short = 'Keep the local changes')
+]
 
 directory = common.DirectoryHasRepositoryLib()
 
@@ -38,6 +42,8 @@ def func(parser, options, args):
 
     stack = directory.repository.current_stack
     iw = stack.repository.default_iw
+    if not options.keep:
+        common.check_clean(directory.repository)
     trans = transaction.StackTransaction(stack, 'goto')
     if patch in trans.applied:
         to_pop = set(trans.applied[trans.applied.index(patch)+1:])
diff --git a/t/t2300-refresh-subdir.sh b/t/t2300-refresh-subdir.sh
index d731a11..89c95db 100755
--- a/t/t2300-refresh-subdir.sh
+++ b/t/t2300-refresh-subdir.sh
@@ -65,7 +65,7 @@ test_expect_success 'refresh -u -p <subdir>' '
 
 test_expect_success 'refresh an unapplied patch' '
     stg refresh -u &&
-    stg goto p0 &&
+    stg goto --keep p0 &&
     test "$(stg status)" = "M foo.txt" &&
     stg refresh -p p1 &&
     test "$(stg status)" = "" &&
diff --git a/t/t2800-goto-subdir.sh b/t/t2800-goto-subdir.sh
index 28b8292..855972b 100755
--- a/t/t2800-goto-subdir.sh
+++ b/t/t2800-goto-subdir.sh
@@ -25,7 +25,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (just pop)' '
-    (cd foo && stg goto p1) &&
+    (cd foo && stg goto --keep p1) &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
     ls foo > actual.txt &&
@@ -48,7 +48,7 @@ cat > expected2.txt <<EOF
 bar
 EOF
 test_expect_success 'Goto in subdirectory (conflicting push)' '
-    (cd foo && stg goto p3) ;
+    (cd foo && stg goto --keep p3) ;
     [ $? -eq 3 ] &&
     cat foo/bar > actual.txt &&
     test_cmp expected1.txt actual.txt &&
diff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh
index f0f79d5..419d86e 100755
--- a/t/t3000-dirty-merge.sh
+++ b/t/t3000-dirty-merge.sh
@@ -26,7 +26,7 @@ test_expect_success 'Push with dirty worktree' '
     echo 4 > a &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
-    conflict stg goto p2 &&
+    conflict stg goto --keep p2 &&
     [ "$(echo $(stg series --applied --noprefix))" = "p1" ] &&
     [ "$(echo $(stg series --unapplied --noprefix))" = "p2" ] &&
     [ "$(echo $(cat a))" = "4" ]

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

[Karl Hasselström]

I'm not opposed to the change as such (even if I personally think it's
very convenient to allow operations like goto if the local changes
don't touch the same files), but ...

On 2009-01-14 22:59:45 +0000, Catalin Marinas wrote:

>  stgit/commands/common.py  |    7 +++++++
>  stgit/commands/goto.py    |    8 +++++++-

... are you sure it wouldn't be better to build the check into
transaction.py, so that all new-infrastructure command would share it?

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

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

[Catalin Marinas]

2009/1/15 Karl Hasselström <kha@treskal.com>:
> I'm not opposed to the change as such (even if I personally think it's
> very convenient to allow operations like goto if the local changes
> don't touch the same files), but ...

That would work if "refresh" only commits files previously added to
the patch (the same way as quilt). I found myself many times moving to
another patch and refreshing more than I wanted to add to that patch
because I forgot to run "status" before "goto".

Maybe we can make this configurable to accommodate both variants
(would be even easier if I move the check to transaction.py).

> On 2009-01-14 22:59:45 +0000, Catalin Marinas wrote:
>
>>  stgit/commands/common.py  |    7 +++++++
>>  stgit/commands/goto.py    |    8 +++++++-
>
> ... are you sure it wouldn't be better to build the check into
> transaction.py, so that all new-infrastructure command would share it?

OK, I'll do this and repost.

-- 
Catalin