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

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

Merged
merged 1 commit into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@LeonardoBraga
Contributor

LeonardoBraga commented Jun 20, 2018

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.

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 21, 2018

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... @LeonardoBraga Can't you make the fix at a less generic level but the place in which the .gitignore file is saved?

@LeonardoBraga

This comment has been minimized.

Contributor

LeonardoBraga commented Jun 21, 2018

@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.

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 22, 2018

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 (@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.

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 22, 2018

could be this file of code:

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 22, 2018

@LeonardoBraga

This comment has been minimized.

Contributor

LeonardoBraga commented Jun 22, 2018

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:

capture

@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 @bpasero

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 22, 2018

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

@LeonardoBraga

This comment has been minimized.

Contributor

LeonardoBraga commented Jun 22, 2018

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

@LeonardoBraga

This comment has been minimized.

Contributor

LeonardoBraga commented Jun 23, 2018

@bpasero @jrieken I isolated the changes around TextFileService, as suggested. Let me know if you have any additional feedback.

@bpasero bpasero modified the milestones: July 2018, June 2018 Jun 25, 2018

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 25, 2018

Looks good to me but I'll leave it up to @bpasero

@jrieken jrieken removed their assignment Jun 25, 2018

@bpasero

@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
@LeonardoBraga

This comment has been minimized.

Contributor

LeonardoBraga commented Jun 25, 2018

@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.

@bpasero bpasero merged commit 1ec986b into Microsoft:master Jun 26, 2018

2 checks passed

VSTS: VS Code 20180625.79 succeeded
Details
license/cla All CLA requirements met.
Details
@bpasero

This comment has been minimized.

Member

bpasero commented Jun 26, 2018

@LeonardoBraga thanks 👍

@LeonardoBraga LeonardoBraga deleted the LeonardoBraga:improves-support-to-unsaved-unc-files branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment