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

[Kuno Meyer]

Thank you for your extensive review. Here my reply, additionally taking 
into account your and John Arbash Meinel's subsequent mails. Have a look 
at the attached patch.

On 14.08.2007 17:26, Alexander Belchenko wrote:
> 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.

That's right. I moved the code, but tried not to further increase the 
nesting/indentation level.

[skip]
>> +# 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.

Yes. You have certainly an important point in stating that the 
case-sensitivity is more filesystem-dependent than platform-dependent.

And a file-system dependency (which always includes a path) is not well 
covered with a static Feature.

I removed the feature completely and introduced a new method 
'osutils.is_case_sensitive(dir)'.

The main problem is now how to test this, without repeating the concrete 
implementation in the testcase. I hope I've taken an acceptable approach.

> 
> 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.')
>> +        

Done. Made the reasons for skipping tests more explicit.

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

Added.

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

Yes. I've attached a sentence directing to the docstring of the
Command class.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: internalglobexpand_v3.1.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20070815/a506c43f/attachment-0001.diff