Skip to content
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

feat: improve is_modifiable checks for unix os's #428

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

DerpDays
Copy link
Contributor

About

This will resolve #342, it improves the is_modifiable function for directories on unix systems by more accurately checking whether the user has write permission for the directory.

This PR changes the function so that it:

  • Checks the groups that the nvim process owner is a part of, instead of only the group that nvim was launched with, this is done using the id -G command (gets all gid's of groups that the user is in) and checking if directory gid is in this list.
    • note: the command output is cached upon first run if successful
    • note: if the command fails it falls back to the previous behavior of only checking if the directory gid equal to the nvim process gid
  • Accounts for weirder permission setups e.g. 170 and 117 (where the user is the owner of the directory) by still checking the group and others permission sets, and doing an bitwise OR with all of the permission sets that they match.

Questions

  • Is the way I went about the cache fine? I saw that the current cache implementation was based around paths, however since the command id -G is independent of directories (based on the nvim process user), I just added an extra variable to the cache module with functions to get and set it.
  • With the current cache, even if the command fails for one directory it will try again for the next directory the user enters, should I just make it so that if it fails the first time to not try again, or can I leave it as is?

@github-actions github-actions bot requested a review from stevearc June 20, 2024 05:56
@stevearc
Copy link
Owner

I refactored the caching logic a bit because we didn't need it to be as fancy. LMK if this looks good to you and I'll merge

@DerpDays
Copy link
Contributor Author

Looks good to me! I had kept it in the cache utility since I wasn't sure on how you'd want it, but this way is a lot easier to follow and nicer!

@stevearc stevearc merged commit 65c53db into stevearc:master Jun 21, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: set modifiable to true if the user is in the directory group
2 participants