storage: filesystem, Avoid overwriting loose obj files#1867
storage: filesystem, Avoid overwriting loose obj files#1867pjbgf merged 2 commits intogo-git:mainfrom
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]>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #55 where attempting to stage files on Windows resulted in "Access is denied" errors when trying to rename loose object files. The root cause was that loose object files were being overwritten instead of being skipped when they already existed, and these files weren't being marked as read-only on Windows, leading to permission errors.
Changes:
- Added platform-specific implementations to set loose and packed object files as read-only on both Windows and Unix systems
- Modified loose object creation to skip overwriting existing files (content-addressable files are immutable)
- Updated tests to run on Windows and verify read-only permissions for both BoundOS and ChrootOS filesystems
- Updated go-billy dependency to version that supports required Chmod functionality
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/filesystem/dotgit/writers_windows.go | Windows-specific implementation for setting files read-only using Windows API and checking read-only status |
| storage/filesystem/dotgit/writers_unix.go | Unix-specific implementation for setting files to 0o444 permissions and checking read-only status |
| storage/filesystem/dotgit/writers.go | Added logic to skip overwriting existing loose objects and removed old fixPermissions implementation |
| storage/filesystem/dotgit/writers_test.go | Enhanced permission tests to run on both Windows and Unix with BoundOS and ChrootOS |
| storage/filesystem/dotgit/dotgit_test.go | Added TestIssue55 to verify loose objects can be safely recreated without errors |
| go.mod | Updated go-billy dependency to newer version |
| go.sum | Updated checksums for new go-billy version and cleaned up unused indirect dependencies |
💡 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]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func fixPermissions(fs billy.Filesystem, path string) { | ||
| fullpath := filepath.Join(fs.Root(), path) | ||
| p, err := windows.UTF16PtrFromString(fullpath) |
There was a problem hiding this comment.
On Windows, fixPermissions/isReadOnly build an OS path via filepath.Join(fs.Root(), path). This can be wrong for filesystem wrappers that route paths to different underlying roots (e.g., RepositoryFilesystem when commondir is enabled, where Root() is the dotgit root but objects may live under the common root). Prefer deriving the real OS path from the filesystem operation itself (e.g., Open/Stat the file via billy and use the returned file's Name()) before calling windows.Get/SetFileAttributes, so permissions are applied to the actual file.
| if err != nil { | ||
| trace.General.Printf("failed to chmod %s: %v", fullpath, err) | ||
| return | ||
| } | ||
|
|
||
| attrs, err := windows.GetFileAttributes(p) | ||
| if err != nil { | ||
| trace.General.Printf("failed to chmod %s: %v", fullpath, err) | ||
| return |
There was a problem hiding this comment.
The log/error messages in writers_windows.go say "chmod" but the implementation uses Windows file attributes (read-only flag), not chmod. Using a more accurate message (e.g., "failed to set read-only attribute" / "failed to set permissions") will make debugging clearer.
| pfPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.pack", f.PackfileHash)) | ||
| idxPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.idx", f.PackfileHash)) | ||
|
|
||
| stat, err = fs.Stat(idxPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm()) | ||
| ro, err := isReadOnly(tc.fs, pfPath) | ||
| require.NoError(t, err) | ||
| assert.True(t, ro, "file %q is not read-only", pfPath) | ||
|
|
||
| stat, err = fs.Stat(revPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm()) | ||
| ro, err = isReadOnly(tc.fs, idxPath) | ||
| require.NoError(t, err) | ||
| assert.True(t, ro, "file %q is not read-only", idxPath) |
There was a problem hiding this comment.
TestPackWriterPermissions no longer verifies that the generated .rev file is read-only, even though PackWriter.save still creates it and calls fixPermissions on it. Adding an assertion for the .rev file would keep permission coverage consistent with the previous test and catch regressions specific to rev files.
| writeObject := func(fs billy.Filesystem) { | ||
| t.Helper() | ||
|
|
||
| dir := New(fs) | ||
| err := dir.Initialize() | ||
| require.NoError(t, err) | ||
|
|
||
| w, err := dir.NewObject() | ||
| require.NoError(t, err) | ||
|
|
||
| err = w.WriteHeader(plumbing.BlobObject, 14) | ||
| require.NoError(t, err) | ||
| n, err := w.Write([]byte("this is a test")) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, 14, n) | ||
|
|
||
| assert.Equal(t, "a8a940627d132695a9769df883f85992f0ff4a43", w.Hash().String()) | ||
|
|
||
| err = w.Close() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
TestIssue55 defines writeObject as a closure over the parent test's *testing.T, but it is invoked from parallel subtests. Using the parent T inside parallel subtests can cause races and mis-attributed failures. Pass the subtest's *testing.T into writeObject (or define the helper inside the subtest) and use that T for Helper()/require/assert.
Forward-port of #1864.
Closes #55.