Improves support to Windows UNC files in a "yet-to-be-saved" state

[Leonardo Braga (LeonardoBraga)]

Fixes #49851

Improves support to Windows UNC files in a "yet-to-be-saved" state, which addresses the problem with creating .gitignore in a network folder from "Add to .gitignore" command.

[Johannes Rieken (jrieken)]

I am not very keen on changing the fsPath-semantics for the URI type because that is used a lot and worst, values of fsPath might be cached and must remain stable... Leonardo Braga (@LeonardoBraga) Can't you make the fix at a less generic level but the place in which the .gitignore file is saved?

[Leonardo Braga (LeonardoBraga)]

Johannes Rieken (@jrieken) I completely understand your concern, and that was not my initial attempt as well. I've been working on the codebase for just a few days, so I might obviously be missing something, but let me explain the reasoning behind the fix and then we could try to find a better path. Sorry for the long reply.

Assuming you have the following scenario:

• Empty folder located at \\some-server\some-share\test
• User executed git init inside test
• User created only one file, a.txt
• The folder does not have a .gitignore file

In the current flow, when the user opens the folder \\some-server\some-share\test in VSCode, goes to the SCM panel, right clicks a.txt and selects Add to .gitignore, VSCode will run the logic in https://github.com/Microsoft/vscode/blob/master/extensions/git/src/repository.ts#L876, that will check whether .gitignore already exists. It doesn't, so the document const will contain a reference to a URI with the scheme untitled. This allows the editor to open with a file that is still in-memory (it does not have the scheme set to file yet). As a user of VSCode, I really like this design decision, as it lets me "confirm" my action by saving the file after I evaluate the changes, or just close it without saving, going back to the state I was in.

The problem here is that the code does not support constructing the fsPath of a UNC resource from a URI that has a scheme different than file.

• First, I tried to force the scheme to be file instead of untitled, which fails as the file is not yet persisted. I gave up that path as it felt like I would need to change the semantics behind the scheme file to make it work before the file actually exists in the disk.
• Then I thought of creating the .gitignore file immediately, which would allow me to have a URI with the scheme file and the rest would just work, but this approach breaks the existing design and removes the ability for the user to go back to the previous state by just closing the file without saving it.
• Then I got to the code in this PR. Even though it is an internal change, based on what I saw in the code, the scheme only transitions from untitled to file, which would not be a problem. Also, from what I could gather, all the use cases where scheme and authority are involved besides UNC, are related to URLs (http or https), so I didn't see any risks. An added "bonus" is that if any similar use cases of in-memory files over UNC paths are required now or in the future, this improvement would enable them transparently.

Let me know if any of the other approaches sound better to you, or if you know about any other possible paths I could explore, I will definitely give them a shot.

[Johannes Rieken (jrieken)]

Let me know if any of the other approaches sound better to you, or if you know about any other possible paths I could explore,

Yeah, don't get me wrong. The change does look good but I am extremely conservative when it comes to uri-changes. It's huge source of bikeshedding and there will be many people arguing withs loads of passion why that change isn't good.

So, I wonder if we can tackle this at the place in which an untitled file is being saved. Somewhere (Benjamin Pasero (@bpasero) should know where) the untitled-uri must be turned into a file-uri and instead of calling fsPath it should construct an fsPath-equivalent from the components of the uri itself.

[Johannes Rieken (jrieken)]

could be this file of code: https://github.com/Microsoft/vscode/blob/b7e6e04666ca8dde560b69081c43365dce77d12a/src/vs/workbench/services/textfile/common/textFileService.ts#L446

[Johannes Rieken (jrieken)]

Reminds me of this series of debt-issues we went through: https://github.com/Microsoft/vscode/issues?utf8=✓&q=is%3Aissue+fsPath+%22Don%27t+call+URI%23fsPath%22+

[Leonardo Braga (LeonardoBraga)]

Just double-checking before implementing your suggestion, the use case I described above is also replicated in https://github.com/Microsoft/vscode/blob/master/extensions/typescript-language-features/src/utils/tsconfig.ts#L63-L64, and as the screenshot below shows, this patch also fixes that:

Johannes Rieken (@jrieken) Do you still prefer to address this use case on a case-by-case approach instead of a generic solution? If so, I no worries, I'll submit a new patch.

/cc Benjamin Pasero (@bpasero)

[Johannes Rieken (jrieken)]

Yeah, I believe a fix in the service for untitled files will also cover the TypeScript part.

[Leonardo Braga (LeonardoBraga)]

Understood! Let me work on that and I'll update the PR.

[Leonardo Braga (LeonardoBraga)]

Benjamin Pasero (@bpasero) Johannes Rieken (@jrieken) I isolated the changes around TextFileService, as suggested. Let me know if you have any additional feedback.

[Johannes Rieken (jrieken)]

Looks good to me but I'll leave it up to Benjamin Pasero (@bpasero)

[Benjamin Pasero (bpasero)]

Leonardo Braga (@LeonardoBraga) good catches, I think the fix goes into the right direction with 2 comments:

• there seems to be a third case where we do the wrong case here
• instead of doing URI.file(resource.fsPath).with({ authority: resource.authority }) I would prefer if we did resource.with({ schema: Schemas.file }) because that makes the conversion much clearer imho

[Leonardo Braga (LeonardoBraga)]

Benjamin Pasero (@bpasero) completely agreed with the recommendation and thanks for pointing out another place that needed the fix. I've updated the PR with the changes you suggested.

[Benjamin Pasero (bpasero)]

Leonardo Braga (@LeonardoBraga) thanks 👍