[MERGE][BUG #151731] Alternative Cherrypicking merges

[John Arbash Meinel]

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

James Westby wrote:
| 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?

THIS    BASE    OTHER
a	a	a
b	b	b	
q	c	c
	d	d
	e	f
		e
		g

Or if we re-write it to find matching sections:

THIS    BASE    OTHER
~ a	a	a
~ b	b	b	
- ---    ---     ---
~ q	c	c
	d	d
~       ---     ---
		f
~       ---     ---
	e	e
~       ---     ---
		g

Obviously ab is maintained without change.
Then we have an obvious conflict for:
BASE => THIS
cde     q
versus
BASE => OTHER
cde  => cdfeg

Standard 3-way merge would give us:
a
b
<<<<<<<
q
=======
c
d
f
e
g
|>>>>>>

However, we are explicitly cherrypicking. Which means we *don't* want lines that
are present in BASE to be present in the output. Which means we want to hide c d
e. There are 2 options

a
b
<<<<<<<
q
=======
f
g
|>>>>>>

or

a
b
<<<<<<<
q
=======
f
|>>>>>>
<<<<<<<
=======
g
|>>>>>>

Either could be seen as valid. My particular implementation does the latter,
partially because I think I prefer it.
Specifically, in the former case, you are throwing away the context lines, and
merging it together into one big region. In the later, the context lines
introduce a break to the conflicting region. (Which doesn't have anything to
match against in THIS.)
Oh, and I don't think we have a way of representing conflicts that span
non-contiguous ranges.
As the current output is:

('conflict', start_BASE, end_BASE, start_THIS, end_THIS, start_OTHER, end_OTHER)

I suppose some possibilities arise. For example, we could change it so that this
function only removes prefix and suffix matches, and keeps all the internal
context. So if you had:

THIS	BASE	OTHER
a	a	a
b	b	b
c	d	d
	e	f
	g	e
		h
		g

Then it would strip 'd' and 'g', but not 'e' from the conflict
a
b
<<<<<<<
c
=======
f
e
h
|>>>>>>

rather than the current
a
b
<<<<<<<
c
=======
f
|>>>>>>
<<<<<<<
=======
h
|>>>>>>

I can't say for sure what the "right" thing is. I can imagine someone doing
something like:

common context

def function_1_to_cherrpick():
~  bar

def function_written_to_not_cherrypick():
~  baz

def function_2_to_cherrpick():
~  bing

That is the case where I think we want to do what I've done, and have:

common context

<<<<<<
======
def function_1_to_cherrpick():
~  bar

|>>>>>
<<<<<<
======
def function_2_to_cherrpick():
~  bing

|>>>>>

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

The _refine... method is because we want to find "unmatched" regions, but all we
have is "get_matching_blocks()". So we track where we were last, and then when
we step to the next matching region, we check to see if any lines were skipped,
which thus must be unmatched.

| 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


Well, technically it is up to the release manager. Who I believe is still Martin.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFH1wclJdeBCYSNAAMRApIaAJ91a/SydIttr0phLwIX5MjfkcfZwwCfWq0A
qHaiwIulKDXrOyoAXaWlIyg=
=6xWS
-----END PGP SIGNATURE-----