-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
cp: fail when trying to copy to read only file on mac #5261
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
cp: fail when trying to copy to read only file on mac #5261
Conversation
|
Cool! Thanks! Since you're on mac, would you be able to figure out if this is at all related to the |
|
Looking at the man page for cp on mac it looks like there is a flag to use clonefile for the copy |
|
Interesting, I can't find |
|
Can't find it online but here what man cp on my mac shows |
|
|
|
reading through reflink behavior it seems like these changes should match whats expected in gnu cp. Seems like |
tertsdiepraam
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.
Hi! Thanks for picking this up again! This is a trickier issue than it might appear, because the whole file removal thing might be wrong...
For example, the linux equivalent first opens the files for reading, before using a syscall. Maybe that's the better way to approach this. For example using fclonefileat instead of clonefile. It's a bit unfortunate I don't have a mac, so I can't check this for myself. Could you maybe check whether that would work?
If not then I think we should merge (a simplified version) of this patch, because it still fixes an important issue, maybe with a comment to state that it is a workaround and that this code should be expanded and rewritten.
|
No problem happy to help out! I can take a look at reimplementing this to match the Linux version better this week. |
|
looking at |
|
Hi! Sorry for not responding for a while, I was very busy... Anyway, let's pick this back up! I think the |
|
Yeah, I think merging this first is good. Should be ready to go |
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.
Sounds good! Do you want to pick up that followup PR as well? I made a new issue for this, if you're interested in picking this up just respond there.
- Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR uutils#5261 cannot regress - Tests provide evidence for closing issue uutils#5349
- Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR uutils#5261 cannot regress - Tests provide evidence for closing issue uutils#5349
- Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR uutils#5261 cannot regress - Tests provide evidence for closing issue uutils#5349
* feat: add comprehensive readonly file regression tests for cp - Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR #5261 cannot regress - Tests provide evidence for closing issue #5349 * perf: optimize readonly regression tests with batched I/O operations - Reduce file I/O overhead by batching file operations - Consolidate setup operations to minimize system calls - Improve test execution time from 0.44s to 0.27s (38% improvement) - Maintain comprehensive test coverage for readonly file behavior * fix: remove duplicate tests and trivial comments per PR feedback - Remove test_cp_readonly_dest_regression (duplicate of test_cp_dest_no_permissions) - Remove test_cp_readonly_dest_with_force (duplicate of test_cp_arg_force) - Remove test_cp_readonly_dest_with_remove_destination (duplicate of test_cp_arg_remove_destination) - Remove test_cp_macos_clonefile_readonly (duplicate of test_cp_existing_target) - Remove test_cp_normal_copy_still_works (duplicate of test_cp_existing_target) - Remove trivial performance comments from readonly tests - Keep existing proven tests per maintainer preferences - Keep unique readonly tests that provide additional coverage
* feat: add comprehensive readonly file regression tests for cp - Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR uutils#5261 cannot regress - Tests provide evidence for closing issue uutils#5349 * perf: optimize readonly regression tests with batched I/O operations - Reduce file I/O overhead by batching file operations - Consolidate setup operations to minimize system calls - Improve test execution time from 0.44s to 0.27s (38% improvement) - Maintain comprehensive test coverage for readonly file behavior * fix: remove duplicate tests and trivial comments per PR feedback - Remove test_cp_readonly_dest_regression (duplicate of test_cp_dest_no_permissions) - Remove test_cp_readonly_dest_with_force (duplicate of test_cp_arg_force) - Remove test_cp_readonly_dest_with_remove_destination (duplicate of test_cp_arg_remove_destination) - Remove test_cp_macos_clonefile_readonly (duplicate of test_cp_existing_target) - Remove test_cp_normal_copy_still_works (duplicate of test_cp_existing_target) - Remove trivial performance comments from readonly tests - Keep existing proven tests per maintainer preferences - Keep unique readonly tests that provide additional coverage
* feat: add comprehensive readonly file regression tests for cp - Add 10 new test functions covering readonly destination behavior - Tests cover basic readonly copying, flag combinations, and edge cases - Include macOS-specific clonefile behavior tests - Ensure readonly file protection from PR uutils#5261 cannot regress - Tests provide evidence for closing issue uutils#5349 * perf: optimize readonly regression tests with batched I/O operations - Reduce file I/O overhead by batching file operations - Consolidate setup operations to minimize system calls - Improve test execution time from 0.44s to 0.27s (38% improvement) - Maintain comprehensive test coverage for readonly file behavior * fix: remove duplicate tests and trivial comments per PR feedback - Remove test_cp_readonly_dest_regression (duplicate of test_cp_dest_no_permissions) - Remove test_cp_readonly_dest_with_force (duplicate of test_cp_arg_force) - Remove test_cp_readonly_dest_with_remove_destination (duplicate of test_cp_arg_remove_destination) - Remove test_cp_macos_clonefile_readonly (duplicate of test_cp_existing_target) - Remove test_cp_normal_copy_still_works (duplicate of test_cp_existing_target) - Remove trivial performance comments from readonly tests - Keep existing proven tests per maintainer preferences - Keep unique readonly tests that provide additional coverage
Fixes issue #5257 by first checking if file is read only or not before removing file and trying clonefile again