storage: filesystem, Avoid overwriting loose obj files#1864
Merged
pjbgf merged 2 commits intogo-git:releases/v5.xfrom Feb 25, 2026
Merged
storage: filesystem, Avoid overwriting loose obj files#1864pjbgf merged 2 commits intogo-git:releases/v5.xfrom
pjbgf merged 2 commits intogo-git:releases/v5.xfrom
Conversation
Align behaviour with upstream and v6, whereby all loose and packed objects are saved on disk as read-only as they are not meant to be modified due to their nature, as they are content addressable files. Signed-off-by: Paulo Gomes <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the storage/filesystem/dotgit writers to better respect the immutability of content-addressed Git object storage and to avoid Windows “Access Denied” failures when attempting to overwrite existing loose objects.
Changes:
- Add OS-specific helpers to mark written pack/idx and loose object files as read-only (Windows file attribute vs Unix perms).
- Update pack/object writer save paths to apply read-only permissions and avoid overwriting existing loose objects.
- Expand/adjust tests to validate permissions and reproduce Issue #55; bump
go-billydependency to include required filesystem behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/filesystem/dotgit/writers_windows.go | Adds Windows-specific permission handling via FILE_ATTRIBUTE_READONLY. |
| storage/filesystem/dotgit/writers_unix.go | Adds Unix permission handling via Chmod(0444) where supported. |
| storage/filesystem/dotgit/writers.go | Applies permission fixes after writing pack/idx and loose objects; skips overwriting loose objects. |
| storage/filesystem/dotgit/writers_test.go | Adds new permission-focused tests for pack and loose object writers across OSFS variants. |
| storage/filesystem/dotgit/dotgit_test.go | Adds regression test intended to reproduce Issue #55 behavior. |
| go.mod | Bumps github.com/go-git/go-billy/v5 to v5.8.0. |
| go.sum | Updates sums for the go-billy version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Loose object files are content-addressable and imutable. They should be created on demand and deleted on repacking. However, they should not be overwritten - assuming the initial file isn't corrupted. The previous lack of validation meant those files were being overwritten when in fact they could just be ignored. In Linux, this was a non-issue, however, in Windows this operation led to Access Denied errors. Some additional moving parts of this fix: - [go-billy](go-git/go-billy#187): Align behaviour supporting dir.NewObject(): - Add support for Chmod in polyfill so that ChrootOS is able to chmod files. - Ensure temporary directories are created for BoundOS to avoid errors when trying to create the temporary file used for loose files. - This PR: - Ensure that in Windows, packed and loose object files are created as read-only, which in this case means setting the flag windows.FILE_ATTRIBUTE_READONLY via x/sys/windows. - Skip renaming the temporary file into the existing loose object, instead simply delete the temporary file. Relates to: - Southclaws/sampctl#422 - git-bug/git-bug#1142 - entireio/cli#455 Signed-off-by: Paulo Gomes <[email protected]>
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.
Loose object files are content-addressable and imutable. They should be created on demand and deleted on repacking. However, they should not be overwritten - assuming the initial file isn't corrupted.
The previous lack of validation meant those files were being overwritten when in fact they could just be ignored. In Linux, this was a non-issue, however, in Windows this operation led to
Access Deniederrors.Some additional moving parts of this fix:
dir.NewObject():Chmodin polyfill so that ChrootOS is able tochmodfiles.windows.FILE_ATTRIBUTE_READONLYviax/sys/windows.Relates to:
Fixes #55.