[MERGE] merge and pull support merge directives

Aaron Bentley aaron.bentley at utoronto.ca
Sun Apr 1 04:21:14 BST 2007


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

John Arbash Meinel wrote:
> 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.

Okay.

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

Sure.  I was just following what the code was already doing.

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

Yeah, I didn't like that, either.  None of the merge APIs are ideal, but
 _merge_helper is the only one that supports all the possible
invocations, so I stayed with that.

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

I intended merge directives to be used with automated systems like PQM
and Bundle Buggy.  Compared to that, this is "manual".  I can take it out.

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

Okay, I can do that, the rev_id change, and the "manual" thing.  I'd
like to save the _merge_helper updates for a separate change.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGDyUp0F+nu1YWqI0RAqQwAJ9U4xTSSxkrPHqoslu1lqbnnQOivQCfUZ9X
D9Y5OvxjVXlqvKvnAx3Ga9s=
=PEds
-----END PGP SIGNATURE-----



More information about the bazaar mailing list