From 101c1ab3deb100eb9522c689bc8bdaa0728373fd Mon Sep 17 00:00:00 2001
From: Javier Alvarez Garcia <ja@daedalean.ai>
Date: Mon, 17 Jun 2024 19:10:33 +0200
Subject: storage: Fix reference updated concurrently error for the filesystem
 storer

If a reference exists in the packed refs file but not in the actual
refs folder, we need to fall back to reading the pack refs file when
storing the reference.

This happens because by the time we are storing the reference, we open
the file with write permissions and read from it. At that point, if the
file is empty, we still need to read the valid reference. Otherwise, it
will read 0, and assume that the references do not match (because the
old value was read from the packed file).
---
 storage/filesystem/dotgit/dotgit.go      | 18 ++++++++++
 storage/filesystem/dotgit/dotgit_test.go | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

(limited to 'storage/filesystem/dotgit')

diff --git a/storage/filesystem/dotgit/dotgit.go b/storage/filesystem/dotgit/dotgit.go
index ada51eb..72c9ccf 100644
--- a/storage/filesystem/dotgit/dotgit.go
+++ b/storage/filesystem/dotgit/dotgit.go
@@ -72,6 +72,9 @@ var (
 	// ErrIsDir is returned when a reference file is attempting to be read,
 	// but the path specified is a directory.
 	ErrIsDir = errors.New("reference path is a directory")
+	// ErrEmptyRefFile is returned when a reference file is attempted to be read,
+	// but the file is empty
+	ErrEmptyRefFile = errors.New("ref file is empty")
 )
 
 // Options holds configuration for the storage.
@@ -661,18 +664,33 @@ func (d *DotGit) readReferenceFrom(rd io.Reader, name string) (ref *plumbing.Ref
 		return nil, err
 	}
 
+	if len(b) == 0 {
+		return nil, ErrEmptyRefFile
+	}
+
 	line := strings.TrimSpace(string(b))
 	return plumbing.NewReferenceFromStrings(name, line), nil
 }
 
+// checkReferenceAndTruncate reads the reference from the given file, or the `pack-refs` file if
+// the file was empty. Then it checks that the old reference matches the stored reference and
+// truncates the file.
 func (d *DotGit) checkReferenceAndTruncate(f billy.File, old *plumbing.Reference) error {
 	if old == nil {
 		return nil
 	}
+
 	ref, err := d.readReferenceFrom(f, old.Name().String())
+	if errors.Is(err, ErrEmptyRefFile) {
+		// This may happen if the reference is being read from a newly created file.
+		// In that case, try getting the reference from the packed refs file.
+		ref, err = d.packedRef(old.Name())
+	}
+
 	if err != nil {
 		return err
 	}
+
 	if ref.Hash() != old.Hash() {
 		return storage.ErrReferenceHasChanged
 	}
diff --git a/storage/filesystem/dotgit/dotgit_test.go b/storage/filesystem/dotgit/dotgit_test.go
index c224397..be66fee 100644
--- a/storage/filesystem/dotgit/dotgit_test.go
+++ b/storage/filesystem/dotgit/dotgit_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/go-git/go-billy/v5/util"
 	fixtures "github.com/go-git/go-git-fixtures/v4"
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/storage"
 	"github.com/stretchr/testify/assert"
 	. "gopkg.in/check.v1"
 )
@@ -1046,3 +1047,63 @@ func (s *SuiteDotGit) TestDeletedRefs(c *C) {
 	c.Assert(refs, HasLen, 1)
 	c.Assert(refs[0].Name(), Equals, plumbing.ReferenceName("refs/heads/foo"))
 }
+
+// Checks that seting a reference that has been packed and checking its old value is successful
+func (s *SuiteDotGit) TestSetPackedRef(c *C) {
+	fs, clean := s.TemporalFilesystem()
+	defer clean()
+
+	dir := New(fs)
+
+	err := dir.SetRef(plumbing.NewReferenceFromStrings(
+		"refs/heads/foo",
+		"e8d3ffab552895c19b9fcf7aa264d277cde33881",
+	), nil)
+	c.Assert(err, IsNil)
+
+	refs, err := dir.Refs()
+	c.Assert(err, IsNil)
+	c.Assert(refs, HasLen, 1)
+	looseCount, err := dir.CountLooseRefs()
+	c.Assert(err, IsNil)
+	c.Assert(looseCount, Equals, 1)
+
+	err = dir.PackRefs()
+	c.Assert(err, IsNil)
+
+	// Make sure the refs are still there, but no longer loose.
+	refs, err = dir.Refs()
+	c.Assert(err, IsNil)
+	c.Assert(refs, HasLen, 1)
+	looseCount, err = dir.CountLooseRefs()
+	c.Assert(err, IsNil)
+	c.Assert(looseCount, Equals, 0)
+
+	ref, err := dir.Ref("refs/heads/foo")
+	c.Assert(err, IsNil)
+	c.Assert(ref, NotNil)
+	c.Assert(ref.Hash().String(), Equals, "e8d3ffab552895c19b9fcf7aa264d277cde33881")
+
+	// Attempt to update the reference using an invalid old reference value
+	err = dir.SetRef(plumbing.NewReferenceFromStrings(
+		"refs/heads/foo",
+		"b8d3ffab552895c19b9fcf7aa264d277cde33881",
+	), plumbing.NewReferenceFromStrings(
+		"refs/heads/foo",
+		"e8d3ffab552895c19b9fcf7aa264d277cde33882",
+	))
+	c.Assert(err, Equals, storage.ErrReferenceHasChanged)
+
+	// Now update the reference and it should pass
+	err = dir.SetRef(plumbing.NewReferenceFromStrings(
+		"refs/heads/foo",
+		"b8d3ffab552895c19b9fcf7aa264d277cde33881",
+	), plumbing.NewReferenceFromStrings(
+		"refs/heads/foo",
+		"e8d3ffab552895c19b9fcf7aa264d277cde33881",
+	))
+	c.Assert(err, IsNil)
+	looseCount, err = dir.CountLooseRefs()
+	c.Assert(err, IsNil)
+	c.Assert(looseCount, Equals, 1)
+}
-- 
cgit