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

From: Catalin Marinas <catalin.marinas@gmail.com>
Date: Mar 25, 2009, 10:24 UTC
Message-ID: <b0943d9e0903250324j9ed0ed9k2d97cbacba6a7801@mail.gmail.com>
In-Reply-To: <20090325090541.GA24889@diana.vm.bytemark.co.uk>

2009/3/25 Karl Hasselström <kha@treskal.com>:

Show 8 quoted lines

> On 2009-03-24 15:40:10 +0000, Catalin Marinas wrote:
>> 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.

Yes, that's what I meant by "doesn't correspond to any tree".

BTW, why don't we keep the tree information directly in the Index object? Since this object is modified only via its own interface, it can do all the checks and avoid the managing of temp_index_tree in the Transaction object.

Show 44 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)".

Yes. But it may be even better to do this in Index. Index.apply_treediff() would set the tree to None and read_tree or write_tree would set it to the corresponding tree.

>  3. Why are empty patches considered not merged?

They would be reported as empty anyway and in general you don't submit empty patches for upstream merging.

-- 
Catalin