[PATCH][UPDATED] Re: make "merge" work nicely with revno:N:patch

[Aaron Bentley]

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

Matthieu Moy wrote:
>> "bzr merge -r revno:42:/path/to/branch" didn't work. Well, not very serious
>> since "bzr merge -r revno:42 /path/to/branch/" does the same, but I've
>> written this extension to revno: partly to be able to easily build a command
>> line like
>>
>> bzr merge -r ${REVISION}

The motivation for doing this still isn't clear to me.  What's wrong
with the other form?

>> so it's better to have it working.

I really, really don't like jamming the branch info into the revision
spec.  But since it's already in, I agree consistency is good.  I hope
someone will step up and generalize revision/tree/branch/file handling soon.

>> Attached is a bundle with an implementation and testcase for it.
> @@ -1347,6 +1351,9 @@
>          elif len(revision) == 1:
>              rev1 = rev2 = revision[0].in_history(b).revno
>          elif len(revision) == 2:
> +            if revision[1].get_branch() != revision[0].get_branch():
> +                raise BzrCommandError(
> +                    "Log doesn't accept two revisions in different branches.")

Why not?  The two revisions may be legitimate arguments even if they're
specified according to different branches.  For example, bzr log -r
revno:-1:/bzr/dev..revno:-1:http://bazaar-vcs/bzr/bzr.dev would be
similar to "missing".  And equivalent to "bzr log -r
branch:/bzr/dev..branch:http://bazaar-vcs/bzr/bzr.dev", I expect.

If we're going to add this new syntax, we might as well take advantage
of the new options it provides.

> @@ -1632,13 +1639,18 @@
>          if revision is not None and len(revision) != 1:
>              raise BzrCommandError("bzr cat --revision takes exactly one number")
>          tree = None
> -        try:
> -            tree, relpath = WorkingTree.open_containing(filename)
> -            b = tree.branch
> -        except NotBranchError:
> -            pass
> +        b = None
> +        if revision is not None and revision[0].get_branch() is not None:
> +            b = Branch.open(revision[0].get_branch())
> +            relpath = filename

Having the meaning of "filename" change depending on the supplied
revision specs looks surprising to me.

> @@ -2150,13 +2162,15 @@
>                  else:
>                      return 1
>  
> -        branch = self._get_remembered_parent(tree, branch, 'Merging from')
> +        if revision is None or len(revision) < 1 or revision[0].needs_branch():
> +            branch = self._get_remembered_parent(tree, branch, 'Merging from')
>  
>          if revision is None or len(revision) < 1:
>              base = [None, None]
>              other = [branch, -1]
>              other_branch, path = Branch.open_containing(branch)
>          else:
> +            branch = revision[0].get_branch() or branch
>              if len(revision) == 1:
>                  base = [None, None]
>                  other_branch, path = Branch.open_containing(branch)
> @@ -2167,6 +2181,9 @@
>                  if None in revision:
>                      raise BzrCommandError(
>                          "Merge doesn't permit that revision specifier.")
> +                if revision[1].get_branch() != revision[0].get_branch():
> +                    raise BzrCommandError(
> +                        "Merge doesn't accept two revisions in different branches.")

Again, I don't see why not.  Maybe it's not useful every day, but it
certainly can be.

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

iD8DBQFE1oAr0F+nu1YWqI0RAulPAJ4v776HOzxqi97FsJRFj9QH0xt8XQCeLzuN
+MtoP8WN40lBoNHF+3W2Nw0=
=kdBX
-----END PGP SIGNATURE-----