Add a setting to mark file(s) readonly [nonEditable]

[jackpunt]

fix for: #161715 (and 98% of #4873; if we pull branch files-readonly-active with readonlyToggle then 100%)
see discussion at the bottom of that issue for why this simpler approach; also a lot of edits required)

Enable users to mark files as readonly using include/exclude globs in settings.
(independent of OS-filesystem writability)

So users can avoid accidental typing/edits into files that are OS-writeable, but semantically intended to be read-only.
For example: build/tsc.out *.js files, log files, protogen output files, etc.

Test: open a text file ('foo.any'); observe it is editable/writeable.
In settings.json, add glob"files.readonlyInclude": { "**/foo.any": true } observe that Editor is marked readonly.
In settings.json, add glob "files.readonlyExclude": { "**/foo.any": true } observe that Editor is marked writeable.

[bpasero]

Indeed, you clearly see more touch-points than are dreamt of in my philosophy; happy to transition to your PR.

Thanks 🙏

Note: Emacs has had ^X-^Q (toggle-readonly) for decades, it is widely used, and does not cause confusion

And my PR provides something similar, through a change in settings. We could in theory change this to only operate on the current session and not having it in settings, but I would think that users most likely want VS Code to remember this choice even between restarts and settings is currently the best approach to achieve that.

TTBOMK, regexps/globs do not have a user-friendly way to express exceptions to a general rule

Yes, currently. We have #134415 that discusses a way to configure both excludes and includes from a single glob pattern, but we are not making progress there in the near term. I think as such I will keep having 2 settings for readonly includes and excludes.

[jackpunt]

And my PR provides something similar, through a change in settings.

My original implementation did that; but after seeing a dozen notations accumulating in the setting file, I changed it be self-deleting. But not a big deal either way.

[bpasero]

I think #181708 is good to be merged, will discuss this week with the team. If you have any feedback from trying it out, let me know. I would also ask for feedback in #4873 once we have an insiders build with this change.

[jackpunt]

Still trying for a clean compile, but reading your PR/diff I see:

isReadonly(resource: URI): boolean {
		if (this.readonlyExcludeMatcher.value.matches(resource)) {
			return false; // exclude always wins over include
		}
		return this.readonlyIncludeMatcher.value.matches(resource) ?? false;
	}

The [original] logic was:
IncludeMatcher...matches(resource) && !ExcludeMatcher...matches(resource)

The [new] logic appears to be semantically different from the original. Consider:
readonlyInclude: '**/foo/*, **/bar/*' and readonlyExclude '**/*.ini'

In the original, the "Exclude" only applies if the files would otherwise be included.
With [original]: '~/typical.ini' would NOT be affected, but '~/foo/dev.ini' would be readonlyExclude
With [new] '~/typical.ini' would be indicates as readonlyExclude, even if it is chmod -w.

The [new] scheme would require the Excludes to re-identify each 'Include' directory...
readonlyInclude: '**/foo/*, **/bar/*' and readonlyExclude '**/foo/*.ini', '**/bar/*.ini'

One can debate which form is easy or obvious, but I want to note that it appears to have changed semantics.
The [original] is modeled after tsc config Include/Exclude

[bpasero]

@jackpunt wow that is a great catch and kudos to spotting that 👍 . Thinking about it more, I am fine to going with your suggestion, given it seems to be standard practice in other cases such as TypeScript.

I pushed 350797f to change that.

Let me know if you need help running the PR.

[jackpunt]

[After a 'yarn' to get something of typescript updated, it compiles]
Runs from scripts/code.sh, but does not start from "Run and Debug" |> VS Code
"Could not find the task 'Ensure Prelaunch Dependencies'."

Can you say what is the command (or whatever) to toggle/set/clear the abs path?
Also: where are user/settings.json when running the compiled 'Code - OSS' ?
.profile-oss/User/settings.json had the older, now-unsupported "files.readonlyPath"

PS: it's delightful to have you awake/online/in-real-time... !

---

Found "Toggle Active Editor Readonly", bound it to a key:
"Unable to change readonly state of the active editor. Please ... update ... manually."
Is this the intended result?
Update: That was likely my profile settings with "files.readonlyPath" setting unwriteable.

Hmm, so the command tried to work once, the first time;
and overwrote my workspace's settings.json: files.readonlyInclude
Removing, for ex: "node_modules/**" : "true",

Update:
Found it: need to use updateValue(..., ConfigurationTarget.USER)
To put configurationToAdd.user?values back in to USER settings.json

