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

From: Karl Hasselström <kha@treskal.com>
Date: Mar 23, 2009, 08:45 UTC
Message-ID: <20090323084507.GA6447@diana.vm.bytemark.co.uk>
In-Reply-To: <20090320161233.28989.82497.stgit@pc1117.cambridge.arm.com>

On 2009-03-20 16:15:45 +0000, Catalin Marinas wrote:

Show 16 quoted lines

> 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. The
> trivial merge cases in Index.merge() are ignored when performing
> this operation otherwise the results could be wrong (e.g. a patch
> adding a hunk and a subsequent patch canceling the previous change
> would both be considered merged).
>
> Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
> ---
>
> This is in preparation for the updating of the push command where we
> have this functionality (I think we had it for goto as well but was
> lost with the update to stgit.lib). Test cases with --merged are
> already done for the push command, so I haven't added any for goto
> (but I'll push this patch only after push is updated).

Looks good, except for a few things:

Show 9 quoted lines

> @@ -732,7 +732,7 @@ class Index(RunWithEnv):
>          # to use --binary.
>          self.apply(self.__repository.diff_tree(tree1, tree2, ['--full-index']),
>                     quiet)
> -    def merge(self, base, ours, theirs, current = None):
> +    def merge(self, base, ours, theirs, current = None, check_trivial = True):
>          """Use the index (and only the index) to do a 3-way merge of the
>          L{Tree}s C{base}, C{ours} and C{theirs}. The merge will either
>          succeed (in which case the first half of the return value is

Please update the documentation with your new option. :-)

Show 17 quoted lines

> @@ -752,12 +752,13 @@ class Index(RunWithEnv):
>          assert current == None or isinstance(current, Tree)
>  
>          # Take care of the really trivial cases.
> -        if base == ours:
> -            return (theirs, current)
> -        if base == theirs:
> -            return (ours, current)
> -        if ours == theirs:
> -            return (ours, current)
> +        if check_trivial:
> +            if base == ours:
> +                return (theirs, current)
> +            if base == theirs:
> +                return (ours, current)
> +            if ours == theirs:
> +                return (ours, current)

Uh, what? What's the point of not doing this unconditionally?

Show 10 quoted lines

> @@ -379,3 +385,25 @@ 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 = []
> +        temp_index = self.__stack.repository.temp_index()
> +        temp_index_tree = None

There's no need to create a new temp index here. The transaction object already has one.

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