Re: [StGit PATCH] Add the --merged option to goto

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

2009/3/24 Karl Hasselström <kha@treskal.com>:
> On 2009-03-23 16:33:04 +0000, Catalin Marinas wrote:
>> Now I understand it better :-). Reading my explanation above, it
>> seems that only the T1..T2 case matters and it can be taken care of
>> in the check_merged() function. Checking whether the tree returned
>> by merge() is different than "ours" should be enough for all the
>> above cases.
>
> Hmm. If the tip of the branch is T1, and we reverse-apply the patch
> T1..T2, we get the merge (base T2, ours T1, theirs T1) ... yeah, I see
> what you mean. The problem isn't that we give T1 as the result of this
> merge -- that's actually the right thing to do -- the problem is that
> you don't actually want a merge. What you want is patch application.
> Maybe the apply_treediff method would do?

Yes, see below for an updated patch.

>> I had the impression that an Index object would hold some state and
>> didn't want to break it. It seems OK to use as long as I don't touch
>> self.temp_index_tree. See below for an updated patch:
>
> Yes, an Index object owns a git index file.
>
> And no, not quite. temp_index_tree is set to the tree we know is
> stored in temp_index right now (or None if we don't know). The idea is
> that we'll often want to read a tree into the index that's already
> there, and by keeping track of this we'll get better performance.
> (This works very well in practice.) Apologies if there aren't comments
> explaining this ... the merge method has some docs on the subject.

I figured this out eventually. Anyway, with apply_treedif() there is
no need for the temp_index_tree. In the updated patch below, I don't
even call write_tree() as it isn't needed (to my understanding).
Whatever is in the index after the check_merged() function doesn't
correspond to any tree so it would need to be re-read.

BTW, I implemented push and pop and it now passes all the "push
--merged" tests. I had to correct the "already_merged" case in
Transaction.push_patch() to generate an empty patch.

    Add the --merged option to goto

    This patch adds support for checking which patches were already merged
    upstream. This checking is done by trying to reverse-apply the patches
    in the index before pushing them onto the stack.

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

diff --git a/stgit/argparse.py b/stgit/argparse.py
index 85ee6e3..765579c 100644
--- a/stgit/argparse.py
+++ b/stgit/argparse.py
@@ -225,6 +225,10 @@ def keep_option():
                 short = 'Keep the local changes',
                 default = config.get('stgit.autokeep') == 'yes')]

+def merged_option():
+    return [opt('-m', '--merged', action = 'store_true',
+                short = 'Check for patches merged upstream')]
+
 class CompgenBase(object):
     def actions(self, var): return set()
     def words(self, var): return set()
diff --git a/stgit/commands/goto.py b/stgit/commands/goto.py
index 66f49df..839b75c 100644
--- a/stgit/commands/goto.py
+++ b/stgit/commands/goto.py
@@ -28,7 +28,7 @@ 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 = argparse.keep_option()
+options = argparse.keep_option() + argparse.merged_option()

 directory = common.DirectoryHasRepositoryLib()

@@ -47,8 +47,14 @@ def func(parser, options, args):
         assert not trans.pop_patches(lambda pn: pn in to_pop)
     elif patch in trans.unapplied:
         try:
-            for pn in trans.unapplied[:trans.unapplied.index(patch)+1]:
-                trans.push_patch(pn, iw, allow_interactive = True)
+            to_push = trans.unapplied[:trans.unapplied.index(patch)+1]
+            if options.merged:
+                merged = set(trans.check_merged(to_push))
+            else:
+                merged = set()
+            for pn in to_push:
+                trans.push_patch(pn, iw, allow_interactive = True,
+                                 already_merged = pn in merged)
         except transaction.TransactionHalted:
             pass
     elif patch in trans.hidden:
diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index b146648..e2d9b78 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -297,7 +297,8 @@ class StackTransaction(object):
                     out.info('Deleted %s%s' % (pn, s))
         return popped

-    def push_patch(self, pn, iw = None, allow_interactive = False):
+    def push_patch(self, pn, iw = None, allow_interactive = False,
+                   already_merged = False):
         """Attempt to push the named patch. If this results in conflicts,
         halts the transaction. If index+worktree are given, spill any
         conflicts to them."""
@@ -305,11 +306,15 @@ class StackTransaction(object):
         cd = orig_cd.set_committer(None)
         oldparent = cd.parent
         cd = cd.set_parent(self.top)
-        base = oldparent.data.tree
-        ours = cd.parent.data.tree
-        theirs = cd.tree
-        tree, self.temp_index_tree = self.temp_index.merge(
-            base, ours, theirs, self.temp_index_tree)
+        if already_merged:
+            # the resulting patch is empty
+            tree = cd.parent.data.tree
+        else:
+            base = oldparent.data.tree
+            ours = cd.parent.data.tree
+            theirs = cd.tree
+            tree, self.temp_index_tree = self.temp_index.merge(
+                base, ours, theirs, self.temp_index_tree)
         s = ''
         merge_conflict = False
         if not tree:
@@ -341,7 +346,9 @@ class StackTransaction(object):
         else:
             comm = None
             s = ' (unmodified)'
-        if not merge_conflict and cd.is_nochange():
+        if already_merged:
+            s = ' (merged)'
+        elif not merge_conflict and cd.is_nochange():
             s = ' (empty)'
         out.info('Pushed %s%s' % (pn, s))
         def update():
@@ -379,3 +386,23 @@ class StackTransaction(object):
         assert set(self.unapplied + self.hidden) == set(unapplied + hidden)
         self.unapplied = unapplied
         self.hidden = hidden
+
+    def check_merged(self, patches):
+        """Return a subset of patches already merged."""
+        merged = []
+        self.temp_index.read_tree(self.stack.head.data.tree)
+        # The self.temp_index is modified by apply_treediff() so force
+        # read_tree() the next time merge() is used.
+        self.temp_index_tree = None
+        for pn in reversed(patches):
+            # check whether patch changes can be reversed in the current index
+            cd = self.patches[pn].data
+            if cd.is_nochange():
+                continue
+            try:
+                self.temp_index.apply_treediff(cd.tree, cd.parent.data.tree,
+                                               quiet = True)
+                merged.append(pn)
+            except git.MergeException:
+                pass
+        return merged



Thanks.

-- 
Catalin