While I'm thinking of it: my extension/Command also refused to set/toggle for files named 'settings.json'
because if you set/toggle it to be 'true' then it becomes difficult to set/reset anything else.
Not totally 'elegant' but the first time you hit the toggle-readonly while in settings.json, you add that feature...

[jackpunt]

After viewing async updateReadonly(resource: URI, readonly: true | false | 'toggle')

Another comment: updateReadonly(false) is not the same "remove readonly setting"
In the original, there were 4 values: true, false, 'toggle', and null (or any other token to indicate 'remove')

The point being: sometimes you want a toggle or 'false' to override chmod -w, or readonlyInclude/Exclude
And then later, you want to rescind that and get back to the usual/default value,
and at that point you would updateReadonly('remove', ...).

Likewise, updateReadonly(false,...) needs to be retained so it continues to override a readonlyInclude/Exclude setting;
the current code appears to save 'true' values and remove all 'false' values.

---

The original code was like this [for my own reminder/reference]:

  /**
   * Exact path can specify: true, false, toggle [flip] or null [ignore].
   * Used by interactive command/keybinding to set, clear or toggle isReadonly.
   */
  protected pathGlobsChanged() {
    const readonlyPath = this.configurationService.getValue<{ [glob: string]: boolean | null | 'toggle' }>(GlobManager.path);
    if (readonlyPath !== undefined) {
      const pathValue = readonlyPath[this.resource.path]; // specifically *this* path. NOT in other workspaces...
      // undefined indicates no mention of path; Command sets to 'null' removes from config file --> undefined
      if (pathValue !== undefined) {
        if (pathValue === 'toggle') {
          // modify settings, so subsequent 'toggle' will be seen as a change:
          this.pathReadonly = readonlyPath[this.resource.path] = !this.oldReadonly;
          this.configurationService.updateValue(GlobManager.path, readonlyPath, ConfigurationTarget.USER);
        } else {
          this.pathReadonly = pathValue;
        }
      }
    }
    this.isReadonly(); // fire event if/when onDidChangeReadonly
  }

I'll try hacking in a 'clear' value to trigger removal...

[jackpunt]

About isReadonly() {...} I foresee a problem.
It appears that if configuredReadonlyFromPermissions is true,
then there is no way to make a chmod -w file editable?
The intent was to see the file initially/default as non-editable,
but a user could toggleReadonly (or set readonlyInclude Glob)
to obtain the "let me edit and I'll try to override the file-mode"

May explain some of the "Unable to change readonly state of the active editor." ??

I don't exactly what stat?.locked implies, but the doc says:
"... editors should offer to edit the contents and ask the user upon saving to remove the lock"
But it appears that isReadonly() will always be true, so the user cannot edit the contents... ?

The #161716 logic was like:

isReadonly() {
  if (is a readonly Provider) return true;
  if (specific path is specified, from the Command) return the specified value;
  if (readonlyInclude matches) return that value, as modified by readonlyExclude;
  if (configuredReadonlyFromPermissions == false) return false; [the 'legacy' behavior]
  else return value from actual stat.readonly bit;
}

#181708 code:

  isReadonly(resource: URI, stat?: IFileStatWithMetadata): boolean {
    if (this.configuredReadonlyFromPermissions && stat?.locked) {
      return true; // leverage file permissions if configured as such
    }

    return this.readonlyIncludeMatcher.value.matches(resource) && !this.readonlyExcludeMatcher.value.matches(resource);
  }

[jackpunt]

The previous comment exposes another concern:
Including the Command value in the readonlyInclude setting
means that the Command value can be overridden by the readonlyExclude setting.

To continue the original example, with workspace settings:
"readonlyInclude": '**/foo/*, **/bar/*' , "readonlyExclude" '**/*.ini'

A user will find it difficult to set or toggle '**/foo/example.ini'
My answer was to use a separate config setting name for the Command.

maybe one could scan readonlyInclude to find the 'single, absolute path' Globs,
and apply those differentially, but having a separate setting seems cleaner & easier.
(esp as Users can know that the Command 'setting' is independent of and overrides the include/exclude values)

[bpasero]

@jackpunt good stuff, I have pushed 1976534 to address your feedback.

