[rfc] [patch] revisionspec fix for branch:../../branch2

[John A Meinel]

Denys Duchier wrote:
> I changed the parsing of revisionspec to address the annoying problem that specs
> like branch:../../branch2 didn't currently work.  In so doing I simplified the
> code by using a regex to split on ".." not followed by "/".  I also added
> bzrlib.option to MODULES_TO_DOCTEST specifically for testing the new cases I
> added to _parse_revision_str's docstring.
> 
> This fix is available as revno=1521 of my bzr.fix.revisionspec branch:
> 
>          http://delta.univ-orleans.fr/~duchier/bzr/bzr.fix.revisionspec
> 
> 
> ------------------------------------------------------------
> revno: 1521
> committer: Denys Duchier <denys.duchier at mozart-oz.org>
> branch nick: bzr.fix.revisionspec
> timestamp: Tue 2006-01-17 16:31:21 +0100
> message:
>   distinguish ../ as path to branch and .. as revspec separator
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> === modified file 'bzrlib/option.py'
> --- bzrlib/option.py	
> +++ bzrlib/option.py	
> @@ -71,6 +71,10 @@
>      BzrError: No namespace registered for string: 'abc'
>      >>> _parse_revision_str('branch:../branch2')
>      [<RevisionSpec_branch branch:../branch2>]
> +    >>> _parse_revision_str('branch:../../branch2')
> +    [<RevisionSpec_branch branch:../../branch2>]
> +    >>> _parse_revision_str('branch:../../branch2..23')
> +    [<RevisionSpec_branch branch:../../branch2>, <RevisionSpec_int 23>]
>      """
>      # TODO: Maybe move this into revisionspec.py
>      old_format_re = re.compile('\d*:\d*')
> @@ -85,20 +89,9 @@
>              else:
>                  revs.append(RevisionSpec(None))
>      else:
> -        next_prefix = None
> -        for x in revstr.split('..'):
> -            if not x:
> -                revs.append(RevisionSpec(None))
> -            elif x[-1] == ':':
> -                # looks like a namespace:.. has happened
> -                next_prefix = x + '..'
> -            else:
> -                if next_prefix is not None:
> -                    x = next_prefix + x
> -                revs.append(RevisionSpec(x))
> -                next_prefix = None
> -        if next_prefix is not None:
> -            revs.append(RevisionSpec(next_prefix))
> +        sep = re.compile("\\.\\.(?!/)")

This looks a little better in raw mode:
sep = re.compile(r'\.\.(?!/)')

But only a little.


> +        for x in sep.split(revstr):
> +            revs.append(RevisionSpec(x or None))
>      return revs
>  
>  
> 
> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py	
> +++ bzrlib/tests/__init__.py	
> @@ -59,6 +59,7 @@
>                        bzrlib.merge3,
>                        bzrlib.osutils,
>                        bzrlib.store,
> +                      bzrlib.option
>                        ]
>  def packages_to_test():
>      import bzrlib.tests.blackbox
> 
> 

These should be sorted alphabetically.

> 
> ------------------------------------------------------------------------
> 
> 
> 
> --Denys

I think Robert was originally thinking to create a 'recursive decent'
parser. Somehow having it be more careful about pulling off a chunk, and
then parsing the next chunk.
But what you've done solves the direct problem.

With at least the alphabetical fix, +1 from me.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060117/372ef85d/attachment.pgp