[MERGE][BUG #151731] Alternative Cherrypicking merges

[James Westby]

On Tue, 2008-03-04 at 14:30 +0000, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I've been thinking about the cherrypicking bug:
> https://bugs.launchpad.net/bzr/+bug/151731
> 
> My notes on the bug are probably unclear, so I'll try to summarize, and
> present an alternative to 3-way merge that might work very well for
> cherrypicks.

Hi John,

Thanks for looking at this.

I think that I agree with you on the examination of the problem,
and I am trying to look at the code to review the patch, but I don't
feel comfortable giving a full review. I do have some comments though.


> @@ -453,13 +458,15 @@
>      supports_reprocess = True
>      supports_show_base = True
>      history_based = False
> +    supports_cherrypick = True
>      supports_reverse_cherrypick = True
>      winner_idx = {"this": 2, "other": 1, "conflict": 1}
>  
>      def __init__(self, working_tree, this_tree, base_tree,
> other_tree, 
>                   interesting_ids=None, reprocess=False,
> show_base=False,
>                   pb=DummyProgress(), pp=None, change_reporter=None,
> -                 interesting_files=None, do_merge=True):
> +                 interesting_files=None, do_merge=True,
> +                 cherrypick=False):

Nitpick: the cherrypick parameter isn't added to the docstring. However
do_merge isn't there as well. I only noticed this as you took the time
to update one of the previous ones.


> +    def test_merge3_cherrypick_w_mixed(self):
> +        base_text = 'a\nb\nc\nd\ne\n'
> +        this_text = 'a\nb\nq\n'
> +        other_text = 'a\nb\nc\nd\nf\ne\ng\n'
> +        # When cherrypicking, lines in base are not part of the
> conflict
> +        m3 = Merge3(base_text.splitlines(True),
> this_text.splitlines(True),
> +                    other_text.splitlines(True), is_cherrypick=True)
> +        m_lines = m3.merge_lines()
> +        self.assertEqualDiff('a\n'
> +                             'b\n'
> +                             '<<<<<<<\n'
> +                             'q\n'
> +                             '=======\n'
> +                             'f\n'
> +                             '>>>>>>>\n'
> +                             '<<<<<<<\n'
> +                             '=======\n'
> +                             'g\n'
> +                             '>>>>>>>\n',
> +                             ''.join(m_lines))

This is the only test that I don't fully understand, could you take
a minute to explain the result to me please?

All of the changes seem reasonable to me. The only grey areas I have
are the above test, and the _refine method.

In my tests this seems to do the right thing though, and using the
test case in the bug report it gives the expected result.

I would guess this is a little intrusive for 1.3, but that is up to
your second reviewer I guess.

Thanks,

James