[MERGE] new merge-type=lca

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Aaron Bentley wrote:
> This is a very good description.  It'd be great to preserve it for interested
> developers in doc/developers somewhere.

Okay.

> I wonder if this merge is good enough that we don't need to warn users that
> we've detected a criss-cross when merging?

In the case of criss-cross, they will sometimes see their merge
resolutions shown as conflicts with alternative resolutions of the same
conflict (this is what the 'conflicted-a' / 'conflicted-b' lines do).
So I *think* that it still makes sense to warn them that the merge may
be a little weirder than normal.  What do you think?

>> +    def _merged_lines(self, file_id):

...

> It's a shame you need to duplicate so many lines here to change one function
> call.  I guess it's not worth adding complexity to do something about it.

Yeah, it seems like a pretty even trade.

>> +    @staticmethod
>> +    def _subtract_plans(old_plan, new_plan):
>> +        matcher = patiencediff.PatienceSequenceMatcher(None, old_plan,
>> +                                                       new_plan)
>> +        last_j = 0
>> +        for i, j, n in matcher.get_matching_blocks():
>> +            for jj in range(last_j, j):
>> +                yield new_plan[jj]
>> +            for jj in range(j, j+n):
>> +                plan_line = new_plan[jj]
>> +                if plan_line[0] == 'new-b':
>> +                    pass
>> +                elif plan_line[0] == 'killed-b':
>> +                    yield 'unchanged', plan_line[1]
>> +                else:
>> +                    yield plan_line
>> +            last_j = j + n
> 
> I see this function is just moved, rather than added, but I find it very hard to
> understand.  It's not obvious to me why subtracting one plan from another should
> produce this particular transformation.
> 
> It's tangential to your change, so don't feel that you need to do this to get my
> approval for lca-merge, but some explanatory comments for this functional would
> be extremely helpful.

Here's the explanation I'm planning to put:
        """Remove changes from new_plan that came from old_plan.

        It is assumed that the difference between the old_plan and new_plan
        is their choice of 'b' text.

        All lines from new_plan that differ from old_plan are emitted
        verbatim.  All lines from new_plan that match old_plan but are
        not about the 'b' revision are emitted verbatim.

        Lines that match and are about the 'b' revision are the lines we
        don't want, so we convert 'killed-b' -> 'unchanged', and 'new-b'
        is skipped entirely.
        """

I don't feel like that explanation's crystal clear, but it's the best
I've been able to come up with.  What do you think?


>> +    def _determine_status(self, revision_id, unique_lines):
>> +        """Determines the status unique lines versus all lcas.
>> +
>> +        Basically, determines why the line is unique to this revision.
>> +
>> +        A line may be determined new or killed, but not both.
>> +
>> +        :return a tuple of (new_this, killed_other):
>> +        """
>> +        new = self._find_new(revision_id)
>> +        killed = set(unique_lines).difference(new)
>> +        return new, killed
> 
> It'd help to say something like “:param unique_lines: a collection of line
> numbers”.  Or perhaps just rename it to “unique_line_numbers”.  When I see a
> variable called “foo_line”, I tend to expect it to be the contents of the line,
> not a line number.

Makes sense.

>> +class _PlanLCAMerge(_PlanMergeBase):
>> +    """
>> +    This implementation differs from _PlanMerge.plan_merge in that:

> It's odd to describe a class' implementation as differing from a method.
> Did you mean to say “This implementation differs from _PlanMergeBase in that:
> ...”?

Docstring represents an earlier sate, where PlanMerge had 'plan_merge'
and 'plan_lca_merge' members instead of there being two classes.  Will fix.

>> +    def test_lca_merge_criss_cross(self):
>> +        tree_a = self.make_branch_and_tree('a')
>> +        self.build_tree_contents([('a/file', 'base-contents\n')])
>> +        tree_a.add('file')
>> +        tree_a.commit('', rev_id='rev1')
>> +        tree_b = tree_a.bzrdir.sprout('b').open_workingtree()
>> +        self.build_tree_contents([('a/file',
>> +                                   'base-contents\nthis-contents\n')])
>> +        tree_a.commit('', rev_id='rev2a')
>> +        self.build_tree_contents([('b/file',
>> +                                   'base-contents\nother-contents\n')])
>> +        tree_b.commit('', rev_id='rev2b')
>> +        self.build_tree_contents([('a/file',
>> +                                   'base-contents\nthis-contents\n')])
> 
> Isn't this call to build_tree_contents redundant?

Yes.  Good catch.

>> +    def plan_lca_merge(self, ver_a, ver_b, base=None):
>> +        from merge import _PlanLCAMerge
> 
> Relative imports are fragile.  Always use “from bzrlib.foo ...”

The spirit is willing, but the flesh is weak.

Thanks for your review.

Aaron

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHfHhu0F+nu1YWqI0RAmX8AJ440BoHy7KrEMrB/QY/ziYlFjK9YQCfbjKb
rUgOAMOC9i8XohS+kLhH8tI=
=1gjt
-----END PGP SIGNATURE-----