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

[John Arbash Meinel]

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

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
> 
> 

I'm a bit concerned about this line:

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

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

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

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

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

...

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

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

...

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

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

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

BB:tweak

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

iEYEARECAAYFAkjJjFAACgkQJdeBCYSNAAM2yQCfS4x3OKZJ6FsQhpp/RPqbpwEX
/7UAoKXkP7MtbB1G0B0zo8R1hFiKTxOf
=Diz/
-----END PGP SIGNATURE-----