Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Case-sensitivity and file system providers #48258

Closed
jrieken opened this issue Apr 20, 2018 · 12 comments
Closed

Case-sensitivity and file system providers #48258

jrieken opened this issue Apr 20, 2018 · 12 comments
Assignees
Labels
remote Remote system operations issues under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Apr 20, 2018

Today we seems to have hardcoded logic to know when paths should be compared case-sensitive or not, e.g. callers of isEqualOrParent or isEqual and likely many more...

With b0ede6c I have added a file system provider flag/capability to signal that it is case-sensitive (default is case-insensitive). We need to come up with a strategy to communicate that information to consumers, e.g. the explorer or file events so that we do the correct thing.

A sample of failure is part of the memos-sample which creates to files that only differ in casing: https://github.com/Microsoft/vscode-extension-samples/blob/58ea8617190ee5566e001b9875b8b2c25a983857/fsprovider-sample/src/extension.ts#L30

@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach remote Remote system operations issues labels Apr 20, 2018
jrieken added a commit that referenced this issue Apr 20, 2018
@bpasero
Copy link
Member

bpasero commented Apr 20, 2018

I guess we would need some service that can compare resources which has all the knowledge of the capabilities of the file system. Possible inputs to this service are the capabilities of the remote file system, and then for file://, the OS and possibly smarter things like having a real understanding of the file system. As a first step we could have a stupid logic in the service for file:// where we simply assume Linux is case-sensitive and the others are not.

@isidorn
Copy link
Contributor

isidorn commented Apr 20, 2018

Introducing such a service makes sense but could it also be a part of the FileService?
I could look into adding this once we decide where this should belong. And I like the idea of starting with stupid logic which respects what we currently have.

@jrieken
Copy link
Member Author

jrieken commented Apr 20, 2018

Introducing such a service makes sense but could it also be a part of the FileService?

👍 for adding this to the FileService, it is somewhat similar to canHandleResource

@bpasero
Copy link
Member

bpasero commented Apr 20, 2018

We can start adding it there. The challenge will be to get access to this service from all places where we need to compare URIs. Also, I guess comparing 2 resources with different schemes should not be supported at all.

@isidorn
Copy link
Contributor

isidorn commented Apr 20, 2018

Assigning to May so we investigate next milestone.

@isidorn isidorn added this to the May 2018 milestone Apr 20, 2018
@jrieken
Copy link
Member Author

jrieken commented Apr 20, 2018

Also, I guess comparing 2 resources with different schemes should not be supported at all.

Well, not supported as they are inequal by definition. So, compare scheme, then authority (both always case-insensitive according to my knowledge), and then compare path (case-sensitive or not) and optionally compare query and ignore fragment.

@isidorn
Copy link
Contributor

isidorn commented May 18, 2018

We were focused on grid this milestone, next week I am on vacation, this is not for endgame -> June

@isidorn isidorn modified the milestones: May 2018, June 2018 May 18, 2018
@bpasero bpasero removed their assignment Jun 12, 2018
@isidorn
Copy link
Contributor

isidorn commented Jun 13, 2018

I plan to start investigating into this tomorrow by:

  • updating the IFileService to have areEqual(first: uri, second: uri): boolean
  • Implementing this in our FileService to simply respect the current platform
  • adopt all the path comparisons to use the IFileService for comparison - it might happen that some clients of this will not have an instance of the IFileService as @bpasero pointer out - but we see

@jrieken
Copy link
Member Author

jrieken commented Jun 13, 2018

Maybe a more portable Comparator would help, e.g. fileService.getUriComparator(): (a: URI, b: URI) => number. Once acquired we can pass the comparator around and use it in other places

@isidorn
Copy link
Contributor

isidorn commented Jun 25, 2018

Did not have time this milestone -> July

@isidorn isidorn modified the milestones: June 2018, July 2018 Jun 25, 2018
@isidorn isidorn modified the milestones: July 2018, August 2018 Jul 27, 2018
@isidorn
Copy link
Contributor

isidorn commented Aug 24, 2018

Does not seem like this will happen in the last couple days of the milestone (risky) -> september

@isidorn
Copy link
Contributor

isidorn commented Dec 17, 2019

I have pushed the changes in the Explorer such that the Explorer will now respect the FileSystemProvider case sensitive capability. Explorer also reacts on the change of this capability.
I know that this does not solve all the issues, but should in theory help with many issues as @aeschli suggested.
So let's try it out with insiders from tomomrrow and for now closing this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
remote Remote system operations issues under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

4 participants