For one, the configuration is ignored for children of userRoamingDataHome where settings are stored. Even if you configured e.g. **/*.json to be readonly, you would still be able to edit settings file.

Furthermore, I have rethought the commands to toggle readonly state ad-hoc and made them NOT write to settings anymore. Instead, they update an in-memory set of resources that are either readonly or writeable for the duration of the session. I think you have rightfully pointed out that writing to settings and thus changing user preference is quite tricky to get right in this case here. If a user really wants to persist readonly configuration, the user has to go through settings her/himself. This change also makes sure that a readonly file as configured by chmod can be made writeable, irrespective of any configured setting.

I also pushed 625d0a1: turns out that toggling readonly state for a file that is readonly via chmod on disk requires the toggle command to resolve the stat first, otherwise you need to invoke the command twice to turn such a file writeable.

Let me know if you find out more 👍

[jackpunt]

I just now saw your response!
As I came here to point you to my branch: https://github.com/jackpunt/vscode/tree/ben-readonly-files2

I still don't know what your configuredReadonlyFromPermissions && stat?.locked does, exactly,
but the other bits mostly work as I expect

I added the new setting for the Command override.
and added , ConfigurationTarget.USER where needed.

now I'll go look at your update...

---

quick peek: rename to InSession, and use a couple of Sets to track the state.
Not sure if the semantics of isReadonly() still hold; will check when I get some time.
Or you can compare to the isReadonly() in my branch which still uses the configurationService to set/hold/clear the values set by the Commands.

Do you get the Icon to update? [[ YES! ]]

---

Ah! now that the Command does not write the USER settings file, that check can come after the Command check.

Hmm, I might still want the ClearActiveEditorReadonlyInSession command...
So I can clear the Command setting/set, and see the effects of using chmod
As it is, I can't tell if the LockIcon is from the Command or the filesystem.

Note: in one of my previous versions, instead of 2 Sets, I used a Map<resource,boolean> to hold the command status.
(and 'clear' would just delete...)

[bpasero]

I still don't know what your configuredReadonlyFromPermissions && stat?.locked does, exactly,
but the other bits mostly work as I expect

I added a new FilePermission.Locked to convey the information of a file on disk having permissions configured that do not allow for writing. This is intentionally different from the existing FilePermission.Readonly: a locked file can still be edited in an editor and upon saving you get a notification asking to remove the lock and try again. There is a new setting files.readonlyFromPermissions that you can use to make the editor readonly. This is essentially the fix for #17670

Hmm, I might still want the ClearActiveEditorReadonlyInSession command

Yeah maybe it would be better if I tracked the in-session state in a Map as well that you can clear, I can look into that.

[bpasero]

@jackpunt change 97be614 introduces a new command to reset the readonly override and uses a ResourceMap for that purpose as you suggested.

[jackpunt]

Wow!
I think that now covers all the functional requirements.
(although I haven't yet tested all this)

One stylistic bit: in files.contributions you have:

    [FILES_READONLY_EXCLUDE_CONFIG]: {
      'type': 'object',
      'patternProperties': {
        '.*': { 'type': 'boolean' }
      },
      'default': {},
      'markdownDescription': nls.localize('readonlyExclude', "Configure paths or [glob patterns](https://code.visualstudio.com/docs/editor/codebasics#_advanced-search-options) to exclude from being marked as read-only if they match as a result of the `#files.readonlyInclude#` setting."),
      'scope': ConfigurationScope.RESOURCE
      },
    [FILES_READONLY_FROM_PERMISSIONS_CONFIG]: {
      'type': 'boolean',
      'description': nls.localize('files.readonlyFromPermissions', "Marks files as readonly when their file permissions indicate as such."),
      'default': false
    },

FILES_READONLY_EXCLUDE_CONFIG is externally defined as 'files.readonlyExclude';
For nls.localize(key, message), the 'key strings' are
A) not derived/synchronized with the definition of FILES_READONLY_EXCLUDE_CONFIG
B) not consistent with including the 'files.' prefix
Maybe don't use the external constants (consistent with the rest of the file)
or use the external def'ns consistently: nls.localize(FILES_READONLY_EXCLUDE_CONFIG, ...
??

[bpasero]

Yeah the keys for nls do not matter that much actually, as long as there are no duplicates in the same file. I pushed a change to align them though.

Thanks for testing!

[jackpunt]

I hope you are working in Seattle this week, and not burning midnight oil in Zurich...

[jackpunt]

Oh! a possible semantic detail:

isReadonly() currently give chmod precedence over Inc/Exc Globs:

    const sessionReadonlyOverride = this.sessionReadonlyOverrides.get(resource);
    if (typeof sessionReadonlyOverride === 'boolean') {
      return sessionReadonlyOverride; // session override always wins
    } if (this.configuredReadonlyFromPermissions && stat?.locked) {
      return true; // leverage file permissions if configured as such
    }

    return this.readonlyIncludeMatcher.value.matches(resource) && !this.readonlyExcludeMatcher.value.matches(resource);

If I mostly want files to open based on the chmod permissions (so FromPermissions: true)
But there are a class of files that need/want to open as 'editable' by default,
then I would want the Globs to have precedence over chmod:

    const sessionReadonlyOverride = this.sessionReadonlyOverrides.get(resource);
    if (typeof sessionReadonlyOverride === 'boolean') {
      return sessionReadonlyOverride;           // determined by session override/Command
    }
    if (this.readonlyIncludeMatcher.value.matches(resource)) {
      return !this.readonlyExcludeMatcher.value.matches(resource);    // determined by Inc/Exc Globs
    }
    // determined by file permissions if configured as such:
    return (this.configuredReadonlyFromPermissions && stat?.locked)  

[bpasero]

@jackpunt that is fair. Can you actually do a PR targetting my branch in my PR so that you could just propose the change yourself and do it?

[jackpunt]

Ok. on it...

[jackpunt]

Before making that mod, i wonder about textFileEditorModel & storedFileWorkingCopy

  override isReadonly(): boolean {
    return this.lastResolvedFileStat?.readonly ||
      this.fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly) ||
      this.filesConfigurationService.isReadonly(this.resource, this.lastResolvedFileStat);
  }

It appears that a chmod -w FileStat.readonly cannot be influenced by filesConfigurationService.isReadonly(...)

I had been working on the theory that filesConfigurationService.isReadonly() was the full arbiter...
I think we need to remove the first check on this.lastResolvedFileStat?.readonly ||
and move it to the proper precedence in filesConfigurationService ??
[goes back to my confusion about stat?.locked, think that may have included the stat.readonly bit...]

[jackpunt]

PR submitted

[bpasero]

@jackpunt thanks, I merged it in but I had to add back the check for stat.readonly:

return (this.configuredReadonlyFromPermissions && stat?.locked) ?? stat?.readonly ?? false;

I think the precedence now is:

• file system provider readonly: true => file is readonly and you cannot change that
• user defined session override exists => file is readonly or writeable
• file is part of user data => file is writeable
• file is part of files.readonlyInclude => file is readonly unless files.readonlyExclude
• file is write locked (chmod...) and configured readonly from permissions => file is readonly
• file is readonly by file system provider => file is readonly
• finally => file is writeable

That is quite complex! I am happy with this precedence I think, but users might have other feedback.

One thing I wonder though: do we need to allow the user overrides to mark a file writeable even if the file system provider is readonly overall? Currently that check is still in the working copy:

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

Lines 1162 to 1165 in 05632b6

	override isReadonly(): boolean {
	return this.fileService.hasCapability(this.resource, FileSystemProviderCapabilities.Readonly) ||
	this.filesConfigurationService.isReadonly(this.resource, this.lastResolvedFileStat);
	}

To be consistent, we should probably move that into the files configuration service as well.

[jackpunt]

had to add back the check for stat.readonly:

Sounds right :( I clearly didn't test every case... <chagrin/>
(and was unclear how .locked related to .readonly)

I think your precedence chart is what I also arrived at. (although I didn't have the 'user data' test)
I don't know the distinction between bullets (1) and (6), is (6) is redundant?

file system provider readonly: true => file is readonly and you cannot change that
file is readonly by file system provider => file is readonly

Related to your question:

do we need to allow the user overrides to mark a file writeable even if the file system provider is readonly overall?

I thinks it's correct that FileSystemProviderCapabilities.Readonly prevents any ability to modify.
(that is: "no"; esp if/when the text has no backing store, or the Editor has no capability to edit)

Back when ReadonlyHelper was an adjunct to SFWC & TFEM, the checks were all in one place.
If you want to move that check back to isReadonly() and consolidate in one place, I'm expect that will work.

[bpasero]

I don't know the distinction between bullets (1) and (6), is (6) is redundant?

It is not: a file system provider can either signal that it is readonly entirely or that specific files are readonly and some are writeable, so there is difference, even though not visually for the user.

I thinks it's correct that FileSystemProviderCapabilities.Readonly prevents any ability to modify.
(that is: "no"; esp if/when the text has no backing store, or the Editor has no capability to edit)

Yeah I like that reasoning: if the entire file system provider is declaring to be readonly, it is likely that no write operation can ever happen. But if a single file is readonly, it indicates that the user maybe able to override that. So let's leave it like that.

[bpasero]

Since #181708 landed, I will go ahead and close this PR. Thanks a ton for your reviews 🙏 . We can still continue to tweak the experience going forward, it would only be released in stable early June.

I will also make sure to ask for user feedback in our nightly insiders release in the fixed issues.