[MERGE/RFC] case sensitivity on Windows
skippy.hammond at gmail.com
Tue Dec 23 21:33:55 GMT 2008
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.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
More information about the bazaar