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

From: Karl Hasselström <kha@treskal.com>
Date: Mar 24, 2009, 13:16 UTC
Message-ID: <20090324131640.GB4040@diana.vm.bytemark.co.uk>
In-Reply-To: <b0943d9e0903230933n5b71a53elcfaa13f00883861d@mail.gmail.com>

On 2009-03-23 16:33:04 +0000, Catalin Marinas wrote:

Show 61 quoted lines

> 2009/3/23 Karl Hasselström <kha@treskal.com>:
>
> > On 2009-03-20 16:15:45 +0000, Catalin Marinas wrote:
> >> @@ -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?
>
> There are a few cases where my algorithm failed because the reverse
> applying of patches fell on one of those special cases (otherwise
> they wouldn't apply). The check_merged() function assumes that if a
> patch can be reversed in a given tree, it was already included in
> that tree.
>
> Let's assume that the tree corresponding to the top patch is T1. We
> have the following cases for reverse-applying a patch which fall
> under the trivial cases above (patch expressed as
> bottom_tree..top_tree):
>
> The empty patch cases should be ignored from such test (not done
> currently):
>
> T1..T1 => merge(T1, T1, T1) == T1
> T2..T2 => merge(T2, T1, T2) == T1
>
> The non-empty patch situations:
>
> T1..T2 => merge(T2, T1, T1) == T1
> T2..T1 => merge(T1, T1, T2) == T2
>
> The T1..T2 is pretty common and happens when the base of a patch
> wasn't modified. Reverse-applying such patch should not normally
> succeed but the merge() here uses one of those special cases. The
> merge() result is correct since we want two trees merged, T1 and T1,
> with a common base, T2, used a helper.
>
> The T2..T1 cases would succeed with both trivial checks and
> apply_treediff() and that's probably OK since if a patch generates
> the same tree when applied, the changes it makes were probably
> already included.
>
> 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? See my other comment below.

Show 17 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.
>
> 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 think what you should do is something like what merge() does:

  if temp_index_tree != branch_tip:
      temp_index.read_tree(branch_tip)
      temp_index_tree = branch_tip
  try:
      temp_index.apply_treediff(patch_bottom, patch_top, quiet = True)
      temp_index_tree = temp_index.write_tree()
      return True
  except MergeException:
      return False

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