dvc: use protected mode by default #3472
Conversation
efiop
force-pushed
the
3261
branch
3 times, most recently
from
c5aa073 to
b1e523c
Compare
| @@ -82,19 +82,24 @@ class RemoteBASE(object): | |||
| DEFAULT_NO_TRAVERSE = True | |||
| DEFAULT_VERIFY = False | |||
|
|
|||
| CACHE_MODE = None | |||
There was a problem hiding this comment.
Generalising it because SSH could use the same optimization a bit later.
| shared = config.get("shared") | ||
| self._file_mode, self._dir_mode = self.SHARED_MODE_MAP[shared] | ||
|
|
||
| if self.protected: |
There was a problem hiding this comment.
Previously we kinda mixed up cache and link protection, the new patch puts the emphasis on cache protection, which is what we really want and should be much less confusing.
| System.reflink(from_info, to_info) | ||
| def reflink(self, from_info, to_info): | ||
| tmp_info = to_info.parent / tmp_fname(to_info.name) | ||
| System.reflink(from_info, tmp_info) |
There was a problem hiding this comment.
Yes, you can change mode on reflink without affecting the source. This is because reflinks have their own inodes, so from fs perspective they are separate files that have their own perms.
There was a problem hiding this comment.
@efiop, Can you mention this on a code itself as a comment?
There was a problem hiding this comment.
Also, can this not be done without link to temp?
There was a problem hiding this comment.
@efiop, Can you mention this on a code itself as a comment?
Sure! Done.
Also, can this not be done without link to temp?
Temp file is needed to make it atomic, so the target file appears already with correct permissions, so there is no chance of something trying to use it while it is in read-only mode.
| @@ -97,7 +97,7 @@ class RelPath(str): | |||
| } | |||
| LOCAL_COMMON = { | |||
| "type": supported_cache_type, | |||
| Optional("protected", default=False): Bool, | |||
| Optional("protected", default=False): Bool, # obsoleted | |||
There was a problem hiding this comment.
So this one is effectively defunct from now on. Just keeping it in the SCHEMA to not break on older configs.
There was a problem hiding this comment.
This does not respect explicit protected = False, does it?
There was a problem hiding this comment.
@skshetry It doesn't, as there is no clear use case for it.
|
Windows is failing, will check it out tomorrow. EDIT: fixed. |
efiop
force-pushed
the
3261
branch
12 times, most recently
from
00ade4a to
d40744e
Compare
efiop
changed the title
efiop
changed the title
efiop
changed the title
|
Please, update docs if needed after this change. |
efiop
force-pushed
the
3261
branch
2 times, most recently
from
d287df6 to
a1a497d
Compare
efiop
force-pushed
the
3261
branch
2 times, most recently
from
517a690 to
fd9b9a0
Compare
Fixes #3261
Fixes #2041
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
iterative/dvc.org#1058
Thank you for the contribution - we'll try to review it as soon as possible. 🙏