[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