{"thread":{"id":"17422","subject":"[StGit PATCH] Check for local changes with \"goto\"","startedAt":"2009-01-28T23:13:05Z","lastAt":"2009-02-06T18:39:47Z","messageCount":8,"participants":["Catalin Marinas","Karl Hasselström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"102367","messageId":"20090128231305.16133.29214.stgit@localhost.localdomain","threadId":"17422","inReplyTo":null,"subject":"[StGit PATCH] Check for local changes with \"goto\"","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-01-28T23:13:05Z","receivedAt":"2009-01-28T23:13:05Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"This is done by default, unless the --keep option is passed, for\nconsistency with the \"pop\" command. The index is checked in the\nTransaction.run() function so that other commands could benefit from\nthis feature (off by default).\n\nSigned-off-by: Catalin Marinas \n---\n stgit/commands/goto.py | 8 ++++++--\n stgit/lib/transaction.py | 7 ++++++-\n t/t2300-refresh-subdir.sh | 2 +-\n t/t2800-goto-subdir.sh | 4 ++--\n t/t3000-dirty-merge.sh | 2 +-\n 5 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/stgit/commands/goto.py b/stgit/commands/goto.py\nindex 60a917e..7c5ad39 100644\n--- a/stgit/commands/goto.py\n+++ b/stgit/commands/goto.py\n@@ -18,6 +18,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA\n from stgit.commands import common\n from stgit.lib import transaction\n from stgit import argparse\n+from stgit.argparse import opt\n \n help = 'Push or pop patches to the given one'\n kind = 'stack'\n@@ -27,7 +28,10 @@ Push/pop patches to/from the stack until the one given on the command\n line becomes current.\"\"\"\n \n args = [argparse.other_applied_patches, argparse.unapplied_patches]\n-options = []\n+options = [\n+ opt('-k', '--keep', action = 'store_true',\n+ short = 'Keep the local changes')\n+]\n \n directory = common.DirectoryHasRepositoryLib()\n \n@@ -52,4 +56,4 @@ def func(parser, options, args):\n raise common.CmdException('Cannot goto a hidden patch')\n else:\n raise common.CmdException('Patch \"%s\" does not exist' % patch)\n- return trans.run(iw)\n+ return trans.run(iw, check_clean = not options.keep)\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex 54de127..2b8f2de 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -183,13 +183,18 @@ class StackTransaction(object):\n self.__checkout(self.__stack.head.data.tree, iw,\n allow_bad_head = True)\n def run(self, iw = None, set_head = True, allow_bad_head = False,\n- print_current_patch = True):\n+ print_current_patch = True, check_clean = False):\n \"\"\"Execute the transaction. Will either succeed, or fail (with an\n exception) and do nothing.\"\"\"\n self.__check_consistency()\n log.log_external_mods(self.__stack)\n new_head = self.head\n \n+ # Check for not clean index\n+ if check_clean and iw and not iw.index.is_clean():\n+ self.__halt('Repository not clean. Use \"refresh\" or '\n+ '\"status --reset\"')\n+\n # Set branch head.\n if set_head:\n if iw:\ndiff --git a/t/t2300-refresh-subdir.sh b/t/t2300-refresh-subdir.sh\nindex d731a11..89c95db 100755\n--- a/t/t2300-refresh-subdir.sh\n+++ b/t/t2300-refresh-subdir.sh\n@@ -65,7 +65,7 @@ test_expect_success 'refresh -u -p ' '\n \n test_expect_success 'refresh an unapplied patch' '\n stg refresh -u &&\n- stg goto p0 &&\n+ stg goto --keep p0 &&\n test \"$(stg status)\" = \"M foo.txt\" &&\n stg refresh -p p1 &&\n test \"$(stg status)\" = \"\" &&\ndiff --git a/t/t2800-goto-subdir.sh b/t/t2800-goto-subdir.sh\nindex 28b8292..855972b 100755\n--- a/t/t2800-goto-subdir.sh\n+++ b/t/t2800-goto-subdir.sh\n@@ -25,7 +25,7 @@ cat > expected2.txt < actual.txt &&\n test_cmp expected1.txt actual.txt &&\n ls foo > actual.txt &&\n@@ -48,7 +48,7 @@ cat > expected2.txt < actual.txt &&\n test_cmp expected1.txt actual.txt &&\ndiff --git a/t/t3000-dirty-merge.sh b/t/t3000-dirty-merge.sh\nindex f0f79d5..419d86e 100755\n--- a/t/t3000-dirty-merge.sh\n+++ b/t/t3000-dirty-merge.sh\n@@ -26,7 +26,7 @@ test_expect_success 'Push with dirty worktree' '\n echo 4 > a &&\n [ \"$(echo $(stg series --applied --noprefix))\" = \"p1\" ] &&\n [ \"$(echo $(stg series --unapplied --noprefix))\" = \"p2\" ] &&\n- conflict stg goto p2 &&\n+ conflict stg goto --keep p2 &&\n [ \"$(echo $(stg series --applied --noprefix))\" = \"p1\" ] &&\n [ \"$(echo $(stg series --unapplied --noprefix))\" = \"p2\" ] &&\n [ \"$(echo $(cat a))\" = \"4\" ]\n"},{"id":"102398","messageId":"20090129034512.GD24344@diana.vm.bytemark.co.uk","threadId":"17422","inReplyTo":"20090128231305.16133.29214.stgit@localhost.localdomain","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-01-29T03:45:12Z","receivedAt":"2009-01-29T03:45:12Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-01-28 23:13:05 +0000, Catalin Marinas wrote:\n\n> This is done by default, unless the --keep option is passed, for\n> consistency with the \"pop\" command. The index is checked in the\n> Transaction.run() function so that other commands could benefit from\n> this feature (off by default).\n\nThis looks good, except for ...\n\n> + # Check for not clean index\n> + if check_clean and iw and not iw.index.is_clean():\n> + self.__halt('Repository not clean. Use \"refresh\" or '\n> + '\"status --reset\"')\n\n... this, which doesn't do what I think you think it does.\n\nIndex.is_clean() calls \"git update-index --refresh\", which checks for\nchanges in the worktree relative to the index. It's bad design to have\nit in Index rather than IndexAndWorktree, but that's my fault, not\nyours. ;-) But the point that breaks your patch is that it doesn't\ncheck for changes between index and HEAD -- try it and see.\n\nThe fix I'd suggest is to move the existing is_clean() method to\nIndexAndWorktree, and call it maybe worktree_clean(). And create a\nmethod in Index() called is_clean(tree) that checks whether the index\nis clean with respect to the given Tree (I think this method should\njust call \"git diff-index --quiet --cached \".). Then call both\nof these methods.\n\nSorry if I just keep creating more work for you. :-/\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"102595","messageId":"b0943d9e0901300601j27ab6ebdq4b38a9f7c0cbe261@mail.gmail.com","threadId":"17422","inReplyTo":"20090129034512.GD24344@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-01-30T14:01:20Z","receivedAt":"2009-01-30T14:01:20Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/1/29 Karl Hasselström :\n> On 2009-01-28 23:13:05 +0000, Catalin Marinas wrote:\n>> + # Check for not clean index\n>> + if check_clean and iw and not iw.index.is_clean():\n>> + self.__halt('Repository not clean. Use \"refresh\" or '\n>> + '\"status --reset\"')\n>\n> ... this, which doesn't do what I think you think it does.\n>\n> Index.is_clean() calls \"git update-index --refresh\", which checks for\n> changes in the worktree relative to the index. It's bad design to have\n> it in Index rather than IndexAndWorktree, but that's my fault, not\n> yours. ;-) But the point that breaks your patch is that it doesn't\n> check for changes between index and HEAD -- try it and see.\n>\n> The fix I'd suggest is to move the existing is_clean() method to\n> IndexAndWorktree, and call it maybe worktree_clean(). And create a\n> method in Index() called is_clean(tree) that checks whether the index\n> is clean with respect to the given Tree (I think this method should\n> just call \"git diff-index --quiet --cached \".). Then call both\n> of these methods.\n\nWhat about this (only pasting the relevant hunks, though they may be\nwrapped by the web interface):\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex e2b4266..7e1b9dd 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -706,9 +706,15 @@ class Index(RunWithEnv):\n ).output_one_line())\n except run.RunException:\n raise MergeException('Conflicting merge')\n- def is_clean(self):\n+ def is_clean(self, tree = None):\n+ \"\"\"Check whether the index is clean relative to the given tree.\"\"\"\n+ if tree:\n+ sha1 = tree.sha1\n+ else:\n+ sha1 = 'HEAD'\n try:\n- self.run(['git', 'update-index', '--refresh']).discard_output()\n+ self.run(['git', 'diff-index', '--quiet', '--cached', sha1]\n+ ).discard_output()\n except run.RunException:\n return False\n else:\n@@ -858,6 +864,15 @@ class IndexAndWorktree(RunWithEnvCwd):\n cmd = ['git', 'update-index', '--remove']\n self.run(cmd + ['-z', '--stdin']\n ).input_nulterm(paths).discard_output()\n+ def worktree_clean(self):\n+ \"\"\"Check whether the worktree is clean relative and no updates or\n+ merges are needed.\"\"\"\n+ try:\n+ self.run(['git', 'update-index', '--refresh']).discard_output()\n+ except run.RunException:\n+ return False\n+ else:\n+ return True\n\n class Branch(object):\n \"\"\"Represents a Git branch.\"\"\"\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex 54de127..8abf296 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -183,13 +183,22 @@ class StackTransaction(object):\n self.__checkout(self.__stack.head.data.tree, iw,\n allow_bad_head = True)\n def run(self, iw = None, set_head = True, allow_bad_head = False,\n- print_current_patch = True):\n+ print_current_patch = True, check_clean = False):\n \"\"\"Execute the transaction. Will either succeed, or fail (with an\n exception) and do nothing.\"\"\"\n self.__check_consistency()\n log.log_external_mods(self.__stack)\n new_head = self.head\n\n+ # Check for not clean index and worktree\n+ if check_clean and iw:\n+ if not iw.worktree_clean():\n+ self.__halt('Repository not clean. Use \"refresh\" or '\n+ '\"status --reset\"')\n+ elif not iw.index.is_clean()):\n+ self.__halt('Index and HEAD different. Use \"repair\" to '\n+ 'recover additional commits')\n+\n # Set branch head.\n if set_head:\n if iw:\n\n\nThanks.\n\n-- \nCatalin\n"},{"id":"102605","messageId":"20090130152649.GA22044@diana.vm.bytemark.co.uk","threadId":"17422","inReplyTo":"b0943d9e0901300601j27ab6ebdq4b38a9f7c0cbe261@mail.gmail.com","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-01-30T15:26:49Z","receivedAt":"2009-01-30T15:26:49Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-01-30 14:01:20 +0000, Catalin Marinas wrote:\n\n> @@ -706,9 +706,15 @@ class Index(RunWithEnv):\n> ).output_one_line())\n> except run.RunException:\n> raise MergeException('Conflicting merge')\n> - def is_clean(self):\n> + def is_clean(self, tree = None):\n> + \"\"\"Check whether the index is clean relative to the given tree.\"\"\"\n> + if tree:\n> + sha1 = tree.sha1\n> + else:\n> + sha1 = 'HEAD'\n> try:\n> - self.run(['git', 'update-index', '--refresh']).discard_output()\n> + self.run(['git', 'diff-index', '--quiet', '--cached', sha1]\n> + ).discard_output()\n> except run.RunException:\n> return False\n> else:\n\nOK (though I personally would have allowed only Tree objects, with no\ndefaulting to the current HEAD).\n\nThe docstring should say s/tree/treeish/.\n\n> @@ -858,6 +864,15 @@ class IndexAndWorktree(RunWithEnvCwd):\n> cmd = ['git', 'update-index', '--remove']\n> self.run(cmd + ['-z', '--stdin']\n> ).input_nulterm(paths).discard_output()\n> + def worktree_clean(self):\n> + \"\"\"Check whether the worktree is clean relative and no updates or\n> + merges are needed.\"\"\"\n> + try:\n> + self.run(['git', 'update-index', '--refresh']).discard_output()\n> + except run.RunException:\n> + return False\n> + else:\n> + return True\n\nClean relative to the index.\n\nAnd what do merges have to do with it?\n\n> + # Check for not clean index and worktree\n> + if check_clean and iw:\n> + if not iw.worktree_clean():\n> + self.__halt('Repository not clean. Use \"refresh\" or '\n> + '\"status --reset\"')\n> + elif not iw.index.is_clean()):\n> + self.__halt('Index and HEAD different. Use \"repair\" to '\n> + 'recover additional commits')\n\nThe first message is good, but the second one is misleading -- you'd\nbe much better off just reusing the same message as for the first\ncase. (You could recommend refresh --index if you want to get fancy.)\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"102631","messageId":"b0943d9e0901300936t4a6e0a37x1968a6949fb7bdda@mail.gmail.com","threadId":"17422","inReplyTo":"20090130152649.GA22044@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-01-30T17:36:57Z","receivedAt":"2009-01-30T17:36:57Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/1/30 Karl Hasselström :\n> On 2009-01-30 14:01:20 +0000, Catalin Marinas wrote:\n>\n>> @@ -706,9 +706,15 @@ class Index(RunWithEnv):\n>> ).output_one_line())\n>> except run.RunException:\n>> raise MergeException('Conflicting merge')\n>> - def is_clean(self):\n>> + def is_clean(self, tree = None):\n>> + \"\"\"Check whether the index is clean relative to the given tree.\"\"\"\n>> + if tree:\n>> + sha1 = tree.sha1\n>> + else:\n>> + sha1 = 'HEAD'\n>> try:\n>> - self.run(['git', 'update-index', '--refresh']).discard_output()\n>> + self.run(['git', 'diff-index', '--quiet', '--cached', sha1]\n>> + ).discard_output()\n>> except run.RunException:\n>> return False\n>> else:\n>\n> OK (though I personally would have allowed only Tree objects, with no\n> defaulting to the current HEAD).\n\nDone.\n\nSee below for an update. I added an __assert_index_worktree_clean()\nfunction in Transaction.\n\nNow, should we add the check_clean argument to Transaction.__init__()\nrather than run() as we do for the allow_bad_head case? The check\nwould need to be done in run() where we have an iw. Just a thought,\nI'm not convinced it is better.\n\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex e2b4266..07079b8 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -706,9 +706,11 @@ class Index(RunWithEnv):\n ).output_one_line())\n except run.RunException:\n raise MergeException('Conflicting merge')\n- def is_clean(self):\n+ def is_clean(self, tree):\n+ \"\"\"Check whether the index is clean relative to the given treeish.\"\"\"\n try:\n- self.run(['git', 'update-index', '--refresh']).discard_output()\n+ self.run(['git', 'diff-index', '--quiet', '--cached', tree.sha1]\n+ ).discard_output()\n except run.RunException:\n return False\n else:\n@@ -858,6 +860,14 @@ class IndexAndWorktree(RunWithEnvCwd):\n cmd = ['git', 'update-index', '--remove']\n self.run(cmd + ['-z', '--stdin']\n ).input_nulterm(paths).discard_output()\n+ def worktree_clean(self):\n+ \"\"\"Check whether the worktree is clean relative to index.\"\"\"\n+ try:\n+ self.run(['git', 'update-index', '--refresh']).discard_output()\n+ except run.RunException:\n+ return False\n+ else:\n+ return True\n\n class Branch(object):\n \"\"\"Represents a Git branch.\"\"\"\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex 54de127..c961222 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -147,6 +147,11 @@ class StackTransaction(object):\n 'This can happen if you modify a branch with git.',\n '\"stg repair --help\" explains more about what to do next.')\n self.__abort()\n+ def __assert_index_worktree_clean(self, iw):\n+ if not iw.worktree_clean() or \\\n+ not iw.index.is_clean(self.stack.head.data.tree):\n+ self.__halt('Repository not clean. Use \"refresh\" or '\n+ '\"status --reset\"')\n def __checkout(self, tree, iw, allow_bad_head):\n if not allow_bad_head:\n self.__assert_head_top_equal()\n@@ -183,13 +188,17 @@ class StackTransaction(object):\n self.__checkout(self.__stack.head.data.tree, iw,\n allow_bad_head = True)\n def run(self, iw = None, set_head = True, allow_bad_head = False,\n- print_current_patch = True):\n+ print_current_patch = True, check_clean = False):\n \"\"\"Execute the transaction. Will either succeed, or fail (with an\n exception) and do nothing.\"\"\"\n self.__check_consistency()\n log.log_external_mods(self.__stack)\n new_head = self.head\n\n+ # Check for clean index and worktree\n+ if check_clean and iw:\n+ self.__assert_index_worktree_clean(iw)\n+\n # Set branch head.\n if set_head:\n if iw:\n\n\n-- \nCatalin\n"},{"id":"103490","messageId":"b0943d9e0902060646hd779681x821e74d9a155d97b@mail.gmail.com","threadId":"17422","inReplyTo":"b0943d9e0901300936t4a6e0a37x1968a6949fb7bdda@mail.gmail.com","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-02-06T14:46:19Z","receivedAt":"2009-02-06T14:46:19Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/1/30 Catalin Marinas :\n> Now, should we add the check_clean argument to Transaction.__init__()\n> rather than run() as we do for the allow_bad_head case?\n\nIt looks like this may be a better option. The previous patch fails if\n\"goto\" pushes a patch with standard git-apply followed by another\npatch with a three-way merge. When Transaction.run() is called, even\nif the patch pushing succeeded, the function complains about local\nchanges because of the \"iw.index.is_clean(self.stack.head)\" check.\n\nIt is also a bit weird to push/pop patches and only complain at the\nend of local changes. Below is an updated patch which does the\nchecking in Transaction.__init__ (only the relevant parts of the\npatch):\n\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex e2b4266..07079b8 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -706,9 +706,11 @@ class Index(RunWithEnv):\n ).output_one_line())\n except run.RunException:\n raise MergeException('Conflicting merge')\n- def is_clean(self):\n+ def is_clean(self, tree):\n+ \"\"\"Check whether the index is clean relative to the given treeish.\"\"\"\n try:\n- self.run(['git', 'update-index', '--refresh']).discard_output()\n+ self.run(['git', 'diff-index', '--quiet', '--cached', tree.sha1]\n+ ).discard_output()\n except run.RunException:\n return False\n else:\n@@ -858,6 +860,14 @@ class IndexAndWorktree(RunWithEnvCwd):\n cmd = ['git', 'update-index', '--remove']\n self.run(cmd + ['-z', '--stdin']\n ).input_nulterm(paths).discard_output()\n+ def worktree_clean(self):\n+ \"\"\"Check whether the worktree is clean relative to index.\"\"\"\n+ try:\n+ self.run(['git', 'update-index', '--refresh']).discard_output()\n+ except run.RunException:\n+ return False\n+ else:\n+ return True\n\n class Branch(object):\n \"\"\"Represents a Git branch.\"\"\"\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex 54de127..e1bd38d 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -75,7 +75,8 @@ class StackTransaction(object):\n your refs and index+worktree, or fail without having done\n anything.\"\"\"\n def __init__(self, stack, msg, discard_changes = False,\n- allow_conflicts = False, allow_bad_head = False):\n+ allow_conflicts = False, allow_bad_head = False,\n+ check_clean = False):\n \"\"\"Create a new L{StackTransaction}.\n\n @param discard_changes: Discard any changes in index+worktree\n@@ -102,6 +103,8 @@ class StackTransaction(object):\n self.__temp_index = self.temp_index_tree = None\n if not allow_bad_head:\n self.__assert_head_top_equal()\n+ if check_clean:\n+ self.__assert_index_worktree_clean()\n stack = property(lambda self: self.__stack)\n patches = property(lambda self: self.__patches)\n def __set_applied(self, val):\n@@ -147,6 +150,12 @@ class StackTransaction(object):\n 'This can happen if you modify a branch with git.',\n '\"stg repair --help\" explains more about what to do next.')\n self.__abort()\n+ def __assert_index_worktree_clean(self):\n+ iw = self.__stack.repository.default_iw\n+ if not iw.worktree_clean() or \\\n+ not iw.index.is_clean(self.stack.head):\n+ self.__halt('Repository not clean. Use \"refresh\" or '\n+ '\"status --reset\"')\n def __checkout(self, tree, iw, allow_bad_head):\n if not allow_bad_head:\n self.__assert_head_top_equal()\n\n\n-- \nCatalin\n"},{"id":"103503","messageId":"20090206153106.GA28897@diana.vm.bytemark.co.uk","threadId":"17422","inReplyTo":"b0943d9e0902060646hd779681x821e74d9a155d97b@mail.gmail.com","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-02-06T15:31:06Z","receivedAt":"2009-02-06T15:31:06Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:\n\n> 2009/1/30 Catalin Marinas :\n>\n> > Now, should we add the check_clean argument to\n> > Transaction.__init__() rather than run() as we do for the\n> > allow_bad_head case?\n>\n> It looks like this may be a better option.\n\nSorry for taking so long to respond, but ... I strongly advise against\nusing default_iw in transaction.py. It's library code, and it should\ntake stuff like index and worktree as input parameters from layers\nthat are higher up in the abstraction stack. Compare the kernel policy\nof having policy in userspace and not in the kernel.\n\nAnd if you accept that reasoning, the check has to go in run() rather\nthan __init__(), because we don't have an iw in __init__(). (Though we\ncould add such a parameter, I guess.)\n\n> The previous patch fails if \"goto\" pushes a patch with standard\n> git-apply followed by another patch with a three-way merge. When\n> Transaction.run() is called, even if the patch pushing succeeded,\n> the function complains about local changes because of the\n> \"iw.index.is_clean(self.stack.head)\" check.\n\nHmm, so that would have to be worked around somehow ... I guess doing\nthe check in __init__() might make sense after all, since that's\nbefore we start changing things. How about adding a\ncheck_clean_relative_to paramter to __init__() that's not a boolean,\nbut an iw to check against? It would default to None, meaning no\ncheck.\n\n> It is also a bit weird to push/pop patches and only complain at the\n> end of local changes.\n\nYou mean the behavior the new infrastructure currently gives you? It's\nactually convenient in a number of cases. Assume for example that you\nhave patch A that changes file foo, patch B that changes file bar, and\nthen local changes to file bar. At this point you can pop A without\nproblem, even though a middle stage is to pop and push B which touches\nthe same file as your local changes -- the existing checks will only\ncompare the diff between the original and final tree with your local\nchanges, and that diff doesn't contain bar.\n\n> Below is an updated patch which does the checking in\n> Transaction.__init__ (only the relevant parts of the patch):\n\nLooks good, except for the hard-coded default_iw as I mentioned above.\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"103538","messageId":"b0943d9e0902061039g37eb521fl26d60d33c45a206@mail.gmail.com","threadId":"17422","inReplyTo":"20090206153106.GA28897@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Check for local changes with \"goto\"","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-02-06T18:39:47Z","receivedAt":"2009-02-06T18:39:47Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/2/6 Karl Hasselström :\n> On 2009-02-06 14:46:19 +0000, Catalin Marinas wrote:\n>\n>> 2009/1/30 Catalin Marinas :\n>>\n>> > Now, should we add the check_clean argument to\n>> > Transaction.__init__() rather than run() as we do for the\n>> > allow_bad_head case?\n>>\n>> It looks like this may be a better option.\n>\n> Sorry for taking so long to respond, but ... I strongly advise against\n> using default_iw in transaction.py. It's library code, and it should\n> take stuff like index and worktree as input parameters from layers\n> that are higher up in the abstraction stack.\n\nOK, no problem with that.\n\n>> The previous patch fails if \"goto\" pushes a patch with standard\n>> git-apply followed by another patch with a three-way merge. When\n>> Transaction.run() is called, even if the patch pushing succeeded,\n>> the function complains about local changes because of the\n>> \"iw.index.is_clean(self.stack.head)\" check.\n>\n> Hmm, so that would have to be worked around somehow ... I guess doing\n> the check in __init__() might make sense after all, since that's\n> before we start changing things. How about adding a\n> check_clean_relative_to paramter to __init__() that's not a boolean,\n> but an iw to check against? It would default to None, meaning no\n> check.\n\nOK, that's better.\n\n>> It is also a bit weird to push/pop patches and only complain at the\n>> end of local changes.\n>\n> You mean the behavior the new infrastructure currently gives you? It's\n> actually convenient in a number of cases. Assume for example that you\n> have patch A that changes file foo, patch B that changes file bar, and\n> then local changes to file bar. At this point you can pop A without\n> problem, even though a middle stage is to pop and push B which touches\n> the same file as your local changes -- the existing checks will only\n> compare the diff between the original and final tree with your local\n> changes, and that diff doesn't contain bar.\n\nYes, I agree that's a nice feature and it would still be available\nwith the --keep option (I wrote in the past why I wouldn't leave the\ncurrent behaviour to be the default).\n\nAbove I was referring to the new behaviour which checks for local\nchanges by default (--keep not passed). If we do the test in run() you\nmay push or pop patches and only fail at the end when actually the\noperation shouldn't have started. I plan to add the auto interactive\nmerging to the new infrastructure (by invoking mergetool if\nIndexAndWorktree.merge() fails, based on the stgit.autoimerge option)\nand pushing may become a more complex operation if enabled.\n\nI'll repost the patch. Thanks.\n\n-- \nCatalin\n"}]}