[MERGE/RFC] case sensitivity on Windows

[Mark Hammond]

On 22/12/2008 5:05 PM, Ian Clatworthy wrote:
> Thanks for working on this and apologies for taking so long..

> There's a lot to like about this patch: ...

Thanks and thanks :)

> When does this case_sensitive attribute ever get set to False?
> (Sorry if that's a dumb question.)

WorkingTree sets this to False after probing the file-system.  All this 
patch does is move the attribute up the inheritance tree, primarily so 
smart_add() can check if it is set to prevent adding 2 items that differ 
only by case on such trees (as tested by test_re_add())

>>> I guess the only reason not to do that is because you may be talking
>>> about the path-on-disk rather than the path-in-the-tree, which can
>>> certainly give different results, and why all of this is really hard to
>>> get right, because you never now for a string whether it is the path on
>>> disk, or the path in memory, etc.
>> Exactly - each consumer of tree_files() needs to be examined and the
>> appropriate action for that command taken, possibly depending on other
>> params, as we saw above.  As you note, in tree_files() you aren't sure
>> which "canonical" version is desired, if any, but each consumer of
>> tree_files() should only have one unambiguous requirement in this regard
>> - hence the burden really does need to fall on each individual command.
>>   I did try passing a param down through tree_files() for this purpose,
>> but it got ugly quickly...
>
> Hmm - that *really* is unfortunate because it implies that cicp support
> will get added/debugged one command at a time. I wonder whether you
> ought to change safe_relpath_files() to cover the most common case
> (strings are inventory names) and add the special case handling into
> the commands (only add and mv?) that behave otherwise?

Yeah - its not as bad as it was when I first started working on this 
patch, and as you mention, given 'mv' and 'add' are the only special 
cases we've identified, I've gone and added a param to list_files and 
removed most of the other callers in builtins.py.  This does clean 
things up alot - I should have investigated this more when John 
suggested it :(

>> +    # use an explicit iterator so we can easily consume the rest on early exit.
>> +    bit_iter = rel.split('/')
>> +    for bit in bit_iter:
>> +        lbit = bit.lower()
>
> Comparing this code to _yield_canonical_inventory_paths in tree.py, is there a
> reason why you did iter(path.split("/")) in the latter but haven't used iter()
> around rel.split('/') above?

The comment is correct, but the code is wrong :(  Explicitly using 
iter() allows us to simply consume the rest without tracking the current 
index we are processing.  This obviously means that the code which 
relied on this fact was not actually being exercised!  So I've corrected 
the code and added more explicit tests for these cases.

I believe I've addressed every other comment in the attached patch.

Thanks,

Mark
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: cicp_filesystem.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20081224/73038c1a/attachment-0001.diff