-
Notifications
You must be signed in to change notification settings - Fork 424
fix: Solve bug when changing permissions with no modification right #4107
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
base: main
Are you sure you want to change the base?
fix: Solve bug when changing permissions with no modification right #4107
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4107 +/- ##
==========================================
+ Coverage 63.58% 63.63% +0.05%
==========================================
Files 315 315
Lines 38562 38621 +59
Branches 2950 2954 +4
==========================================
+ Hits 24518 24578 +60
+ Misses 13975 13974 -1
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Note that I am not responsible for the two lines in question. They were here before, thus already not tested. |
Klaim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in essence but I would need a bit more documentation of the function.
As for the CI error on Windows, I'm not totally sure but I suspect the specific check is about a kind of permission not supported by that OS? Not sure.
| return std::filesystem::last_write_time(path, new_time, std::forward<OtherArgs>(args)...); | ||
| } | ||
|
|
||
| // Check if we have modification rights on a path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add:
- a clarification that this function is not part of
std::filesystem(mamba::fsnamespace reflectsstd::filesystem, see the comment at the top of the file for why); - clarifications on the preconditions, in particular what happens if the provided path doesnt lead to a file or directory;
|
|
||
| #ifndef _WIN32 | ||
| #include <fcntl.h> | ||
| #include <grp.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these headers required if you're using only the C++ standard library api?
Description
When trying to set execution mode for a file or setting permissions to cache directory, an error may occur if we have no modification right on the parent folder.
This error occurs regardless of the current permissions on the targeted file or folder.
Thus, even if we already have the required permissions on the targeted file or folder, the error occurs.
This pull request introduces a function
has_permissions()that tests if we already have the wanted permissions on a file or folder.This new function is used at the two places described above, in order to avoid a useless setting of permissions that would lead to an error.
Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.