vscode icon indicating copy to clipboard operation
vscode copied to clipboard

Stat all folders and files within appRoot as readonly (#138815)

Open gjsjohnmurray opened this issue 1 year ago • 7 comments

Second attempt at resolving #138815 to reduce the likelihood of a naive user storing their projects within the appRoot folder and then being upset when a VS Code update deletes their files.

This PR implements @bpasero's suggestion at https://github.com/microsoft/vscode/pull/155443#issuecomment-1191411763 but also makes Explorer play nice.

junk

gjsjohnmurray avatar Aug 15 '22 17:08 gjsjohnmurray

/assign @bpasero

gjsjohnmurray avatar Aug 15 '22 17:08 gjsjohnmurray

Submitting the PR has revealed that I've broken layering rules in the way I made Explorer work out whether to force metadata resolution so it gets to know when folders and files are read-only.

@bpasero please advise whether there's another way to achieve this.

gjsjohnmurray avatar Aug 15 '22 17:08 gjsjohnmurray

To avoid the layer breaker I have made Explorer get metadata for all file:// uris.

gjsjohnmurray avatar Aug 15 '22 21:08 gjsjohnmurray

Thanks, will review when I find time for it. @lramos15 fyi for explorer changes.

bpasero avatar Aug 16 '22 05:08 bpasero

Added a warning notification when workspace opens, similar to the one the previous PR had per-file:

image

gjsjohnmurray avatar Aug 17 '22 13:08 gjsjohnmurray

Sorry, will probably not have a lot of time this month, might push this back a bit.

bpasero avatar Aug 31 '22 08:08 bpasero

@bpasero @lramos15 there have been two new issues raised this week by Windows users who have lost all their files. Neither yet proven to be caused by an update, but please can this PR make progress as a way of preventing future incidents of that sort?

gjsjohnmurray avatar Sep 20 '22 05:09 gjsjohnmurray

@gjsjohnmurray I am not easy accepting this change, especially forcing requireMetadata will have a severe significant impact on explorer performance and this case here only impacts a handful of users.

How about a different approach where we simply open a notification if a user opens the installation dir or any child of that as a workspace folder? We could even leverage the banner for this to make it more prominent:

image

Maybe it could also be a modal dialog to be very annoying.

bpasero avatar Sep 22 '22 05:09 bpasero

@bpasero your verdict doesn't surprise me. I was uneasy about having to get metadata so unconditionally. Closing this. Please see #161534 instead.

gjsjohnmurray avatar Sep 22 '22 22:09 gjsjohnmurray