Merge pull request #1944 from go-git/fix-perms

pjbgf · web-flow · commit 45ae193b3a60 · 2026-03-31T13:44:26.000+01:00
dotgit: skip writing pack files that already exist on disk
diff --git a/storage/filesystem/dotgit/writers.go b/storage/filesystem/dotgit/writers.go
@@ -132,30 +132,64 @@ func (w *PackWriter) clean() error {
 
 func (w *PackWriter) save() error {
 	base := w.fs.Join(objectsPath, packPath, fmt.Sprintf("pack-%s", w.checksum))
-	idx, err := w.fs.Create(fmt.Sprintf("%s.idx", base))
+
+	// Pack files are content addressable. Each file is checked
+	// individually — if it already exists on disk, skip creating it.
+	idxPath := fmt.Sprintf("%s.idx", base)
+	exists, err := fileExists(w.fs, idxPath)
 	if err != nil {
 		return err
 	}
+	if !exists {
+		idx, err := w.fs.Create(idxPath)
+		if err != nil {
+			return err
+		}
 
-	if err := w.encodeIdx(idx); err != nil {
-		_ = idx.Close()
-		return err
-	}
+		if err := w.encodeIdx(idx); err != nil {
+			_ = idx.Close()
+			return err
+		}
 
-	if err := idx.Close(); err != nil {
-		return err
+		if err := idx.Close(); err != nil {
+			return err
+		}
+		fixPermissions(w.fs, idxPath)
 	}
-	fixPermissions(w.fs, fmt.Sprintf("%s.idx", base))
 
 	packPath := fmt.Sprintf("%s.pack", base)
-	if err := w.fs.Rename(w.fw.Name(), packPath); err != nil {
+	exists, err = fileExists(w.fs, packPath)
+	if err != nil {
 		return err
 	}
-	fixPermissions(w.fs, packPath)
+	if !exists {
+		if err := w.fs.Rename(w.fw.Name(), packPath); err != nil {
+			return err
+		}
+		fixPermissions(w.fs, packPath)
+	} else {
+		// Pack already exists, clean up the temp file.
+		return w.clean()
+	}
 
 	return nil
 }
 
+// fileExists checks whether path already exists as a regular file.
+// It returns (true, nil) for an existing regular file, (false, nil) when the
+// path does not exist, and (false, err) if the path exists but is not a
+// regular file (e.g. a directory or symlink).
+func fileExists(fs billy.Filesystem, path string) (bool, error) {
+	fi, err := fs.Lstat(path)
+	if err != nil {
+		return false, nil
+	}
+	if !fi.Mode().IsRegular() {
+		return false, fmt.Errorf("unexpected file type for %q: %s", path, fi.Mode().Type())
+	}
+	return true, nil
+}
+
 func (w *PackWriter) encodeIdx(writer io.Writer) error {
 	idx, err := w.writer.Index()
 	if err != nil {
@@ -235,7 +269,6 @@ func (s *syncedReader) sleep() {
 		atomic.StoreUint32(&s.blocked, 1)
 		<-s.news
 	}
-
 }
 
 func (s *syncedReader) Seek(offset int64, whence int) (int64, error) {
@@ -293,7 +326,7 @@ func (w *ObjectWriter) save() error {
 	// Loose objects are content addressable, if they already exist
 	// we can safely delete the temporary file and short-circuit the
 	// operation.
-	if _, err := w.fs.Stat(file); err == nil || os.IsExist(err) {
+	if _, err := w.fs.Lstat(file); err == nil || os.IsExist(err) {
 		return w.fs.Remove(w.f.Name())
 	}
 
diff --git a/storage/filesystem/dotgit/writers_test.go b/storage/filesystem/dotgit/writers_test.go
@@ -181,6 +181,95 @@ func TestPackWriterPermissions(t *testing.T) {
 	}
 }
 
+func TestPackWriterExistingReadOnly(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name string
+		fs   billy.Filesystem
+	}{
+		{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
+		{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			f := fixtures.Basic().One()
+
+			dot := New(tc.fs)
+			require.NoError(t, dot.Initialize())
+
+			pfPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.pack", f.PackfileHash))
+			idxPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.idx", f.PackfileHash))
+
+			writePack := func() {
+				t.Helper()
+				w, err := dot.NewObjectPack()
+				require.NoError(t, err)
+
+				_, err = io.Copy(w, f.Packfile())
+				require.NoError(t, err)
+				require.NoError(t, w.Close())
+			}
+
+			writePack()
+
+			ro, err := isReadOnly(tc.fs, pfPath)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", pfPath)
+
+			ro, err = isReadOnly(tc.fs, idxPath)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", idxPath)
+
+			writePack()
+
+			// Remove .idx only, keep .pack — the next write must
+			// recreate .idx without touching the existing .pack.
+			require.NoError(t, tc.fs.Remove(idxPath))
+			writePack()
+
+			_, err = tc.fs.Lstat(idxPath)
+			require.NoError(t, err, ".idx should have been recreated")
+
+			ro, err = isReadOnly(tc.fs, idxPath)
+			require.NoError(t, err)
+			assert.True(t, ro, "recreated %q is not read-only", idxPath)
+		})
+	}
+}
+
+func TestPackWriterRejectsNonRegularFile(t *testing.T) {
+	t.Parallel()
+
+	for _, ext := range []string{".idx", ".pack"} {
+		t.Run(ext, func(t *testing.T) {
+			t.Parallel()
+
+			f := fixtures.Basic().One()
+			fs := osfs.New(t.TempDir(), osfs.WithBoundOS())
+
+			dot := New(fs)
+			require.NoError(t, dot.Initialize())
+
+			// Place a directory where the pack file should go.
+			path := filepath.Join("objects", "pack",
+				fmt.Sprintf("pack-%s%s", f.PackfileHash, ext))
+			require.NoError(t, fs.MkdirAll(path, 0o755))
+
+			w, err := dot.NewObjectPack()
+			require.NoError(t, err)
+
+			_, err = io.Copy(w, f.Packfile())
+			require.NoError(t, err)
+
+			err = w.Close()
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "unexpected file type")
+		})
+	}
+}
+
 func TestObjectWriterPermissions(t *testing.T) {
 	t.Parallel()
 
