[PATCH] (reminder) Fixes for revno:N:path

[Matthieu Moy]

John Arbash Meinel <john at arbash-meinel.com> writes:

> If I'm reading this patch correctly, basically you are just expanding
> the support for 'revno:*' syntax to more commands. Is that correct?

Yes.

>> === modified file bzrlib/builtins.py
>> --- bzrlib/builtins.py
>> +++ bzrlib/builtins.py
>> @@ -1346,7 +1346,11 @@
>>          else:
>>              # local dir only
>>              # FIXME ? log the current subdir only RBC 20060203 
>> -            dir, relpath = bzrdir.BzrDir.open_containing('.')
>> +            if revision is not None and len(revision) > 0 and revision[0].get_branch():
>> +                location = revision[0].get_branch()
>> +            else:
>> +                location = '.'
>> +            dir, relpath = bzrdir.BzrDir.open_containing(location)
>>              b = dir.open_branch()
>
> ^- This is the sort of thing that I'm concerned syntax like this is
> going to cause. All of a sudden every command needs to check the 10
> different ways that a branch + revision can be supplied.

Well, in the long term, I'm now convinced that we should move to
something more general than this revisionspecifier (URL;revspec for
example, but I didn't hear enough opinions to be 100% convinced this
is the way to go).

So, I prefer keeping my time for something better in the long term
than hacking too much on this one.

>> @@ -1355,6 +1359,12 @@
>>          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():
>> +                # b is taken from revision[0].get_branch(), and
>> +                # show_log will use its revision_history. Having
>> +                # different branches will lead to weird behaviors.
>> +                raise BzrCommandError(
>> +                    "Log doesn't accept two revisions in different branches.")
>
> ^- Doesn't this trigger something weird if you just do:
> bzr log -r revno:2:branch..-1

It will be forbidden.

There are some corner-cases in which it could be accepted, like

$ bzr log -r revno:2:./..-1

but these corner-case have no benefit for the user (bzr log -r 2..-1
is obviously better).

> Since the second one will return None, but the first one will return a
> path? I think you need to check whether one of them is None.

If one is None, and not the other, they will be different, and it will
be forbidden.

[ cmd_cat(...)]
>> @@ -1648,6 +1658,8 @@
>>  
>>          if tree is None:
>>              b, relpath = Branch.open_containing(filename)
>> +        if revision is not None and revision[0].get_branch() is not None:
>> +            b = Branch.open(revision[0].get_branch())
>
> ^- So will it use the Branch rather than using the 'tree.branch'?

Yes. If you say "revno:N:some/branch" it means you want revision N in
some/branch, not in tree.branch.

> So maybe we need to sit back for a bit, and figure out how to make
> things a bit cleaner.

Same as above. I think the long term solution is not here, but this
fixes something broken, so it's good in the short term.

-- 
Matthieu