[MERGE] merge and pull support merge directives

John Arbash Meinel john at arbash-meinel.com
Thu Mar 29 16:16:45 BST 2007


John Arbash Meinel has voted +1 (conditional).
Status is now: Conditionally approved
Comment:
For -r, we could evaluate it after installing the revisions (since then 
we do have a copy of the branch).

I don't really like that "get_target_revision()" has a side effect of 
installing revisions. It doesn't seem correct for a "get_*" function. 
Maybe: "install_revisions()" which is defined as returning the tip 
revision.


I realize it makes the review a bit more difficult, but we really should 
try to use "revision_id" instead of "rev_id". At least it is listed in 
HACKING: ``revision_id`` not ``rev_id`` or ``revid``
(This isn't a blocking thing, but we should clean up the code when we 
can, or it will never be cleaned up)



I really don't like that you have to fake the --revision argument with:
+                other_rev_id = mergeable.get_target_revision(
+                    tree.branch.repository)
+                revision = [RevisionSpec.from_string(
+                    'revid:' + other_rev_id)]

To me that just means the merge api is incorrect. And certainly you 
don't have to fix that here.



Why are all these tests called "test_manual_*"
+    def test_manual_merge_user_r(self):


So the only thing I would block on is that the function is called 
"get_target_revision" and has the side effect of installing revisions. 
The rest is just discussion.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C45FAE388.7050605%40utoronto.ca%3E



More information about the bazaar mailing list