Repository navigation
fix: preserve read-only hardlink attributes on Windows - #381
Open
karthiksenv wants to merge 3 commits into
Open
karthiksenv wants to merge 3 commits into
karthiksenv wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
==========================================
+ Coverage 69.53% 73.67% +4.14%
==========================================
Files 26 26
Lines 1940 2082 +142
Branches 319 196 -123
==========================================
+ Hits 1349 1534 +185
+ Misses 553 504 -49
- Partials 38 44 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
karthiksenv
marked this pull request as ready for review
November 6, 2026 04:03
This branch has not been deployed
This file contains hidden or 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
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.
Removing a read-only hardlink on Windows currently clears the read-only attribute before unlinking. That attribute is shared by the remaining links, so removing a workspace file can leave its cache object writable, as reported in treeverse/dvc#11119.
Use Windows extended file disposition for read-only regular-file unlink failures. This removes the requested name without clearing the shared attribute and also covers files removed through
shutil.rmtree. The helper closes its handle on success or failure and preserves the original deletion error if closing also fails. Add direct/recursive hardlink regressions, failure-path tests, and a README compatibility note.The handle-based approach follows the suggestion and independent reproduction in treeverse/dvc#11119 (comment).
Compatibility decision for review: if the OS or filesystem rejects this operation, removal raises an error rather than falling back to chmod. This applies to singly linked read-only regular files too. Such environments may previously have removed these files successfully; preserving shared attributes takes precedence in this change. Older Windows versions and network filesystems have not been exercised.
Validation on Windows, Python 3.12.15:
utils.removeimplementation is loaded from HEAD into the test process, and pass with the patch. This comparison does not replace the entire installed package with an unchanged baseline.python -m pytest tests --ignore=tests/test_reflink.py --ignore=tests/benchmarks -q -o asyncio_default_fixture_loop_scope=function --tb=shortwith a fresh local--basetemp.--platform win32and--platform linux.git diff --checkpasses.32edb41or this patch imported into the same environment. In fresh NTFS repositories, bothdvc checkout --relinkanddvc unprotectleft the shared cache object writable on the base, and a subsequent workspace append corrupted its contents. Both commands preserved cache protection with the patch, and the append was blocked. Theunprotectcase also verified that the selected file became a writable independent copy while the second workspace hardlink and cache stayed protected.Full Nox setup stopped at its native
reflinktest dependency, which requires Microsoft C++ build tools unavailable locally. Full Nox and full pre-commit remain deferred.Refresh in
43d80cb:d3ad931, including its pytest-asyncio minimum-version and fixture-loop-scope configuration fix. The contribution diff remains five files and 177 added lines.Test follow-up in
210ef44simulates Linux and macOS platform detection and checks that callers receiveENOTSUPand retain the original file contents. The 13 focused removal/API cases pass on Windows; the changed test file passes Ruff 0.17.3 and Mypy. The Tests workflow passed on this follow-up: https://github.com/treeverse/dvc-objects/actions/runs/43127057147 . The complete Tests workflow passed on the preceding merge43d80cb.AI assistance: OpenAI Codex investigated the issue, prepared the implementation and tests, and ran the checks above.