[MERGE] cleanup of merge command

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
In general, it would be nice to see doc strings for the
new functions, like "_select_branch" and "_from_branch".

I think it would be a lot clearer if we could use a different
variable name than 'branch' when we are talking about a path
to a branch, versus an actual branch object.
branch_path comes to mind.
I know we have to use 'branch' as the parameter from the
user (because that is what we wanted the help text to show),
but changing away from that internally would help clarity.

Especially code like this:
+        other_loc, branch = self._select_branch(tree, branch, revision, 
-1)
+        if revision is not None and len(revision) == 2:
+            base_loc, branch = self._select_branch(tree, branch, 
revision, 0)

Where I believe the variable we are passing is a path, but the variable
being returned is a Branch object.



Calling the function: _get_mergeable_from_branch() rather than
_from_branch() would help. (maybe _get_merger_from_branch, at least
something that indicates *what* you are getting from the branch).



I didn't see any direct tests for:

Merger.from_uncommitted, Merger.from_mergeable, Merger.from_revision_ids

There is a test that uses from_uncommitted, but more as a side-effect
than because it is meant to be testing that api.

So a few tests that exercise those functions and assert that we get the
right Merger object would be good.


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