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

[John Arbash Meinel]

Matthieu Moy wrote:
> Hi,
> 
> Attached is a version of my fixes for the revno:N:path revisionspec
> updated for the current bzr.dev.
> 
> 

Sorry for the delay in reviewing this. I got a little swamped this week.
I think these changes are before my updates to the revision spec stuff,
which just landed this week. Though they look mostly disjoint, so there
shouldn't be a huge conflict.

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

...




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

I think rather than doing all this work for every command, we should
write a helper function, that takes some paths, and a revision list, and
returns some WorkingTree, Branch, relative paths, and revision_id objects.

Then commands change from all of this special case stuff, to just
handing their parameters off to a helper function. Where we can get all
of the logic for stuff like this correct.

>  
>          if revision is None:
> @@ -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

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 revision[0].spec is None:
>                  # missing begin-range means first revision
>                  rev1 = 1
> @@ -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'?

So I believe I understand what you are trying to do. And if we decide
that we want revision spec to return branch paths, I think the code is
okay. But I think this is just going to get worse and worse, as we keep
trying to shoehorn existing code. And then we end up with a big
hacked-together mess.

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

One thing we have going for us, is that most internal functions should
be taking Branch objects, and shouldn't really care how we got there. So
at least any sort of trickiness should be encapsulated withing the cmd_*
layer.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060901/6bd4111a/attachment.pgp