{"thread":{"id":"18435","subject":"[StGit PATCH] Add the --merged option to goto","startedAt":"2009-03-20T16:15:45Z","lastAt":"2009-03-31T07:27:01Z","messageCount":10,"participants":["Catalin Marinas","Karl Hasselström"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108730","messageId":"20090320161233.28989.82497.stgit@pc1117.cambridge.arm.com","threadId":"18435","inReplyTo":null,"subject":"[StGit PATCH] Add the --merged option to goto","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@arm.com","sentAt":"2009-03-20T16:15:45Z","receivedAt":"2009-03-20T16:15:45Z","isPatch":true,"sender":{"key":"catalin.marinas@arm.com","avatar":null},"body":"This patch adds support for checking which patches were already merged\nupstream. This checking is done by trying to reverse-apply the patches\nin the index before pushing them onto the stack. The trivial merge cases\nin Index.merge() are ignored when performing this operation otherwise\nthe results could be wrong (e.g. a patch adding a hunk and a subsequent\npatch canceling the previous change would both be considered merged).\n\nSigned-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n---\n\nThis is in preparation for the updating of the push command where we\nhave this functionality (I think we had it for goto as well but was lost\nwith the update to stgit.lib). Test cases with --merged are already done\nfor the push command, so I haven't added any for goto (but I'll push\nthis patch only after push is updated).\n\n stgit/argparse.py        |    4 ++++\n stgit/commands/goto.py   |   12 +++++++++---\n stgit/lib/git.py         |   15 ++++++++-------\n stgit/lib/transaction.py |   42 +++++++++++++++++++++++++++++++++++-------\n 4 files changed, 56 insertions(+), 17 deletions(-)\n\ndiff --git a/stgit/argparse.py b/stgit/argparse.py\nindex 85ee6e3..765579c 100644\n--- a/stgit/argparse.py\n+++ b/stgit/argparse.py\n@@ -225,6 +225,10 @@ def keep_option():\n                 short = 'Keep the local changes',\n                 default = config.get('stgit.autokeep') == 'yes')]\n \n+def merged_option():\n+    return [opt('-m', '--merged', action = 'store_true',\n+                short = 'Check for patches merged upstream')]\n+\n class CompgenBase(object):\n     def actions(self, var): return set()\n     def words(self, var): return set()\ndiff --git a/stgit/commands/goto.py b/stgit/commands/goto.py\nindex 66f49df..839b75c 100644\n--- a/stgit/commands/goto.py\n+++ b/stgit/commands/goto.py\n@@ -28,7 +28,7 @@ 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 = argparse.keep_option()\n+options = argparse.keep_option() + argparse.merged_option()\n \n directory = common.DirectoryHasRepositoryLib()\n \n@@ -47,8 +47,14 @@ def func(parser, options, args):\n         assert not trans.pop_patches(lambda pn: pn in to_pop)\n     elif patch in trans.unapplied:\n         try:\n-            for pn in trans.unapplied[:trans.unapplied.index(patch)+1]:\n-                trans.push_patch(pn, iw, allow_interactive = True)\n+            to_push = trans.unapplied[:trans.unapplied.index(patch)+1]\n+            if options.merged:\n+                merged = set(trans.check_merged(to_push))\n+            else:\n+                merged = set()\n+            for pn in to_push:\n+                trans.push_patch(pn, iw, allow_interactive = True,\n+                                 already_merged = pn in merged)\n         except transaction.TransactionHalted:\n             pass\n     elif patch in trans.hidden:\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex e0a3c96..875e352 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -732,7 +732,7 @@ class Index(RunWithEnv):\n         # to use --binary.\n         self.apply(self.__repository.diff_tree(tree1, tree2, ['--full-index']),\n                    quiet)\n-    def merge(self, base, ours, theirs, current = None):\n+    def merge(self, base, ours, theirs, current = None, check_trivial = True):\n         \"\"\"Use the index (and only the index) to do a 3-way merge of the\n         L{Tree}s C{base}, C{ours} and C{theirs}. The merge will either\n         succeed (in which case the first half of the return value is\n@@ -752,12 +752,13 @@ class Index(RunWithEnv):\n         assert current == None or isinstance(current, Tree)\n \n         # Take care of the really trivial cases.\n-        if base == ours:\n-            return (theirs, current)\n-        if base == theirs:\n-            return (ours, current)\n-        if ours == theirs:\n-            return (ours, current)\n+        if check_trivial:\n+            if base == ours:\n+                return (theirs, current)\n+            if base == theirs:\n+                return (ours, current)\n+            if ours == theirs:\n+                return (ours, current)\n \n         if current == theirs:\n             # Swap the trees. It doesn't matter since merging is\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex b146648..8cd5e50 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -297,7 +297,8 @@ class StackTransaction(object):\n                     out.info('Deleted %s%s' % (pn, s))\n         return popped\n \n-    def push_patch(self, pn, iw = None, allow_interactive = False):\n+    def push_patch(self, pn, iw = None, allow_interactive = False,\n+                   already_merged = False):\n         \"\"\"Attempt to push the named patch. If this results in conflicts,\n         halts the transaction. If index+worktree are given, spill any\n         conflicts to them.\"\"\"\n@@ -305,11 +306,14 @@ class StackTransaction(object):\n         cd = orig_cd.set_committer(None)\n         oldparent = cd.parent\n         cd = cd.set_parent(self.top)\n-        base = oldparent.data.tree\n-        ours = cd.parent.data.tree\n-        theirs = cd.tree\n-        tree, self.temp_index_tree = self.temp_index.merge(\n-            base, ours, theirs, self.temp_index_tree)\n+        if already_merged:\n+            tree = cd.tree\n+        else:\n+            base = oldparent.data.tree\n+            ours = cd.parent.data.tree\n+            theirs = cd.tree\n+            tree, self.temp_index_tree = self.temp_index.merge(\n+                base, ours, theirs, self.temp_index_tree)\n         s = ''\n         merge_conflict = False\n         if not tree:\n@@ -341,7 +345,9 @@ class StackTransaction(object):\n         else:\n             comm = None\n             s = ' (unmodified)'\n-        if not merge_conflict and cd.is_nochange():\n+        if already_merged:\n+            s = ' (merged)'\n+        elif not merge_conflict and cd.is_nochange():\n             s = ' (empty)'\n         out.info('Pushed %s%s' % (pn, s))\n         def update():\n@@ -379,3 +385,25 @@ class StackTransaction(object):\n         assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n         self.unapplied = unapplied\n         self.hidden = hidden\n+\n+    def check_merged(self, patches):\n+        \"\"\"Return a subset of patches already merged.\"\"\"\n+        merged = []\n+        temp_index = self.__stack.repository.temp_index()\n+        temp_index_tree = None\n+        ours = self.stack.head.data.tree\n+\n+        for pn in reversed(patches):\n+            # check whether patch changes can be reversed in the current tree\n+            cd = self.patches[pn].data\n+            base = cd.tree\n+            theirs = cd.parent.data.tree\n+            tree, temp_index_tree = \\\n+                    temp_index.merge(base, ours, theirs, temp_index_tree,\n+                                     check_trivial = False)\n+            if tree:\n+                merged.append(pn)\n+                ours = tree\n+\n+        temp_index.delete()\n+        return merged\n"},{"id":"109022","messageId":"20090323084507.GA6447@diana.vm.bytemark.co.uk","threadId":"18435","inReplyTo":"20090320161233.28989.82497.stgit@pc1117.cambridge.arm.com","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-03-23T08:45:07Z","receivedAt":"2009-03-23T08:45:07Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-03-20 16:15:45 +0000, Catalin Marinas wrote:\n\n> This patch adds support for checking which patches were already\n> merged upstream. This checking is done by trying to reverse-apply\n> the patches in the index before pushing them onto the stack. The\n> trivial merge cases in Index.merge() are ignored when performing\n> this operation otherwise the results could be wrong (e.g. a patch\n> adding a hunk and a subsequent patch canceling the previous change\n> would both be considered merged).\n>\n> Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n> ---\n>\n> This is in preparation for the updating of the push command where we\n> have this functionality (I think we had it for goto as well but was\n> lost with the update to stgit.lib). Test cases with --merged are\n> already done for the push command, so I haven't added any for goto\n> (but I'll push this patch only after push is updated).\n\nLooks good, except for a few things:\n\n> @@ -732,7 +732,7 @@ class Index(RunWithEnv):\n>          # to use --binary.\n>          self.apply(self.__repository.diff_tree(tree1, tree2, ['--full-index']),\n>                     quiet)\n> -    def merge(self, base, ours, theirs, current = None):\n> +    def merge(self, base, ours, theirs, current = None, check_trivial = True):\n>          \"\"\"Use the index (and only the index) to do a 3-way merge of the\n>          L{Tree}s C{base}, C{ours} and C{theirs}. The merge will either\n>          succeed (in which case the first half of the return value is\n\nPlease update the documentation with your new option. :-)\n\n> @@ -752,12 +752,13 @@ class Index(RunWithEnv):\n>          assert current == None or isinstance(current, Tree)\n>  \n>          # Take care of the really trivial cases.\n> -        if base == ours:\n> -            return (theirs, current)\n> -        if base == theirs:\n> -            return (ours, current)\n> -        if ours == theirs:\n> -            return (ours, current)\n> +        if check_trivial:\n> +            if base == ours:\n> +                return (theirs, current)\n> +            if base == theirs:\n> +                return (ours, current)\n> +            if ours == theirs:\n> +                return (ours, current)\n\nUh, what? What's the point of not doing this unconditionally?\n\n> @@ -379,3 +385,25 @@ class StackTransaction(object):\n>          assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n>          self.unapplied = unapplied\n>          self.hidden = hidden\n> +\n> +    def check_merged(self, patches):\n> +        \"\"\"Return a subset of patches already merged.\"\"\"\n> +        merged = []\n> +        temp_index = self.__stack.repository.temp_index()\n> +        temp_index_tree = None\n\nThere's no need to create a new temp index here. The transaction\nobject already has one.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"109086","messageId":"b0943d9e0903230933n5b71a53elcfaa13f00883861d@mail.gmail.com","threadId":"18435","inReplyTo":"20090323084507.GA6447@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-03-23T16:33:04Z","receivedAt":"2009-03-23T16:33:04Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/3/23 Karl Hasselström <kha@treskal.com>:\n> On 2009-03-20 16:15:45 +0000, Catalin Marinas wrote:\n>> @@ -752,12 +752,13 @@ class Index(RunWithEnv):\n>>          assert current == None or isinstance(current, Tree)\n>>\n>>          # Take care of the really trivial cases.\n>> -        if base == ours:\n>> -            return (theirs, current)\n>> -        if base == theirs:\n>> -            return (ours, current)\n>> -        if ours == theirs:\n>> -            return (ours, current)\n>> +        if check_trivial:\n>> +            if base == ours:\n>> +                return (theirs, current)\n>> +            if base == theirs:\n>> +                return (ours, current)\n>> +            if ours == theirs:\n>> +                return (ours, current)\n>\n> Uh, what? What's the point of not doing this unconditionally?\n\nThere are a few cases where my algorithm failed because the reverse\napplying of patches fell on one of those special cases (otherwise they\nwouldn't apply). The check_merged() function assumes that if a patch\ncan be reversed in a given tree, it was already included in that tree.\n\nLet's assume that the tree corresponding to the top patch is T1. We\nhave the following cases for reverse-applying a patch which fall under\nthe trivial cases above (patch expressed as bottom_tree..top_tree):\n\nThe empty patch cases should be ignored from such test (not done currently):\n\nT1..T1 => merge(T1, T1, T1) == T1\nT2..T2 => merge(T2, T1, T2) == T1\n\nThe non-empty patch situations:\n\nT1..T2 => merge(T2, T1, T1) == T1\nT2..T1 => merge(T1, T1, T2) == T2\n\nThe T1..T2 is pretty common and happens when the base of a patch\nwasn't modified. Reverse-applying such patch should not normally\nsucceed but the merge() here uses one of those special cases. The\nmerge() result is correct since we want two trees merged, T1 and T1,\nwith a common base, T2, used a helper.\n\nThe T2..T1 cases would succeed with both trivial checks and\napply_treediff() and that's probably OK since if a patch generates the\nsame tree when applied, the changes it makes were probably already\nincluded.\n\nNow I understand it better :-). Reading my explanation above, it seems\nthat only the T1..T2 case matters and it can be taken care of in the\ncheck_merged() function. Checking whether the tree returned by merge()\nis different than \"ours\" should be enough for all the above cases.\n\n>> @@ -379,3 +385,25 @@ class StackTransaction(object):\n>>          assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n>>          self.unapplied = unapplied\n>>          self.hidden = hidden\n>> +\n>> +    def check_merged(self, patches):\n>> +        \"\"\"Return a subset of patches already merged.\"\"\"\n>> +        merged = []\n>> +        temp_index = self.__stack.repository.temp_index()\n>> +        temp_index_tree = None\n>\n> There's no need to create a new temp index here. The transaction\n> object already has one.\n\nI had the impression that an Index object would hold some state and\ndidn't want to break it. It seems OK to use as long as I don't touch\nself.temp_index_tree. See below for an updated patch:\n\n\nAdd the --merged option to goto\n\nFrom: Catalin Marinas <catalin.marinas@gmail.com>\n\nThis patch adds support for checking which patches were already merged\nupstream. This checking is done by trying to reverse-apply the patches\nin the index before pushing them onto the stack.\n\nSigned-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n---\n stgit/argparse.py        |    4 ++++\n stgit/commands/goto.py   |   12 +++++++++---\n stgit/lib/git.py         |    2 +-\n stgit/lib/transaction.py |   38 +++++++++++++++++++++++++++++++-------\n 4 files changed, 45 insertions(+), 11 deletions(-)\n\ndiff --git a/stgit/argparse.py b/stgit/argparse.py\nindex 85ee6e3..765579c 100644\n--- a/stgit/argparse.py\n+++ b/stgit/argparse.py\n@@ -225,6 +225,10 @@ def keep_option():\n                 short = 'Keep the local changes',\n                 default = config.get('stgit.autokeep') == 'yes')]\n\n+def merged_option():\n+    return [opt('-m', '--merged', action = 'store_true',\n+                short = 'Check for patches merged upstream')]\n+\n class CompgenBase(object):\n     def actions(self, var): return set()\n     def words(self, var): return set()\ndiff --git a/stgit/commands/goto.py b/stgit/commands/goto.py\nindex 66f49df..839b75c 100644\n--- a/stgit/commands/goto.py\n+++ b/stgit/commands/goto.py\n@@ -28,7 +28,7 @@ Push/pop patches to/from the stack until the one\ngiven on the command\n line becomes current.\"\"\"\n\n args = [argparse.other_applied_patches, argparse.unapplied_patches]\n-options = argparse.keep_option()\n+options = argparse.keep_option() + argparse.merged_option()\n\n directory = common.DirectoryHasRepositoryLib()\n\n@@ -47,8 +47,14 @@ def func(parser, options, args):\n         assert not trans.pop_patches(lambda pn: pn in to_pop)\n     elif patch in trans.unapplied:\n         try:\n-            for pn in trans.unapplied[:trans.unapplied.index(patch)+1]:\n-                trans.push_patch(pn, iw, allow_interactive = True)\n+            to_push = trans.unapplied[:trans.unapplied.index(patch)+1]\n+            if options.merged:\n+                merged = set(trans.check_merged(to_push))\n+            else:\n+                merged = set()\n+            for pn in to_push:\n+                trans.push_patch(pn, iw, allow_interactive = True,\n+                                 already_merged = pn in merged)\n         except transaction.TransactionHalted:\n             pass\n     elif patch in trans.hidden:\ndiff --git a/stgit/lib/git.py b/stgit/lib/git.py\nindex e0a3c96..fcac918 100644\n--- a/stgit/lib/git.py\n+++ b/stgit/lib/git.py\n@@ -732,7 +732,7 @@ class Index(RunWithEnv):\n         # to use --binary.\n         self.apply(self.__repository.diff_tree(tree1, tree2, ['--full-index']),\n                    quiet)\n-    def merge(self, base, ours, theirs, current = None):\n+    def merge(self, base, ours, theirs, current = None, check_trivial = True):\n         \"\"\"Use the index (and only the index) to do a 3-way merge of the\n         L{Tree}s C{base}, C{ours} and C{theirs}. The merge will either\n         succeed (in which case the first half of the return value is\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex b146648..9fa75c1 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -297,7 +297,8 @@ class StackTransaction(object):\n                     out.info('Deleted %s%s' % (pn, s))\n         return popped\n\n-    def push_patch(self, pn, iw = None, allow_interactive = False):\n+    def push_patch(self, pn, iw = None, allow_interactive = False,\n+                   already_merged = False):\n         \"\"\"Attempt to push the named patch. If this results in conflicts,\n         halts the transaction. If index+worktree are given, spill any\n         conflicts to them.\"\"\"\n@@ -305,11 +306,14 @@ class StackTransaction(object):\n         cd = orig_cd.set_committer(None)\n         oldparent = cd.parent\n         cd = cd.set_parent(self.top)\n-        base = oldparent.data.tree\n-        ours = cd.parent.data.tree\n-        theirs = cd.tree\n-        tree, self.temp_index_tree = self.temp_index.merge(\n-            base, ours, theirs, self.temp_index_tree)\n+        if already_merged:\n+            tree = cd.tree\n+        else:\n+            base = oldparent.data.tree\n+            ours = cd.parent.data.tree\n+            theirs = cd.tree\n+            tree, self.temp_index_tree = self.temp_index.merge(\n+                base, ours, theirs, self.temp_index_tree)\n         s = ''\n         merge_conflict = False\n         if not tree:\n@@ -341,7 +345,9 @@ class StackTransaction(object):\n         else:\n             comm = None\n             s = ' (unmodified)'\n-        if not merge_conflict and cd.is_nochange():\n+        if already_merged:\n+            s = ' (merged)'\n+        elif not merge_conflict and cd.is_nochange():\n             s = ' (empty)'\n         out.info('Pushed %s%s' % (pn, s))\n         def update():\n@@ -379,3 +385,21 @@ class StackTransaction(object):\n         assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n         self.unapplied = unapplied\n         self.hidden = hidden\n+\n+    def check_merged(self, patches):\n+        \"\"\"Return a subset of patches already merged.\"\"\"\n+        merged = []\n+        temp_index_tree = None\n+        ours = self.stack.head.data.tree\n+        for pn in reversed(patches):\n+            # check whether patch changes can be reversed in the current tree\n+            cd = self.patches[pn].data\n+            base = cd.tree\n+            theirs = cd.parent.data.tree\n+            tree, temp_index_tree = \\\n+                    self.temp_index.merge(base, ours, theirs, temp_index_tree,\n+                                          check_trivial = False)\n+            if tree and tree != ours:\n+                merged.append(pn)\n+                ours = tree\n+        return merged\n\n\n-- \nCatalin\n"},{"id":"109209","messageId":"20090324131640.GB4040@diana.vm.bytemark.co.uk","threadId":"18435","inReplyTo":"b0943d9e0903230933n5b71a53elcfaa13f00883861d@mail.gmail.com","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-03-24T13:16:40Z","receivedAt":"2009-03-24T13:16:40Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-03-23 16:33:04 +0000, Catalin Marinas wrote:\n\n> 2009/3/23 Karl Hasselström <kha@treskal.com>:\n>\n> > On 2009-03-20 16:15:45 +0000, Catalin Marinas wrote:\n> >> @@ -752,12 +752,13 @@ class Index(RunWithEnv):\n> >>          assert current == None or isinstance(current, Tree)\n> >>\n> >>          # Take care of the really trivial cases.\n> >> -        if base == ours:\n> >> -            return (theirs, current)\n> >> -        if base == theirs:\n> >> -            return (ours, current)\n> >> -        if ours == theirs:\n> >> -            return (ours, current)\n> >> +        if check_trivial:\n> >> +            if base == ours:\n> >> +                return (theirs, current)\n> >> +            if base == theirs:\n> >> +                return (ours, current)\n> >> +            if ours == theirs:\n> >> +                return (ours, current)\n> >\n> > Uh, what? What's the point of not doing this unconditionally?\n>\n> There are a few cases where my algorithm failed because the reverse\n> applying of patches fell on one of those special cases (otherwise\n> they wouldn't apply). The check_merged() function assumes that if a\n> patch can be reversed in a given tree, it was already included in\n> that tree.\n>\n> Let's assume that the tree corresponding to the top patch is T1. We\n> have the following cases for reverse-applying a patch which fall\n> under the trivial cases above (patch expressed as\n> bottom_tree..top_tree):\n>\n> The empty patch cases should be ignored from such test (not done\n> currently):\n>\n> T1..T1 => merge(T1, T1, T1) == T1\n> T2..T2 => merge(T2, T1, T2) == T1\n>\n> The non-empty patch situations:\n>\n> T1..T2 => merge(T2, T1, T1) == T1\n> T2..T1 => merge(T1, T1, T2) == T2\n>\n> The T1..T2 is pretty common and happens when the base of a patch\n> wasn't modified. Reverse-applying such patch should not normally\n> succeed but the merge() here uses one of those special cases. The\n> merge() result is correct since we want two trees merged, T1 and T1,\n> with a common base, T2, used a helper.\n>\n> The T2..T1 cases would succeed with both trivial checks and\n> apply_treediff() and that's probably OK since if a patch generates\n> the same tree when applied, the changes it makes were probably\n> already included.\n>\n> Now I understand it better :-). Reading my explanation above, it\n> seems that only the T1..T2 case matters and it can be taken care of\n> in the check_merged() function. Checking whether the tree returned\n> by merge() is different than \"ours\" should be enough for all the\n> above cases.\n\nHmm. If the tip of the branch is T1, and we reverse-apply the patch\nT1..T2, we get the merge (base T2, ours T1, theirs T1) ... yeah, I see\nwhat you mean. The problem isn't that we give T1 as the result of this\nmerge -- that's actually the right thing to do -- the problem is that\nyou don't actually want a merge. What you want is patch application.\nMaybe the apply_treediff method would do? See my other comment below.\n\n> >> @@ -379,3 +385,25 @@ class StackTransaction(object):\n> >>          assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n> >>          self.unapplied = unapplied\n> >>          self.hidden = hidden\n> >> +\n> >> +    def check_merged(self, patches):\n> >> +        \"\"\"Return a subset of patches already merged.\"\"\"\n> >> +        merged = []\n> >> +        temp_index = self.__stack.repository.temp_index()\n> >> +        temp_index_tree = None\n> >\n> > There's no need to create a new temp index here. The transaction\n> > object already has one.\n>\n> I had the impression that an Index object would hold some state and\n> didn't want to break it. It seems OK to use as long as I don't touch\n> self.temp_index_tree. See below for an updated patch:\n\nYes, an Index object owns a git index file.\n\nAnd no, not quite. temp_index_tree is set to the tree we know is\nstored in temp_index right now (or None if we don't know). The idea is\nthat we'll often want to read a tree into the index that's already\nthere, and by keeping track of this we'll get better performance.\n(This works very well in practice.) Apologies if there aren't comments\nexplaining this ... the merge method has some docs on the subject.\n\nI think what you should do is something like what merge() does:\n\n  if temp_index_tree != branch_tip:\n      temp_index.read_tree(branch_tip)\n      temp_index_tree = branch_tip\n  try:\n      temp_index.apply_treediff(patch_bottom, patch_top, quiet = True)\n      temp_index_tree = temp_index.write_tree()\n      return True\n  except MergeException:\n      return False\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"109220","messageId":"b0943d9e0903240840m3f22b702qd48293caad4187e3@mail.gmail.com","threadId":"18435","inReplyTo":"20090324131640.GB4040@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-03-24T15:40:10Z","receivedAt":"2009-03-24T15:40:10Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/3/24 Karl Hasselström <kha@treskal.com>:\n> On 2009-03-23 16:33:04 +0000, Catalin Marinas wrote:\n>> Now I understand it better :-). Reading my explanation above, it\n>> seems that only the T1..T2 case matters and it can be taken care of\n>> in the check_merged() function. Checking whether the tree returned\n>> by merge() is different than \"ours\" should be enough for all the\n>> above cases.\n>\n> Hmm. If the tip of the branch is T1, and we reverse-apply the patch\n> T1..T2, we get the merge (base T2, ours T1, theirs T1) ... yeah, I see\n> what you mean. The problem isn't that we give T1 as the result of this\n> merge -- that's actually the right thing to do -- the problem is that\n> you don't actually want a merge. What you want is patch application.\n> Maybe the apply_treediff method would do?\n\nYes, see below for an updated patch.\n\n>> I had the impression that an Index object would hold some state and\n>> didn't want to break it. It seems OK to use as long as I don't touch\n>> self.temp_index_tree. See below for an updated patch:\n>\n> Yes, an Index object owns a git index file.\n>\n> And no, not quite. temp_index_tree is set to the tree we know is\n> stored in temp_index right now (or None if we don't know). The idea is\n> that we'll often want to read a tree into the index that's already\n> there, and by keeping track of this we'll get better performance.\n> (This works very well in practice.) Apologies if there aren't comments\n> explaining this ... the merge method has some docs on the subject.\n\nI figured this out eventually. Anyway, with apply_treedif() there is\nno need for the temp_index_tree. In the updated patch below, I don't\neven call write_tree() as it isn't needed (to my understanding).\nWhatever is in the index after the check_merged() function doesn't\ncorrespond to any tree so it would need to be re-read.\n\nBTW, I implemented push and pop and it now passes all the \"push\n--merged\" tests. I had to correct the \"already_merged\" case in\nTransaction.push_patch() to generate an empty patch.\n\n    Add the --merged option to goto\n\n    This patch adds support for checking which patches were already merged\n    upstream. This checking is done by trying to reverse-apply the patches\n    in the index before pushing them onto the stack.\n\n    Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n\ndiff --git a/stgit/argparse.py b/stgit/argparse.py\nindex 85ee6e3..765579c 100644\n--- a/stgit/argparse.py\n+++ b/stgit/argparse.py\n@@ -225,6 +225,10 @@ def keep_option():\n                 short = 'Keep the local changes',\n                 default = config.get('stgit.autokeep') == 'yes')]\n\n+def merged_option():\n+    return [opt('-m', '--merged', action = 'store_true',\n+                short = 'Check for patches merged upstream')]\n+\n class CompgenBase(object):\n     def actions(self, var): return set()\n     def words(self, var): return set()\ndiff --git a/stgit/commands/goto.py b/stgit/commands/goto.py\nindex 66f49df..839b75c 100644\n--- a/stgit/commands/goto.py\n+++ b/stgit/commands/goto.py\n@@ -28,7 +28,7 @@ Push/pop patches to/from the stack until the one\ngiven on the command\n line becomes current.\"\"\"\n\n args = [argparse.other_applied_patches, argparse.unapplied_patches]\n-options = argparse.keep_option()\n+options = argparse.keep_option() + argparse.merged_option()\n\n directory = common.DirectoryHasRepositoryLib()\n\n@@ -47,8 +47,14 @@ def func(parser, options, args):\n         assert not trans.pop_patches(lambda pn: pn in to_pop)\n     elif patch in trans.unapplied:\n         try:\n-            for pn in trans.unapplied[:trans.unapplied.index(patch)+1]:\n-                trans.push_patch(pn, iw, allow_interactive = True)\n+            to_push = trans.unapplied[:trans.unapplied.index(patch)+1]\n+            if options.merged:\n+                merged = set(trans.check_merged(to_push))\n+            else:\n+                merged = set()\n+            for pn in to_push:\n+                trans.push_patch(pn, iw, allow_interactive = True,\n+                                 already_merged = pn in merged)\n         except transaction.TransactionHalted:\n             pass\n     elif patch in trans.hidden:\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex b146648..e2d9b78 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -297,7 +297,8 @@ class StackTransaction(object):\n                     out.info('Deleted %s%s' % (pn, s))\n         return popped\n\n-    def push_patch(self, pn, iw = None, allow_interactive = False):\n+    def push_patch(self, pn, iw = None, allow_interactive = False,\n+                   already_merged = False):\n         \"\"\"Attempt to push the named patch. If this results in conflicts,\n         halts the transaction. If index+worktree are given, spill any\n         conflicts to them.\"\"\"\n@@ -305,11 +306,15 @@ class StackTransaction(object):\n         cd = orig_cd.set_committer(None)\n         oldparent = cd.parent\n         cd = cd.set_parent(self.top)\n-        base = oldparent.data.tree\n-        ours = cd.parent.data.tree\n-        theirs = cd.tree\n-        tree, self.temp_index_tree = self.temp_index.merge(\n-            base, ours, theirs, self.temp_index_tree)\n+        if already_merged:\n+            # the resulting patch is empty\n+            tree = cd.parent.data.tree\n+        else:\n+            base = oldparent.data.tree\n+            ours = cd.parent.data.tree\n+            theirs = cd.tree\n+            tree, self.temp_index_tree = self.temp_index.merge(\n+                base, ours, theirs, self.temp_index_tree)\n         s = ''\n         merge_conflict = False\n         if not tree:\n@@ -341,7 +346,9 @@ class StackTransaction(object):\n         else:\n             comm = None\n             s = ' (unmodified)'\n-        if not merge_conflict and cd.is_nochange():\n+        if already_merged:\n+            s = ' (merged)'\n+        elif not merge_conflict and cd.is_nochange():\n             s = ' (empty)'\n         out.info('Pushed %s%s' % (pn, s))\n         def update():\n@@ -379,3 +386,23 @@ class StackTransaction(object):\n         assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n         self.unapplied = unapplied\n         self.hidden = hidden\n+\n+    def check_merged(self, patches):\n+        \"\"\"Return a subset of patches already merged.\"\"\"\n+        merged = []\n+        self.temp_index.read_tree(self.stack.head.data.tree)\n+        # The self.temp_index is modified by apply_treediff() so force\n+        # read_tree() the next time merge() is used.\n+        self.temp_index_tree = None\n+        for pn in reversed(patches):\n+            # check whether patch changes can be reversed in the current index\n+            cd = self.patches[pn].data\n+            if cd.is_nochange():\n+                continue\n+            try:\n+                self.temp_index.apply_treediff(cd.tree, cd.parent.data.tree,\n+                                               quiet = True)\n+                merged.append(pn)\n+            except git.MergeException:\n+                pass\n+        return merged\n\n\n\nThanks.\n\n-- \nCatalin\n"},{"id":"109328","messageId":"20090325090541.GA24889@diana.vm.bytemark.co.uk","threadId":"18435","inReplyTo":"b0943d9e0903240840m3f22b702qd48293caad4187e3@mail.gmail.com","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-03-25T09:05:41Z","receivedAt":"2009-03-25T09:05:41Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-03-24 15:40:10 +0000, Catalin Marinas wrote:\n\n> Anyway, with apply_treedif() there is no need for the\n> temp_index_tree. In the updated patch below, I don't even call\n> write_tree() as it isn't needed (to my understanding).\n\nYes. You're not interested in the result of the patch application, you\njust want to know if it succeeded or not.\n\n> Whatever is in the index after the check_merged() function doesn't\n> correspond to any tree so it would need to be re-read.\n\nIt does correspond to _some_ tree, but as we have no particular reason\nto expect that it might be a tree that we're going to need again\nimmediately, we can set temp_index_tree to None to force re-reading,\nrather than pay the cost of write_tree.\n\nWhen pushing patches, on the other hand, we have good reasons to\nbelieve that the next tree we'll need in the index is the same (or at\nleast very close to) the one produced in the last merge. Consider the\ncase of popping a few patches, changing the message on the top patch,\nand then pushing the patches back, for example.\n\n> +    def check_merged(self, patches):\n> +        \"\"\"Return a subset of patches already merged.\"\"\"\n> +        merged = []\n> +        self.temp_index.read_tree(self.stack.head.data.tree)\n> +        # The self.temp_index is modified by apply_treediff() so force\n> +        # read_tree() the next time merge() is used.\n> +        self.temp_index_tree = None\n> +        for pn in reversed(patches):\n> +            # check whether patch changes can be reversed in the current index\n> +            cd = self.patches[pn].data\n> +            if cd.is_nochange():\n> +                continue\n> +            try:\n> +                self.temp_index.apply_treediff(cd.tree, cd.parent.data.tree,\n> +                                               quiet = True)\n> +                merged.append(pn)\n> +            except git.MergeException:\n> +                pass\n> +        return merged\n\nSome points here:\n\n  1. Check if temp_index_tree already contains the tree you want\n     instead of doing read_tree unconditionally. That is, change\n\n       self.temp_index.read_tree(self.stack.head.data.tree)\n\n     to\n\n       if self.stack.head.data.tree != self.temp_index_tree:\n           self.temp_index.read_tree(self.stack.head.data.tree)\n           self.temp_index_tree = self.stack.head.data.tree\n\n  2. If apply_treediff fails (raises MergeException), the index wasn't\n     modified at all (but I guess you knew that, since your code\n     relies on that fact), so there's no reason to set temp_index_tree\n     to None in that case. So in order to not clear temp_index_tree\n     unnecessarily in case none of the patches reverse-apply (a case I\n     personally encounter frequently, since I leave the -m flag on all\n     the time), move\n\n       self.temp_index_tree = None\n\n     to just after (or just before) \"merged.append(pn)\".\n\n  3. Why are empty patches considered not merged?\n\nOther than that, this logic looks fine.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"109332","messageId":"b0943d9e0903250324j9ed0ed9k2d97cbacba6a7801@mail.gmail.com","threadId":"18435","inReplyTo":"20090325090541.GA24889@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-03-25T10:24:13Z","receivedAt":"2009-03-25T10:24:13Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/3/25 Karl Hasselström <kha@treskal.com>:\n> On 2009-03-24 15:40:10 +0000, Catalin Marinas wrote:\n>> Whatever is in the index after the check_merged() function doesn't\n>> correspond to any tree so it would need to be re-read.\n>\n> It does correspond to _some_ tree, but as we have no particular reason\n> to expect that it might be a tree that we're going to need again\n> immediately, we can set temp_index_tree to None to force re-reading,\n> rather than pay the cost of write_tree.\n\nYes, that's what I meant by \"doesn't correspond to any tree\".\n\nBTW, why don't we keep the tree information directly in the Index\nobject? Since this object is modified only via its own interface, it\ncan do all the checks and avoid the managing of temp_index_tree in the\nTransaction object.\n\n>> +    def check_merged(self, patches):\n>> +        \"\"\"Return a subset of patches already merged.\"\"\"\n>> +        merged = []\n>> +        self.temp_index.read_tree(self.stack.head.data.tree)\n>> +        # The self.temp_index is modified by apply_treediff() so force\n>> +        # read_tree() the next time merge() is used.\n>> +        self.temp_index_tree = None\n>> +        for pn in reversed(patches):\n>> +            # check whether patch changes can be reversed in the current index\n>> +            cd = self.patches[pn].data\n>> +            if cd.is_nochange():\n>> +                continue\n>> +            try:\n>> +                self.temp_index.apply_treediff(cd.tree, cd.parent.data.tree,\n>> +                                               quiet = True)\n>> +                merged.append(pn)\n>> +            except git.MergeException:\n>> +                pass\n>> +        return merged\n>\n> Some points here:\n>\n>  1. Check if temp_index_tree already contains the tree you want\n>     instead of doing read_tree unconditionally. That is, change\n>\n>       self.temp_index.read_tree(self.stack.head.data.tree)\n>\n>     to\n>\n>       if self.stack.head.data.tree != self.temp_index_tree:\n>           self.temp_index.read_tree(self.stack.head.data.tree)\n>           self.temp_index_tree = self.stack.head.data.tree\n>\n>  2. If apply_treediff fails (raises MergeException), the index wasn't\n>     modified at all (but I guess you knew that, since your code\n>     relies on that fact), so there's no reason to set temp_index_tree\n>     to None in that case. So in order to not clear temp_index_tree\n>     unnecessarily in case none of the patches reverse-apply (a case I\n>     personally encounter frequently, since I leave the -m flag on all\n>     the time), move\n>\n>       self.temp_index_tree = None\n>\n>     to just after (or just before) \"merged.append(pn)\".\n\nYes. But it may be even better to do this in Index.\nIndex.apply_treediff() would set the tree to None and read_tree or\nwrite_tree would set it to the corresponding tree.\n\n>  3. Why are empty patches considered not merged?\n\nThey would be reported as empty anyway and in general you don't submit\nempty patches for upstream merging.\n\n-- \nCatalin\n"},{"id":"109522","messageId":"20090326111554.GA19337@diana.vm.bytemark.co.uk","threadId":"18435","inReplyTo":"b0943d9e0903250324j9ed0ed9k2d97cbacba6a7801@mail.gmail.com","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-03-26T11:15:54Z","receivedAt":"2009-03-26T11:15:54Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-03-25 10:24:13 +0000, Catalin Marinas wrote:\n\n> BTW, why don't we keep the tree information directly in the Index\n> object? Since this object is modified only via its own interface, it\n> can do all the checks and avoid the managing of temp_index_tree in\n> the Transaction object.\n\nI guess that might be a good idea -- it should be doable without any\nextra overhead for users that don't want it.\n\n> Yes. But it may be even better to do this in Index.\n> Index.apply_treediff() would set the tree to None and read_tree or\n> write_tree would set it to the corresponding tree.\n\nWe'd have to cover the other index operations too. But yes, this is\nprobably a good idea.\n\n> >  3. Why are empty patches considered not merged?\n>\n> They would be reported as empty anyway and in general you don't\n> submit empty patches for upstream merging.\n\nAh, duh. I was forgetting what the \"merged\" detection was for in the\nfirst place.\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"109919","messageId":"b0943d9e0903300901i469bd899v3518b43c331bd9df@mail.gmail.com","threadId":"18435","inReplyTo":"20090326111554.GA19337@diana.vm.bytemark.co.uk","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Catalin Marinas","fromEmail":"catalin.marinas@gmail.com","sentAt":"2009-03-30T16:01:12Z","receivedAt":"2009-03-30T16:01:12Z","isPatch":true,"sender":{"key":"catalin.marinas@gmail.com","avatar":null},"body":"2009/3/26 Karl Hasselström <kha@treskal.com>:\n> On 2009-03-25 10:24:13 +0000, Catalin Marinas wrote:\n>\n>> BTW, why don't we keep the tree information directly in the Index\n>> object? Since this object is modified only via its own interface, it\n>> can do all the checks and avoid the managing of temp_index_tree in\n>> the Transaction object.\n>\n> I guess that might be a good idea -- it should be doable without any\n> extra overhead for users that don't want it.\n\nI tried but gave up quickly. The IndexAndWorktree class also dirties\nthe Index with the merge operations, so it is not worth the hassle.\n\nThat's the updated patch:\n\n    Add the --merged option to goto\n\n    This patch adds support for checking which patches were already merged\n    upstream. This checking is done by trying to reverse-apply the patches\n    in the index before pushing them onto the stack.\n\n    Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>\n\ndiff --git a/stgit/argparse.py b/stgit/argparse.py\nindex 85ee6e3..765579c 100644\n--- a/stgit/argparse.py\n+++ b/stgit/argparse.py\n@@ -225,6 +225,10 @@ def keep_option():\n                 short = 'Keep the local changes',\n                 default = config.get('stgit.autokeep') == 'yes')]\n\n+def merged_option():\n+    return [opt('-m', '--merged', action = 'store_true',\n+                short = 'Check for patches merged upstream')]\n+\n class CompgenBase(object):\n     def actions(self, var): return set()\n     def words(self, var): return set()\ndiff --git a/stgit/commands/goto.py b/stgit/commands/goto.py\nindex 66f49df..839b75c 100644\n--- a/stgit/commands/goto.py\n+++ b/stgit/commands/goto.py\n@@ -28,7 +28,7 @@ Push/pop patches to/from the stack until the one\ngiven on the command\n line becomes current.\"\"\"\n\n args = [argparse.other_applied_patches, argparse.unapplied_patches]\n-options = argparse.keep_option()\n+options = argparse.keep_option() + argparse.merged_option()\n\n directory = common.DirectoryHasRepositoryLib()\n\n@@ -47,8 +47,14 @@ def func(parser, options, args):\n         assert not trans.pop_patches(lambda pn: pn in to_pop)\n     elif patch in trans.unapplied:\n         try:\n-            for pn in trans.unapplied[:trans.unapplied.index(patch)+1]:\n-                trans.push_patch(pn, iw, allow_interactive = True)\n+            to_push = trans.unapplied[:trans.unapplied.index(patch)+1]\n+            if options.merged:\n+                merged = set(trans.check_merged(to_push))\n+            else:\n+                merged = set()\n+            for pn in to_push:\n+                trans.push_patch(pn, iw, allow_interactive = True,\n+                                 already_merged = pn in merged)\n         except transaction.TransactionHalted:\n             pass\n     elif patch in trans.hidden:\ndiff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py\nindex b146648..4148ff3 100644\n--- a/stgit/lib/transaction.py\n+++ b/stgit/lib/transaction.py\n@@ -297,7 +297,8 @@ class StackTransaction(object):\n                     out.info('Deleted %s%s' % (pn, s))\n         return popped\n\n-    def push_patch(self, pn, iw = None, allow_interactive = False):\n+    def push_patch(self, pn, iw = None, allow_interactive = False,\n+                   already_merged = False):\n         \"\"\"Attempt to push the named patch. If this results in conflicts,\n         halts the transaction. If index+worktree are given, spill any\n         conflicts to them.\"\"\"\n@@ -305,11 +306,15 @@ class StackTransaction(object):\n         cd = orig_cd.set_committer(None)\n         oldparent = cd.parent\n         cd = cd.set_parent(self.top)\n-        base = oldparent.data.tree\n-        ours = cd.parent.data.tree\n-        theirs = cd.tree\n-        tree, self.temp_index_tree = self.temp_index.merge(\n-            base, ours, theirs, self.temp_index_tree)\n+        if already_merged:\n+            # the resulting patch is empty\n+            tree = cd.parent.data.tree\n+        else:\n+            base = oldparent.data.tree\n+            ours = cd.parent.data.tree\n+            theirs = cd.tree\n+            tree, self.temp_index_tree = self.temp_index.merge(\n+                base, ours, theirs, self.temp_index_tree)\n         s = ''\n         merge_conflict = False\n         if not tree:\n@@ -341,7 +346,9 @@ class StackTransaction(object):\n         else:\n             comm = None\n             s = ' (unmodified)'\n-        if not merge_conflict and cd.is_nochange():\n+        if already_merged:\n+            s = ' (merged)'\n+        elif not merge_conflict and cd.is_nochange():\n             s = ' (empty)'\n         out.info('Pushed %s%s' % (pn, s))\n         def update():\n@@ -379,3 +386,25 @@ class StackTransaction(object):\n         assert set(self.unapplied + self.hidden) == set(unapplied + hidden)\n         self.unapplied = unapplied\n         self.hidden = hidden\n+\n+    def check_merged(self, patches):\n+        \"\"\"Return a subset of patches already merged.\"\"\"\n+        merged = []\n+        if self.temp_index_tree != self.stack.head.data.tree:\n+            self.temp_index.read_tree(self.stack.head.data.tree)\n+            self.temp_index_tree = self.stack.head.data.tree\n+        for pn in reversed(patches):\n+            # check whether patch changes can be reversed in the current index\n+            cd = self.patches[pn].data\n+            if cd.is_nochange():\n+                continue\n+            try:\n+                self.temp_index.apply_treediff(cd.tree, cd.parent.data.tree,\n+                                               quiet = True)\n+                merged.append(pn)\n+                # The self.temp_index was modified by apply_treediff() so\n+                # force read_tree() the next time merge() is used.\n+                self.temp_index_tree = None\n+            except git.MergeException:\n+                pass\n+        return merged\n\n\n-- \nCatalin\n"},{"id":"109981","messageId":"20090331072701.GA7730@diana.vm.bytemark.co.uk","threadId":"18435","inReplyTo":"b0943d9e0903300901i469bd899v3518b43c331bd9df@mail.gmail.com","subject":"Re: [StGit PATCH] Add the --merged option to goto","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2009-03-31T07:27:01Z","receivedAt":"2009-03-31T07:27:01Z","isPatch":true,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2009-03-30 17:01:12 +0100, Catalin Marinas wrote:\n\n> 2009/3/26 Karl Hasselström <kha@treskal.com>:\n>\n> > On 2009-03-25 10:24:13 +0000, Catalin Marinas wrote:\n> >\n> > > BTW, why don't we keep the tree information directly in the\n> > > Index object? Since this object is modified only via its own\n> > > interface, it can do all the checks and avoid the managing of\n> > > temp_index_tree in the Transaction object.\n> >\n> > I guess that might be a good idea -- it should be doable without\n> > any extra overhead for users that don't want it.\n>\n> I tried but gave up quickly. The IndexAndWorktree class also dirties\n> the Index with the merge operations, so it is not worth the hassle.\n\nOK. (Though you should be able to set the tree to None for those\ncases, since the meaning of None is simply that we don't promise\nanything about what tree is currently in the index.)\n\nAnd since I've run out of even remotely plausible things to complain\nabout,\n\nAcked-by: Karl Hasselström <kha@treskal.com>\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"}]}