storage/filesystem: a rejected `CheckAndSetReference` leaves an empty loose ref that breaks reference iteration

Bug Description

On the filesystem storage, CheckAndSetReference(new, old) can have side effects when it fails.

When the reference has no loose file (it is packed, or doesn't exist) and old doesn't match, the call correctly returns an error, but it leaves an empty loose ref file behind. From then on:

• Repository.References() / IterReferences() fails for the whole repository with ref file is empty,
• git show-ref fails with fatal: bad ref, and git for-each-ref warns and skips the ref.

A second, related problem: on filesystems without billy.ReadAndWriteCapability, the old check is skipped entirely when the ref has no loose file, so a stale update is silently applied.

Expected behaviour (reference git)

git update-ref <ref> <new> <old> stores <new> "after verifying that the current value of the <ref> matches <old-oid>". When the check fails, nothing is written:

$ git update-ref refs/bugs/missing $B $A
fatal: update_ref failed for ref 'refs/bugs/missing': cannot lock ref 'refs/bugs/missing': unable to resolve reference 'refs/bugs/missing'

$ git update-ref refs/bugs/packed $A $A     # packed ref, currently at $B
fatal: update_ref failed for ref 'refs/bugs/packed': cannot lock ref 'refs/bugs/packed': is at 5a327ab... but expected 2b3edd5...

In both cases no loose file is created, and git for-each-ref still lists the packed ref.

Tracing git (2.47.3) shows why: it takes refs/heads/topic.lock (O_CREAT|O_EXCL), checks the current value, and on mismatch unlinks the lock. On success it writes the lock file and renames it over the ref. The ref file itself is never opened for writing.

Root cause

References below are at e0993107 (current main); releases/v5.x has the same code.

1. setRefRwfs creates the loose file before checking old

dotgit_setref.go#L21-L53 opens the loose ref file with O_RDWR|O_CREATE, locks it, then calls checkReferenceAndTruncate. When the file was just created, that function reads it as empty, falls back to packed-refs, and returns ErrReferenceNotFound or ErrReferenceHasChanged. The file it created is never removed.

The empty file then makes walkReferencesTree return ErrEmptyRefFile, which aborts the whole iteration. (DotGit.Ref on that single name still falls back to packed-refs.)

2. setRefNorwfs skips the check when there is no loose file

dotgit_setref.go#L63-L92 only compares old when Stat(fileName) succeeds. For a packed or missing ref it goes straight to Create and writes, so a stale update to a packed ref overwrites it, and a stale update to a missing ref creates it. On a loose-ref mismatch it also returns a plain fmt.Errorf rather than storage.ErrReferenceHasChanged.

Impact

A mismatch on a packed ref is the normal way to reject a stale update, so this isn't limited to exotic races:

• Server side (receive-pack): for an update command, updateReferences passes the client's old value straight to CheckAndSetReference. Calling updateReferences directly with a packed refs/heads/main and a stale old value returns reference has changed concurrently (correct), leaves an empty refs/heads/main, and IterReferences() then fails. With two concurrent pushes to the same branch, the rejected push would leave the served repository unable to list its references. I have not reproduced this through a full network push.
• Fetch: Remote.updateLocalReferenceStorage reads the local ref right before calling CheckAndSetReference, so it is only affected if another writer changes a packed ref in between.
• Library users relying on CheckAndSetReference for optimistic concurrency, who hit the failure path by design.

Regression test

I have a test for storage/filesystem/dotgit that fails on main: for both write paths (rw/norwfs) × (missing ref / packed ref), a SetRef with a stale old must return the error, leave no loose file, keep Refs() working, and leave Ref() unchanged. Happy to open a PR once the direction is agreed.

Possible fixes

1. Check before creating. When old != nil, open the loose file without O_CREATE. If it doesn't exist, compare old against packed-refs first and return without touching the disk; only on a match create, lock, and run the existing check. Apply the same packed-refs check in setRefNorwfs. Small and local. A concurrent RemoveRef between the first check and the lock can still leave an empty file; removing it afterwards isn't safe, as a writer already waiting on the same file lock would then write into an unlinked file.
2. Use git's lock-file protocol for ref updates: create <ref>.lock exclusively, check the current value (loose or packed), write the lock file, rename it over the ref, and unlink it on failure. This matches git, never leaves a partial ref, never exposes a truncated ref to concurrent readers, and coordinates with git itself. It changes every ref write, though, including how a stale .lock is handled.

AI disclosure: this issue was investigated and drafted with AI assistance (Claude Code, Claude Opus 5). The reproductions, traces and outputs above were run locally against the versions listed.

go-git Version

main at e0993107 (v6.0.0-alpha.5-149)

Steps to Reproduce

git init -q /tmp/repro && cd /tmp/repro
git commit -q --allow-empty -m first
git commit -q --allow-empty -m second
git commit -q --allow-empty -m third
git update-ref refs/heads/topic HEAD~1
git pack-refs --all
go run ./repro /tmp/repro $(git rev-parse HEAD~2) $(git rev-parse HEAD)

package main

import (
	"fmt"
	"os"

	"github.com/go-git/go-git/v6"
	"github.com/go-git/go-git/v6/plumbing"
)

func main() {
	r, err := git.PlainOpen(os.Args[1])
	if err != nil {
		panic(err)
	}
	stale, newHash := plumbing.NewHash(os.Args[2]), plumbing.NewHash(os.Args[3])

	for _, name := range []plumbing.ReferenceName{"refs/heads/topic", "refs/heads/missing"} {
		err := r.Storer.CheckAndSetReference(
			plumbing.NewHashReference(name, newHash),
			plumbing.NewHashReference(name, stale),
		)
		fmt.Printf("CheckAndSetReference(%s): %v\n", name, err)
	}

	_, err = r.References()
	fmt.Printf("Repository.References(): %v\n", err)
}

Output:

CheckAndSetReference(refs/heads/topic): reference has changed concurrently
CheckAndSetReference(refs/heads/missing): reference not found
Repository.References(): ref file is empty

$ ls -l .git/refs/heads/
-rw-rw-r-- 1 user user 0 Sep 15 23:25 missing
-rw-rw-r-- 1 user user 0 Sep 15 23:25 topic

$ git for-each-ref
warning: ignoring broken ref refs/heads/missing
warning: ignoring broken ref refs/heads/topic
2d3f10fc5de33faee73daa860e4ec66d7802830c commit	refs/heads/master

$ git show-ref
fatal: git show-ref: bad ref refs/heads/missing (0000000000000000000000000000000000000000)

Both errors returned by CheckAndSetReference are correct; the problem is what's left on disk.

Additional Information

Regression test, to put in storage/filesystem/dotgit/dotgit_test.go

func (s *SuiteDotGit) TestSetRefWithStaleOldLeavesNoLooseRef() {
	packedHash := plumbing.NewHash("1111111111111111111111111111111111111111")
	stale := plumbing.NewReferenceFromStrings("refs/heads/main", "2222222222222222222222222222222222222222")
	updated := plumbing.NewReferenceFromStrings("refs/heads/main", "3333333333333333333333333333333333333333")

	for _, rw := range []bool{true, false} {
		for _, packed := range []bool{false, true} {
			s.Run(fmt.Sprintf("rw=%t/packed=%t", rw, packed), func() {
				fs := s.EmptyFS()
				dirFS := fs
				if !rw {
					dirFS = &norwfs{fs}
				}
				wantErr := plumbing.ErrReferenceNotFound
				if packed {
					s.Require().NoError(util.WriteFile(fs, "packed-refs", []byte(packedHash.String()+" refs/heads/main\n"), 0o644))
					wantErr = storage.ErrReferenceHasChanged
				}
				dir := New(dirFS)

				s.ErrorIs(dir.SetRef(updated, stale), wantErr)

				_, err := fs.Stat("refs/heads/main")
				s.ErrorIs(err, os.ErrNotExist)
				_, err = dir.Refs()
				s.NoError(err)
				ref, err := dir.Ref("refs/heads/main")
				if packed {
					s.Require().NoError(err)
					s.Equal(packedHash, ref.Hash())
				} else {
					s.ErrorIs(err, plumbing.ErrReferenceNotFound)
				}
			})
		}
	}
}