Remove chmod and duplicated permissions functions #3782
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3782 +/- ##
==========================================
- Coverage 89.02% 88.99% -0.04%
==========================================
Files 255 255
Lines 14583 14579 -4
==========================================
- Hits 12982 12974 -8
- Misses 1601 1605 +4 ☔ View full report in Codecov by Sentry. |
Sploder12
force-pushed
the
fix-platform-specific-functions-in-abstraction
branch
from
6c58cd2 to
a5015cd
Compare
Sploder12
force-pushed
the
fix-platform-specific-functions-in-abstraction
branch
from
5b088d8 to
d50d4e4
Compare
| return ::chmod(path, mode); | ||
| } | ||
| std::error_code ec{}; | ||
| std::filesystem::permissions(path, permissions, ec); |
There was a problem hiding this comment.
Should we maybe do something with the error code instead of throwing it away? Maybe log or change the signature to return the code?
There was a problem hiding this comment.
If not, std::error_code overloads the bool operator, so you shouldn't need the static cast.
There was a problem hiding this comment.
changing the signature wouldn't work since Windows can fail for reasons that don't create or map to an error_code. But logging sounds like a good idea!
| @@ -296,15 +296,15 @@ mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, QFileDevice | |||
| throw std::runtime_error(fmt::format("unable to create directory '{}'", dir_path)); | |||
| } | |||
|
|
|||
| if (permissions) | |||
| if (permissions != std::filesystem::perms::none) | |||
There was a problem hiding this comment.
permissions is an enum where std::filesystem::perms::none is 0, so this isn't strictly necessary.
There was a problem hiding this comment.
The enum is not implicitly convertible to bool, I prefer this over an explicit cast to bool
tests/linux/test_platform_linux.cpp
Outdated
| EXPECT_CALL(*mock_file_ops, permissions(_)).WillOnce(Return(QFileDevice::ReadOwner | QFileDevice::WriteOwner)); | ||
| EXPECT_CALL(*mock_file_ops, setPermissions(_, _)).WillOnce(Return(true)); | ||
| EXPECT_CALL(*mock_file_ops, get_permissions(_)) | ||
| .WillOnce(Return(std::filesystem::perms::owner_read | std::filesystem::perms::owner_write)); |
There was a problem hiding this comment.
Since you're not testing the permissions and its a mock, you should be able to just Return(), no?
There was a problem hiding this comment.
Return() won't work since get_permissions doesn't return void, I chose owner read write since that's how the test was originally with QPermissions. But any std perms should be fine.
tests/linux/test_platform_linux.cpp
Outdated
| EXPECT_CALL(*mock_file_ops, permissions(_)).WillOnce(Return(QFileDevice::ReadOwner | QFileDevice::WriteOwner)); | ||
| EXPECT_CALL(*mock_file_ops, setPermissions(_, _)).WillOnce(Return(false)); | ||
| EXPECT_CALL(*mock_file_ops, get_permissions(_)) | ||
| .WillOnce(Return(std::filesystem::perms::owner_read | std::filesystem::perms::owner_write)); |
Sploder12
force-pushed
the
fix-platform-specific-functions-in-abstraction
branch
from
024baf7 to
48de023
Compare
This PR removes
chmodfromPlatform.has well as functions that are similar infile_ops.h. They have all been changed to useMP_PLATFORM.set_permissions. This should not change behavior on Unix systems. On Windows this changes the behavior to modify ACLs as well as DOS permissions instead of just DOS permissions.MULTI-1420