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

[Matthieu Moy]

Aaron Bentley <aaron.bentley at utoronto.ca> writes:

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

Well, the main problem for which I introduced this was that
previously, there were no way _at all_ to diff two remote revisions.

Now, I find it very conveinient too to have a way to specify any
remote or local revision with a single string. For example, in DVC, I
have a lisp structure to specify revisions (somewhat similar to
revision spec in bzr). Then, to show the diff, I just run
(pseudo-code) :

  bzr diff -r (revision-to-string rev1)..(revision-to-string rev2)

I found it conveinient in DVC, I didn't have to write shell scripts
calling bzr, but I'm sure I would also appreciate the facility to
store a local or remote revision in a single variable
(foo="revno:42:path/to/branch").

Command-line interface doesn't mean only /user/ interface, it's the
interface to anything calling bzr as an executable (as opposed to
calling bzrlib).

>>> so it's better to have it working.
>
> I really, really don't like jamming the branch info into the revision
> spec.  

Well, I'm not the one who started this. "branch:..." has been there
for some time before I proposed and implemented that other revision
spec. The difference beign that "branch:..." does a local fetch.
That's conveinient in some cases, but that also means that doing

$ bzr diff -r branch:/my/kernel/tree/..branch:/my/other/kernel/tree/

will fetch all the kernel history in my local branch, which may be
completely unrelated from the kernel. Since we have no garbage
collector as of now, this might mean adding hundreds of megabytes to
the local repository, potentially making it somewhat unuseable.

So, I didn't want to use this local fetch trick, and did something
IMHO cleaner.


Anyway, to say "diff this revision of this branch with this other
revision of this other branch", I believe the two ways are to add
branch information in revision specifiers, or to add revision
information in branch locations, and I don't find one really better
than the other.

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

That's an option. I went the safe and simple way and prevented it
cleanly, since I suppose it wouldn't have worked out-of-the-box
otherwise (indeed, I don't know how to make it work, since there may
be several paths from one revision to another in the history).

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

I'm open to proposals to make it better. I don't know what's the best
syntax.

I don't like the idea of providing the full path in a single URL. To
me,

$ <some-command> http://site.com/path/to/file.txt

means that there is actually something in
http://site.com/path/to/file.txt, not that there's a branch in, i.e.
http://site.com/path/ that refers to a tree containing a file
to/file.txt. Indeed, if there's another branch in
http://site.com/path/to/, then, I don't even know how to cat file.txt
in the first branch.

Had this been only me, I'd have removed the existing syntax with all
in the URL, but I didn't want to break the existing ;-).

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

Same as above, I just went the safe way. Well, this one might work
out-of-the-box. Will it be acceptable when bzr will record partial
merges like this one?

On those two points, I'd say "take it as it is for now", I won't have
time to add testcases for the general case (they are definitely
needed) before a few days/weeks.

For now, I'll just change the message to say "doesn't (yet)
accept ..." and add a comment.

-- 
Matthieu