[MERGE][BUG #233817] Missing shows merged revisions with --include-merges

[Vincent Ladeuil]

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> Vincent Ladeuil wrote:
    >>>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:
    >> 
    >> <snip/>
    >> 
    john> So the primary issue is that (as of yet) we can't do
    john> dotted revno numbering without accessing the full
    john> revision graph. And your work here:
    >> 
    >> <snip/>
    >> 
    john> ^- Will give wrong numbers for anything resembling
    john> complex history. I can give specific graphs that it
    john> will break, or you can just trust me. :)
    >> 
    >> Trusting you, I propose this patch.
    >> 
    >> Vincent
    >> 
    >> 

    john> I'm a bit concerned about this line:

    john> -    for revno, rev_id in revisions:
    john> +    for rev in revisions:
    john> +        # We need the following for backward compatibilty (hopefully
    john> +        # this will be deprecated soon :-/) -- vila 080911
    john> +        if len(rev) == 2:
    john> +            revno, rev_id = rev
    john> +            merge_depth = 0
    john> +        else:
    john> +            revno, rev_id, merge_depth = rev

    john> I really don't like sometimes passing in 2 values and sometimes passing
    john> in 3. Is there something we can do better here?

Yes. Two possible solutions:

1) Always pass 3 values.

2) Rework the whole thing to use LogRevision objects (since this
   is where we take care of the problem *today*).

I did try (1) put it required updating tests and breaking
backward compatibility. As I intend to do (2), I punted.

    john> ...
    john> -        mainline.append((cur_revno, cur))
    john> +        mainline.append((str(cur_revno), cur))

    john> ^- Since you are changing the signature, what about setting
    john> "merge_depth=0" here for all mainline revisions. So something like:

This was a drive-by fix, doing (2) will get rid of it.

    john> mainline.append((str(cur_revno), cur, 0))

    john> ...

    john> +    def test_include_merges(self):
    john> +        tree = self.make_branch_and_tree('tree')
    john> +        rev1 = tree.commit('one', rev_id='rev1')

    john> ^- It would be nice to use the "BranchBuilder" api for this. Just as a
    john> way to encourage new tests to be written without hitting disk unless
    john> they are testing that portion of code.

Hmm, what I need here is the ability to create a branch from
another existing one and then merge. This is not (yet?) provided
by branch_builder and adding that is outside the scope of a tweak
I think. I filed https://bugs.edge.launchpad.net/bzr/+bug/269319.

    john> ...

    john> +        self.assertUnmerged([], [('2', 'rev2', 0), ('3', 'rev3',0 ),
    john> +                                 ('4', 'rev6', 0),
    john> +                                 ('3.1.1', 'rev4', 1), ('3.1.2',
    john> 'rev5', 1),
    john> +                                 ],

    john> ^- Just a quick check that we are sure this order is correct.

    john> Also, you don't have a check if we set "backward=True".

Right.

On the other hand, if you look at that patch history, you will
notice quite a few errors encountered before getting the
'backward' right :) So I can guarantee that if anybody mess with
that, they will notice quickly.

But I'll add tests anyway.

    john> BB:tweak

Thanks,

        Vincent