Enable FileSystemProvider to stat a file as readonly

Currently a FileSystemProvider can register as readonly, meaning that every file it provides is opened in a readonly editor. This is nice, but even better if the FileStat it returns from its stat() method had an optional readonly property. Then I think this line could easily treat a true readonly value as a signal to set isReadonly true. @bpasero what do you think?

I think this is generally a nice idea, but I am worried that it will make file operations very complicated: imagine a folder where just a few files are readonly. Now a fs operation runs that operates on all files in the folder, not just one (e.g. rename of the folder? delete?). Naturally with this we would first have to ask the provider if any of the files that are potentially modified are readonly before even starting the operation, which seems quite crazy to me.

Fair point, but what happens at the moment in such scenarios if the user has set restrictive OS-level file permissions to protect just a few of the files in a folder tree? Does VSCode already run a preflight check on, say, a parent folder delete and decline to start the operation if any of the descendant files isn't deletable?

@gjsjohnmurray yeah we are not doing anything but rely fully on the OS to do the right thing. If one file in a folder is write protected, the OS may decide that a rename on the parent folder is not possible (have not validated that).

@bpasero I'd also argue that it shouldn't be VSCode's responsibility to recursively check the readonly stat of every file within a folder before asking the FileSystemProvider to delete the folder. The FSP is best placed to know if that's an appropriate check to make. For example my FSP might simply fail the folder delete request immediately because it's a root folder. I certainly don't want you to have wasted time (yours and mine) walking the entire tree and asking me to stat each file for you if I'm always going to reject your subsequent request to delete the root folder.

@gjsjohnmurray yeah good point 👍

Please can this issue get some love soon?

@jrieken are you open for a PR to implement this? I'm guessing it needs to be treated as proposed API. I'd copy the FileStat interface into vscode.proposed.d.ts, add readonly?: boolean, and wire this up. That has become a bit more complicated since @bpasero did some restructuring in 920d80e but ultimately the new readonly from FileStat would have its effect here.

vscode/src/vs/workbench/services/textfile/common/textFileEditorModel.ts

Line 1031 in 8134d8c

    
           return this.fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly);

Proposing API is usually the very last step and requires an "internal" API first and I don't think there is one. I believe that would be around here and than needs options for editors, custom editor etc

I have submitted draft PR #111237 for comment and guidance.

One thing to note is that if we decide to push this, the experience for readonly files will be different from how we treat OS readonly files. We would probably not adopt this for OS local files to preserve this current imho inferior behaviour:

you can always type in readonly files, even if they are readonly
you can save readonly files and be asked to overwrite the write protection at that point
you can still decide to "Save As" to a different file name to avoid readonly'ess

With this solution all you see is a readonly editor.

Little update to #73122 (comment): with 5a8936a there is a new notion of FileWriteLock errors that a provider can throw and a new unlock write capability. As such, the feature of unlocking write protected files is fully independent from "readonly" notion. As such I think it would be possible to allow for readonly files that turn the editor readonly without impacting the scenario of write protected files.

Timing wise I cannot make any better promises than tackling this in our issue grooming iteration.

Some API ideas:

Implemented by @gjsjohnmurray

/**
 * The `FileStat`-type represents metadata about a file
 */
export interface FileStat {
	/**
	 * The file is readonly.
	 */
	readonly?: boolean;
}

Suggested by @jrieken

/**
 * The `FileStat`-type represents metadata about a file
 */
export interface FileStat {
	/**
	 * Permissions of the file.
	 */
	permissions?: {
		
		/**
		 * The file is readonly.
		 */
		readonly?: boolean;
	};
}

Suggested by @bpasero

export enum FileStatMode {
	/**
	 * The file is readonly.
	 */
	Readonly = 1,
}

/**
 * The `FileStat`-type represents metadata about a file
 */
export interface FileStat {

	/**
	 * The mode of the file, e.g. when the file is readonly.
	 *
	 * *Note:* This value might be a bitmask, e.g. `FileStatMode.Readonly | FileStatMode.Other`.
	 */
	mode?: FileStatMode;
}

I like FileStatMode but we should make sure to use the same numbers as unix mode. That makes it simple to implement because most underlying API provide the mode-mask already

@jrieken hm interesting idea, but I guess there are multiple modes on Unix that can lead to a file being readonly, e.g.:

you are not the owner and only owners can write
you are not in the group of owners and only group owners can write
the file is globally not writeable (only with sudo?)

There is readable, writeable, executable and that is repeated for the owner, the group of the owner, and everyone else. https://en.wikipedia.org/wiki/File-system_permissions#Notation_of_traditional_Unix_permissions. Tho, in mode there is also the type of the file

Proposal after API call:

export enum FilePermission {
	/**
	 * The file is readonly.
	 */
	Readonly = 1
}

/**
 * The `FileStat`-type represents metadata about a file
 */
export interface FileStat {

	/**
	 * The permissions of the file, e.g. whether the file is readonly.
	 *
	 * *Note:* This value might be a bitmask, e.g. `FilePermission.Readonly | FilePermission.Other`.
	 */
	permissions?: FilePermission;
}

Let's keep this open to have a single item for API proposal to finalization

@bpasero when retrospecively reviewing your much-appreciated updates to my original PR I noticed this:

vscode/src/vs/platform/files/test/electron-browser/diskFileService.test.ts

Lines 92 to 94 in c4f3f8f

    
           setReadonly(enabled: boolean): void { 
        
           	this.readonly = true; 
        
           }

Is there a reason why line 93 isn't as follows?

this.readonly = enabled;

@gjsjohnmurray yeah good point, I can change that.

Btw 23dc403 has a couple more changes that were needed. I forgot that in many places we do a check for readonly on the file system provider, but now with individual files potentially being readonly, that check needs to change.

I think this shipped in 1.57 so can be closed, right?

https://code.visualstudio.com/updates/v1_57#_enable-file-system-providers-to-declare-a-file-as-readonly

@gjsjohnmurray it is only proposed API, not finalized yet.

@bpasero sorry, I misread the 1.57 release notes. I often find I miss the point where the "Proposed Extension APIs" section begins. Maybe the text of each subsection of that could helpfully reiterate "proposed API" in future release notes?

Verification

Test that you can mark a file as readonly without enabling proposed API via the permissions property on a file. #124846 has details how to use it.

@gjsjohnmurray if you are able to verify in current insiders, that would be ideal 👍

I hope this isn't considered an annoying question, but what is the process for an api change like this to graduate out of the insiders/proposed status?

Not at all, see: https://github.com/microsoft/vscode/wiki/Extension-API-process

@bpasero thanks, I was already clicking through the wiki, but seemed to have missed that.

So without jumping on calls, is there a way I can help move this forward? The change seems quite minimal, and would help 2 extensions I'm developing (both are languages with their own virtual file systems for some parts).

I saw this list, in the wiki:

Is there a non-trivial usage-sample for this new API?
Are there multiple (ideally not similar) use-cases that this proposal solves?
Is the proposal too narrow (not solving enough) or too broad (trying to solve too much)?
Who would use this new API?

And I'm not completely sure where this should be discussed? Should I raise a new issue, there are already quite a few issues about this topic on file specific read-only status.

@DavyLandman this API will be available in our upcoming release next week.

Thanks @bpasero!

I believe this was verified by #124846?

@gjsjohnmurray if you are able to verify in current insiders, that would be ideal 👍

/verified

First I had to work out how to make fsprovider-sample use the new API because it is no longer in vscode.proposed.d.ts but not yet published in @types/vscode