Mark files as read-only when copying them into the archive. #141
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plietar
force-pushed
the
read-only-archive
branch
from
14d26e1 to
85f8b40
Compare
|
Looks like Windows is genuinely failing tests for me, not just #142. Will look into it. |
plietar
force-pushed
the
read-only-archive
branch
3 times, most recently
from
16685dd to
ca25304
Compare
It is easy for a user to inspect a packet's result by opening it up in their editor and accidentally edit and save the file. If they didn't have a file store and no other packet contains a copy of the file then the packet may be irremediably corrupted. Marking the file as read-only is an easy way to prevent this. Of course a user can always force their way around it, but this should at least prevent most accidental occurrences. A side-effect of this change is that file copied out of the archive will now be read-only as well, since copying the files preserves the access mode. This is desirable when copying files from a dependency, but may not be when copying outside of a packet context. If this causes problems we can introduce some option to `orderly_copy_files` to add the write bit back on files it copies. Files that were put into the file store were already being marked as read-only, so this change makes the two implementations more consistent. The executable bit was being cleared as well, which seems unnecessary to me and in some contexts even wrong (a user may well want to include a bash script in their report). I've changed the chmod operation from "a-wx" to "a-w". This doesn't matter on Windows, where executable bits don't exist. A few places around the codebase were using `unlink(recursive=TRUE)` to remove directories, which on Windows fails silently if the directory contains read-only files. One example of this was the temporary draft directory for a packet, which would not be deleted on completion of a run if a file from a dependency had been copied in. While `unlink` has a `force` option to change the permissions before deleting, I've decided to use `fs::dir_delete` instead, which always allows deleting read-only files, and which does not fail silently. For consistency I've replaced all the uses of unlink, even some that we don't expect to be removing files from the archive or file store.
plietar
force-pushed
the
read-only-archive
branch
from
ca25304 to
97c06e9
Compare
richfitz
approved these changes
May 31, 2024
plietar
added a commit
that referenced
this pull request
Jul 2, 2024
Since [orderly2#141], orderly marks all files in the archive as read-only. Similarly, files in the file store have always been read-only. This helps prevent accidental corruption by users. However this has had the undesirable side-effect of copying the files with the read-only bit set, even when exporting for interactive use by the user. One scenario where this has come up is a user running `orderly_dependency` interactively twice. They would expect the second call to succeed and overwrite the existing files. However this was not the case, and instead the second call fails as the read-only files cannot be overwritten. The user ends up having to manually delete these files (or mark them writable) everytime they want to update their copy. This change marks all files that are exported as writable. This does not apply to calls to `orderly_dependency` from a job context, in which case the files are still read-only. [orderly2#141]: #141
plietar
added a commit
that referenced
this pull request
Jul 8, 2024
Since [orderly2#141], orderly marks all files in the archive as read-only. Similarly, files in the file store have always been read-only. This helps prevent accidental corruption by users. However this has had the undesirable side-effect of copying the files with the read-only bit set, even when exporting for interactive use by the user. One scenario where this has come up is a user running `orderly_dependency` interactively twice. They would expect the second call to succeed and overwrite the existing files. However this was not the case, and instead the second call fails as the read-only files cannot be overwritten. The user ends up having to manually delete these files (or mark them writable) everytime they want to update their copy. This change marks all files that are exported as writable. This does not apply to calls to `orderly_dependency` from a job context, in which case the files are still read-only. [orderly2#141]: #141
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is easy for a user to inspect a packet's result by opening it up in their editor and accidentally edit and save the file. If they didn't have a file store and no other packet contains a copy of the file then the packet may be irremediably corrupted.
Marking the file as read-only is an easy way to prevent this. Of course a user can always force their way around it, but this should at least prevent most accidental occurrences.
A side-effect of this change is that file copied out of the archive will now be read-only as well, since copying the files preserves the access mode. This is desirable when copying files from a dependency, but may not be when copying outside of a packet context. If this causes problems we can introduce some option to
orderly_copy_filesto add the write bit back on files it copies.Files that were put into the file store were already being marked as read-only, so this change makes the two implementations more consistent. The executable bit was being cleared as well, which seems unnecessary to me and in some contexts even wrong (a user may well want to include a bash script in their report). I've changed the chmod operation from "a-wx" to "a-w". This doesn't matter on Windows, where executable bits don't exist.
A few places around the codebase were using
unlink(recursive=TRUE)to remove directories, which on Windows fails silently if the directory contains read-only files. One example of this was the temporary draft directory for a packet, which would not be deleted on completion of a run if a file from a dependency had been copied in. Whileunlinkhas aforceoption to change the permissions before deleting, I've decided to usefs::dir_deleteinstead, which always allows deleting read-only files, and which does not fail silently. For consistency I've replaced all the uses of unlink, even some that we don't expect to be removing files from the archive or file store.