r224145 - Allow module deps to be printed in an arbitrary order

[Justin Bogner]

Richard Smith <richard at metafoo.co.uk> writes:
> On Fri, Dec 12, 2014 at 12:59 PM, Justin Bogner <mail at justinbogner.com> wrote:
>> Richard Smith <richard at metafoo.co.uk> writes:
>>> Thankyou!
>>>
>>> I think the change you're referring to is that it removed the
>>> pruning of /x/..  /. That was not unintentional - there were no
>>> tests for that behaviour, and it's wrong if x is a symlink - but I
>>> had intended to investigate further prior to checkin and forgot.
>>    
>> Yep, that's what I was looking at. I agree that the behaviour is a
>> bit iffy, but if such a path were to survive to the VFS writer we'd
>> assert, as it doesn't support path traversal. I'm looking into making
>> a test that actually triggers that path so I can figure out what to
>> do with it.
>
> Thanks, that sounds like a good start. This seems like a very tricky
> problem, but it also seems important to get it right; directory
> symlinks in include directories are not uncommon.
>
>> In any case, after looking a bit, the ".." change is not why these tests
>> started failing. The output order changed here because removeDotPaths
>> also improved on removePathTraversal by returning early if there are no
>> changes. If removeDotPaths changes anything, the path separators are
>> canonicalized and all end up being \\ on windows, but if it doesn't
>> change anything some of the separators may be /, since that's how they
>> were passed on the commandline.
>
> Aha, yes, that was an entirely unintentional change. (Is that actually a
> problem, or was it just something the tests weren't prepared for?)

It could lead to some directories showing up redundantly in the
vfs.yaml, each with a subset of the contents. I'm not sure if this would
break anything or not, but it's certainly confusing.

>> I'm not entirely sure how best to handle that. Maybe
>> ModuleDependencyListener should canonicalize path separators as a
>> separate step?
>
> Sounds like a good idea. Adding a call to llvm::sys::path::native should do
> the trick. Thanks for figuring this out!

Done in r224164. I've also added a TODO about the .. thing.