[MERGE] internal glob expansion for all commands on win32 and for invocations from the test suite

[Alexander Belchenko]

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

bb:comment

Your patch seems technically correct, and I hope it will be useful
for people with *nix background working on win32 console
(as the fates decree).

I just have some questions.

> === modified file 'bzrlib/commands.py'
> --- bzrlib/commands.py	2007-08-07 19:18:38 +0000
> +++ bzrlib/commands.py	2007-08-09 18:35:37 +0000
> @@ -575,7 +564,7 @@
>          if ap[-1] == '?':
>              if args:
>                  argdict[argname] = args.pop(0)
> -        elif ap[-1] == '*': # all remaining arguments
> +        elif ap[-1] in ('*', '#'): # all remaining arguments, possibly expandable
>              if args:
>                  argdict[argname + '_list'] = args[:]
>                  args = []

^-- why not run glob_expand() here? It's the most appropriate place, IMO.
v-- Because otherwise

> @@ -603,6 +592,10 @@
>              else:
>                  argdict[argname] = args.pop(0)
>              
> +        # internal glob expansion
> +        if glob_expand and ap.endswith('#') and (argname + '_list') in argdict:
> +            argdict[argname + '_list'] = win32utils.glob_expand(argdict[argname + '_list'])
> +        

^-- you need to make a lot of extra checks, while in previous place above
you need to check only for glob_expand flag.

So, why?

> === modified file 'bzrlib/externalcommand.py'
> --- bzrlib/externalcommand.py	2007-03-21 01:34:41 +0000
> +++ bzrlib/externalcommand.py	2007-08-09 18:35:37 +0000
> @@ -56,7 +56,9 @@
>      def run(self, *args, **kwargs):
>          raise NotImplementedError('should not be called on %r' % self)
>  
> -    def run_argv_aliases(self, argv, alias_argv=None):
> +    def run_argv_aliases(self, argv, alias_argv=None, glob_expand=False):
> +        if glob_expand:
> +            argv = (argv or []) + ['--glob_expand']
>          return os.spawnv(os.P_WAIT, self.path, [self.path] + argv)
>  
>      def help(self):
> 

^-- /me wonder why we keep externalcommand interface, while we have
nice plugin system.
Of course, this question is not for you.
Someone from core developers know the answer?

> === modified file 'bzrlib/tests/test_win32utils.py'
> --- bzrlib/tests/test_win32utils.py	2007-07-24 19:40:40 +0000
> +++ bzrlib/tests/test_win32utils.py	2007-08-09 18:35:37 +0000
> @@ -17,41 +17,30 @@
> -# Features
> -# --------
> -
> -class _NeedsGlobExpansionFeature(Feature):
> -
> -    def _probe(self):
> -        return sys.platform == 'win32'
> -
> +# Feature
> +# -------
> +
> +class _CaseInsensitiveFilesystem(Feature):
> +
> +    def available(self):
> +        return (sys.platform == 'win32')
> +    
>      def feature_name(self):
> -        return 'Internally performed glob expansion'
> +        return "CaseInsensitiveFilesystemFeature"
> +    

^-- probably there are no linux users who want to run
bzr selftest on case-insensitive filesystem
(e.g. mounted USB flash disk, or mounted FAT32/NTFS disk
on dual boot machine), especially because now tests runs
in /tmp.
But IMO right way to check if file system is actually CIF
is to try read some file with uppercased name (e.g. TEST),
ensure that such file is absent, then write file with the same
but lowercased name (e.g. test) and check uppercased name
again.

I'm not sure is it worth to implement such complicated
logic for your patch, but I'm also not sure that Feature
name is accurate then.

And one more thing. Usually feature implements method _probe
to check is feature available. In your simply case it's OK
to override `available` method, but for more complicated
checks like I describe above it's better. If you planning
to extend this feature with more complicated check,
it's better to start using `_probe` now.

v-- And then you provide separate check for platform,
that inside has actually the same behavior as your
CaseInsensitiveFilesystemFeature above.
Why you using TestSkipped here instead of some sort of Features?

> +    def _requires_win32(self):
> +        if sys.platform != 'win32':
> +            raise TestSkipped('Win32 platform specific.')
> +        


I ran tests for wildcards:

E:\Bazaar\mydev\win32.globs>python bzr --no-plugins selftest wildcards -v
Bazaar (bzr) 0.90.0dev0

running 6 tests...
blackbox.test_add.TestAdd.test_add_with_wildcards
 OK                 991ms
blackbox.test_add.TestAdd.test_add_with_wildcards_unicode
 OK                 531ms
blackbox.test_commit.TestCommit.test_commit_with_wildcards
 OK                1272ms
blackbox.test_ignore.TestCommands.test_ignore_with_wildcards
 OK                 689ms
blackbox.test_mv.TestMove.test_mv_with_wildcards
 OK                 922ms
blackbox.test_remove.TestRemove.test_remove_with_wildcards
 OK                1271ms

- ----------------------------------------------------------------------
Ran 6 tests in 5.689s

OK
tests passed


Your patch affects many commands, especially status, diff and revert,
that used very often.
I think it will be nice to have simple test to check
how these commands works with wildcards.

Also, if you have inspiration, I'd like to suggest to
describe new '#' suffix of commands arguments in docs.
There is no actual topic in documentation about these suffixes,
only short section in HACKING entitled
"Processing Command Lines". IMO this information is very
important and useful for plugins writers. So, if you plan
to write something about suffixes, then it's the most
appropriate place IMO.

[µ]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGwcmXzYr338mxwCURAmseAKCVnYMdDbCN9ei+XxafjEdXgmheegCaA8Cg
OSubCE8AUWatKhcfrsAnV7s=
=Nvvj
-----END PGP SIGNATURE-----