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

From: Karl Hasselström <kha@treskal.com>
Date: Mar 25, 2009, 09:05 UTC
Message-ID: <20090325090541.GA24889@diana.vm.bytemark.co.uk>
In-Reply-To: <b0943d9e0903240840m3f22b702qd48293caad4187e3@mail.gmail.com>

On 2009-03-24 15:40:10 +0000, Catalin Marinas wrote:

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

Yes. You're not interested in the result of the patch application, you just want to know if it succeeded or not.

> Whatever is in the index after the check_merged() function doesn't
> correspond to any tree so it would need to be re-read.

It does correspond to _some_ tree, but as we have no particular reason to expect that it might be a tree that we're going to need again immediately, we can set temp_index_tree to None to force re-reading, rather than pay the cost of write_tree.

When pushing patches, on the other hand, we have good reasons to believe that the next tree we'll need in the index is the same (or at least very close to) the one produced in the last merge. Consider the case of popping a few patches, changing the message on the top patch, and then pushing the patches back, for example.

Show 19 quoted lines

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

Some points here:

  1. Check if temp_index_tree already contains the tree you want
     instead of doing read_tree unconditionally. That is, change

       self.temp_index.read_tree(self.stack.head.data.tree)

to

       if self.stack.head.data.tree != self.temp_index_tree:
           self.temp_index.read_tree(self.stack.head.data.tree)
           self.temp_index_tree = self.stack.head.data.tree

  2. If apply_treediff fails (raises MergeException), the index wasn't
     modified at all (but I guess you knew that, since your code
     relies on that fact), so there's no reason to set temp_index_tree
     to None in that case. So in order to not clear temp_index_tree
     unnecessarily in case none of the patches reverse-apply (a case I
     personally encounter frequently, since I leave the -m flag on all
     the time), move

       self.temp_index_tree = None

     to just after (or just before) "merged.append(pn)".

  3. Why are empty patches considered not merged?

Other than that, this logic looks fine.

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