fix(refs): parse packed-refs once per listing and apply git's racy rule (GO-001, GO-002, GO-004, GO-012, GO-014)

collectRefs walks the loose files first, then loads packed-refs once and
resolves every name against it, instead of calling lookupRef (and so
statting and possibly re-parsing the file) per name. loadPacked rechecks
after taking pmu so concurrent callers share one parse, and a racy file
whose content did not change keeps its parse.

fileSig now records when it was read and is trusted only when that was more
than racyWindow after the mtime (git's racy-git rule), not measured from
now. Refs over 5000 packed refs right after a rewrite: 677 ms -> 1.5 ms.

A corrupt pseudo-ref at the top of the git dir (not HEAD) is not found.
Head, Refs, ResolveRef, LookupRef and Resolve return ErrClosed after Close.
Resolve documents git's order, tags before heads, and the 40-hex case.

commit: df84e16c30768f472ba52f7a84ace472f681a945
author: brice · 11d ago · 2026-10-10 22:15 UTC
parent: 219ceec
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

4 files · +515 -65

M refs.go +160 -55


@@ -12,6 +12,7 @@ import (
12	12		"slices"
13	13		"strings"
14	14		"syscall"
	15	+	"time"
15	16		)
16	17
17	18		const (

@@ -51,6 +52,7 @@ type packedRef struct {
51	52		// packedRefs is an immutable parsed packed-refs file, sorted by name.
52	53		type packedRefs struct {
53	54		sig fileSig
	55	+	raw string // the file content: the names in refs point into it
54	56		refs []packedRef
55	57		}
56	58

@@ -65,7 +67,8 @@ func (p *packedRefs) find(name string) (packedRef, bool) {
65	67		}
66	68
67	69		// loadPacked returns the current content of packed-refs, reparsing the file
68		-	// when its stat data changed (or always with force).
	70	+	// when its stat data changed (or always with force). Concurrent callers share
	71	+	// one parse: the check is repeated once pmu is held.
69	72		func (r Repo) loadPacked(force bool) (packedRefs, error) {
70	73		path := r.dir + "/packed-refs"
71	74		sig, err := statSig(path)

@@ -77,9 +80,17 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
77	80		}
78	81		r.pmu.Lock()
79	82		defer r.pmu.Unlock()
	83	+	cur := r.packed.Load()
	84	+	if !force && cur != nil && (cur.sig.read > sig.read \|\| cur.sig.same(sig)) {
	85	+	// Another caller parsed the file while we waited for the lock. It read
	86	+	// the file after we looked at it, so its result is at least as fresh
	87	+	// as ours would be.
	88	+	return cur, nil
	89	+	}
	90	+	read := time.Now().UnixNano()
80	91		f, err := os.Open(path)
81	92		if errors.Is(err, fs.ErrNotExist) {
82		-	p := &packedRefs{}
	93	+	p := &packedRefs{sig: fileSig{read: read}}
83	94		r.packed.Store(p)
84	95		return p, nil
85	96		}

@@ -98,6 +109,13 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
98	109		if _, err := io.ReadFull(f, data); err != nil {
99	110		return nil, err
100	111		}
	112	+	if !force && cur != nil && cur.raw == string(data) {
	113	+	// Racy stat data, same content (Git's racy rule): keep the parse
	114	+	// and record that the file was read now.
	115	+	p := &packedRefs{sig: sigOf(st, read), raw: cur.raw, refs: cur.refs}
	116	+	r.packed.Store(p)
	117	+	return p, nil
	118	+	}
101	119		p, err := parsePackedRefs(string(data))
102	120		if err != nil {
103	121		return nil, err

@@ -107,7 +125,7 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
107	125		}); i < len(p.refs) && strings.HasPrefix(p.refs[i].name, "refs/replace/") {
108	126		return nil, unsupported("replace refs (refs/replace in packed-refs)")
109	127		}
110		-	p.sig = sigOf(st)
	128	+	p.sig = sigOf(st, read)
111	129		r.packed.Store(p)
112	130		return p, nil
113	131		}

@@ -115,7 +133,7 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
115	133		// parsePackedRefs parses a packed-refs file. The ref names are substrings of
116	134		// data.
117	135		func parsePackedRefs(data string) (*packedRefs, error) {
118		-	p := &packedRefs{}
	136	+	p := &packedRefs{raw: data}
119	137		fullyPeeled, peeled := false, false
120	138		first := true
121	139		for len(data) > 0 {

@@ -276,52 +294,100 @@ func validRootRef(name string) bool {
276	294		// validRef accepts the names lookupRef may read.
277	295		func validRef(name string) bool { return validRefName(name) \|\| validRootRef(name) }
278	296
279		-	// lookupRef resolves name through symbolic refs. loose refs win over packed
280		-	// ones. The loose file is read before packed-refs, so a concurrent pack-refs
281		-	// (which writes packed-refs first and deletes the loose file after) is never
282		-	// missed.
283		-	func (r *Repo) lookupRef(name string) (Ref, bool, error) {
	297	+	// refWalk is a ref lookup that went through the loose files and may still
	298	+	// need packed-refs.
	299	+	type refWalk struct {
	300	+	ref Ref
	301	+	cur string // the name to look up in packed-refs when !done
	302	+	done bool // the lookup is complete: ok tells whether it found a ref
	303	+	ok bool
	304	+	}
	305	+
	306	+	// isRootRef reports whether name is a pseudo-ref of the Git directory
	307	+	// (HEAD, ORIG_HEAD...) rather than a name under refs/.
	308	+	func isRootRef(name string) bool { return !strings.HasPrefix(name, "refs/") }
	309	+
	310	+	// lookupLoose follows name through the loose files and symbolic refs. It
	311	+	// stops at a loose ref that holds an id, or at the first name that has no
	312	+	// loose file: packed-refs decides about that one (see finish).
	313	+	func (r *Repo) lookupLoose(name string) (refWalk, error) {
284	314		if !validRef(name) {
285		-	return Ref{}, false, nil
	315	+	return refWalk{done: true}, nil
286	316		}
287		-	ref := Ref{Name: name}
288		-	cur := name
	317	+	w := refWalk{ref: Ref{Name: name}, cur: name}
289	318		for depth := 0; ; depth++ {
290	319		if depth > maxSymrefDepth {
291		-	return Ref{}, false, corrupt("ref %s: symbolic ref loop", name)
	320	+	return refWalk{}, corrupt("ref %s: symbolic ref loop", name)
292	321		}
293		-	lr, found, err := r.readLooseRef(cur)
	322	+	lr, found, err := r.readLooseRef(w.cur)
294	323		if err != nil {
295		-	return Ref{}, false, err
	324	+	if errors.Is(err, ErrCorrupt) && isRootRef(w.cur) && w.cur != "HEAD" {
	325	+	// A pseudo-ref such as FETCH_HEAD or MERGE_HEAD can hold
	326	+	// anything (FETCH_HEAD has one line per fetched ref), and
	327	+	// a revision like "ABCDEF" may name such a file by
	328	+	// accident: it is not a ref, so not found. A broken HEAD
	329	+	// stays an error.
	330	+	return refWalk{done: true}, nil
	331	+	}
	332	+	return refWalk{}, err
296	333		}
297	334		if found && lr.symref != "" {
298	335		if depth == 0 {
299		-	ref.Symref = lr.symref
	336	+	w.ref.Symref = lr.symref
300	337		}
301	338		if !validRef(lr.symref) {
302		-	return Ref{}, false, nil
	339	+	return refWalk{done: true}, nil
303	340		}
304		-	cur = lr.symref
	341	+	w.cur = lr.symref
305	342		continue
306	343		}
307	344		if found {
308		-	ref.ID = lr.id
309		-	return ref, true, nil
	345	+	w.ref.ID = lr.id
	346	+	w.done, w.ok = true, true
	347	+	return w, nil
310	348		}
311		-	p, err := r.loadPacked(false)
312		-	if err != nil {
313		-	return Ref{}, false, err
314		-	}
315		-	e, ok := p.find(cur)
316		-	if !ok {
317		-	return Ref{}, false, nil
318		-	}
319		-	ref.ID = e.id
320		-	if depth == 0 {
321		-	ref.Peeled, ref.PeeledKnown = e.peeled, e.peeledKnown
322		-	}
323		-	return ref, true, nil
	349	+	return w, nil
	350	+	}
	351	+	}
	352	+
	353	+	// finish completes a lookup with the content of packed-refs. The loose files
	354	+	// have to be read before packed-refs is loaded: a concurrent pack-refs writes
	355	+	// packed-refs first and deletes the loose file after, so it is never missed.
	356	+	func (w refWalk) finish(p packedRefs) (Ref, bool) {
	357	+	if w.done {
	358	+	return w.ref, w.ok
	359	+	}
	360	+	e, ok := p.find(w.cur)
	361	+	if !ok {
	362	+	return Ref{}, false
	363	+	}
	364	+	w.ref.ID = e.id
	365	+	if w.cur == w.ref.Name {
	366	+	w.ref.Peeled, w.ref.PeeledKnown = e.peeled, e.peeledKnown
	367	+	}
	368	+	return w.ref, true
	369	+	}
	370	+
	371	+	// lookupRef resolves name through symbolic refs. loose refs win over packed
	372	+	// ones.
	373	+	func (r *Repo) lookupRef(name string) (Ref, bool, error) {
	374	+	if r.snap.Load() == nil {
	375	+	return Ref{}, false, ErrClosed
	376	+	}
	377	+	w, err := r.lookupLoose(name)
	378	+	if err != nil {
	379	+	return Ref{}, false, err
	380	+	}
	381	+	if w.done {
	382	+	ref, ok := w.finish(nil)
	383	+	return ref, ok, nil
324	384		}
	385	+	p, err := r.loadPacked(false)
	386	+	if err != nil {
	387	+	return Ref{}, false, err
	388	+	}
	389	+	ref, ok := w.finish(p)
	390	+	return ref, ok, nil
325	391		}
326	392
327	393		// ResolveRef returns the object the ref with this full name (such as

@@ -356,6 +422,9 @@ func (r *Repo) LookupRef(name string) (Ref, error) {
356	422		// yet) is not an error: id is then the zero ObjectID. For a detached HEAD
357	423		// target is empty and id is the commit.
358	424		func (r *Repo) Head() (target string, id ObjectID, err error) {
	425	+	if r.snap.Load() == nil {
	426	+	return "", ObjectID{}, ErrClosed
	427	+	}
359	428		lr, found, err := r.readLooseRef("HEAD")
360	429		if err != nil {
361	430		return "", ObjectID{}, err

@@ -397,7 +466,9 @@ func (r *Repo) Refs(prefix string) iter.Seq2[Ref, error] {
397	466		}
398	467
399	468		func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
400		-	// Loose refs first, packed-refs after: see lookupRef.
	469	+	if r.snap.Load() == nil {
	470	+	return nil, ErrClosed
	471	+	}
401	472		var dir string
402	473		switch {
403	474		case strings.HasPrefix(prefix, "refs/"):

@@ -407,15 +478,33 @@ func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
407	478		default:
408	479		return nil, nil
409	480		}
	481	+	// Loose refs first, packed-refs after: see refWalk.finish.
410	482		var loose []string
411	483		if err := r.walkLoose(dir, prefix, &loose, 0); err != nil {
412	484		return nil, err
413	485		}
	486	+	slices.Sort(loose)
	487	+	loose = slices.Compact(loose)
	488	+	type item struct {
	489	+	name string
	490	+	w refWalk
	491	+	}
	492	+	items := make([]item, 0, len(loose))
	493	+	for _, n := range loose {
	494	+	w, err := r.lookupLoose(n)
	495	+	if err != nil {
	496	+	if errors.Is(err, ErrCorrupt) {
	497	+	continue // Git ignores broken refs when listing
	498	+	}
	499	+	return nil, err
	500	+	}
	501	+	items = append(items, item{name: n, w: w})
	502	+	}
	503	+	// One load of packed-refs serves every name.
414	504		p, err := r.loadPacked(false)
415	505		if err != nil {
416	506		return nil, err
417	507		}
418		-	names := loose
419	508		start, _ := slices.BinarySearchFunc(p.refs, prefix, func(e packedRef, pre string) int {
420	509		if strings.HasPrefix(e.name, pre) {
421	510		return 0

@@ -427,23 +516,25 @@ func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
427	516		for start > 0 && strings.HasPrefix(p.refs[start-1].name, prefix) {
428	517		start--
429	518		}
	519	+	nLoose := len(items)
430	520		for i := start; i < len(p.refs) && strings.HasPrefix(p.refs[i].name, prefix); i++ {
431		-	names = append(names, p.refs[i].name)
432		-	}
433		-	slices.Sort(names)
434		-	names = slices.Compact(names)
435		-
436		-	out := make([]Ref, 0, len(names))
437		-	for _, n := range names {
438		-	ref, ok, err := r.lookupRef(n)
439		-	if err != nil {
440		-	if errors.Is(err, ErrCorrupt) {
441		-	continue // Git ignores broken refs when listing
442		-	}
443		-	return nil, err
	521	+	n := p.refs[i].name
	522	+	if _, ok := slices.BinarySearch(loose, n); ok {
	523	+	continue // the loose file wins; its walk is already in items
444	524		}
445		-	if ok {
446		-	ref.Name = n
	525	+	if !validRef(n) {
	526	+	continue
	527	+	}
	528	+	items = append(items, item{name: n, w: refWalk{ref: Ref{Name: n}, cur: n}})
	529	+	}
	530	+	if len(items) > nLoose {
	531	+	slices.SortFunc(items, func(a, b item) int { return strings.Compare(a.name, b.name) })
	532	+	}
	533	+	out := make([]Ref, 0, len(items))
	534	+	for i := range items {
	535	+	it := &items[i]
	536	+	if ref, ok := it.w.finish(p); ok {
	537	+	ref.Name = it.name
447	538		out = append(out, ref)
448	539		}
449	540		}

@@ -500,11 +591,25 @@ func (r *Repo) PeelRef(ref Ref) (ObjectID, error) {
500	591		}
501	592
502	593		// Resolve turns a revision name into an object id the way "git rev-parse"
503		-	// does for plain names: a full 40 digit id, "HEAD", a full ref name, a short
504		-	// ref name tried as "<n>", "refs/<n>", "refs/tags/<n>", "refs/heads/<n>",
505		-	// "refs/remotes/<n>" and "refs/remotes/<n>/HEAD" in that order, and finally
506		-	// an abbreviated id of 4 to 39 digits. Revision operators (~, ^, @{}) are not
507		-	// supported. The result is not peeled; use PeelToCommit.
	594	+	// does for plain names. The result is not peeled; use PeelToCommit. Revision
	595	+	// operators (~, ^, @{}, :path) are not supported.
	596	+	//
	597	+	// The name is tried in git's order, and the first match wins:
	598	+	//
	599	+	// 1. a full 40 digit id. It is taken as an id even when a ref has that name
	600	+	// (git warns that the ref is ambiguous and ignores it), and it must exist
	601	+	// in the repository, which "git rev-parse" does not check;
	602	+	// 2. a ref, as "<n>", "refs/<n>", "refs/tags/<n>", "refs/heads/<n>",
	603	+	// "refs/remotes/<n>" and "refs/remotes/<n>/HEAD", in that order. A tag
	604	+	// therefore wins over a branch of the same name, whereas "git checkout"
	605	+	// prefers the branch;
	606	+	// 3. an abbreviated id of 4 to 39 digits, which loses against any ref of
	607	+	// that name. It matches any object, whatever its type, and fails with
	608	+	// ErrAmbiguous if several do; see ResolveCommitPrefix.
	609	+	//
	610	+	// A caller that wants a branch or a tag, or a commit, should not use Resolve:
	611	+	// ResolveRef takes the full ref name and ResolveCommitPrefix only considers
	612	+	// commits. Resolve returns an error wrapping ErrNotFound if nothing matches.
508	613		func (r *Repo) Resolve(rev string) (ObjectID, error) {
509	614		if rev == "" {
510	615		return ObjectID{}, notFound("empty revision")


@@ -12,6 +12,7 @@ import (
12		"slices"	12		"slices"
13		"strings"	13		"strings"
14		"syscall"	14		"syscall"
			15	+	"time"
15		)	16		)
16			17
17		const (	18		const (

@@ -51,6 +52,7 @@ type packedRef struct {
51		// packedRefs is an immutable parsed packed-refs file, sorted by name.	52		// packedRefs is an immutable parsed packed-refs file, sorted by name.
52		type packedRefs struct {	53		type packedRefs struct {
53		sig fileSig	54		sig fileSig
			55	+	raw string // the file content: the names in refs point into it
54		refs []packedRef	56		refs []packedRef
55		}	57		}
56			58

@@ -65,7 +67,8 @@ func (p *packedRefs) find(name string) (packedRef, bool) {
65		}	67		}
66			68
67		// loadPacked returns the current content of packed-refs, reparsing the file	69		// loadPacked returns the current content of packed-refs, reparsing the file
68	-	// when its stat data changed (or always with force).	70	+	// when its stat data changed (or always with force). Concurrent callers share
			71	+	// one parse: the check is repeated once pmu is held.
69		func (r Repo) loadPacked(force bool) (packedRefs, error) {	72		func (r Repo) loadPacked(force bool) (packedRefs, error) {
70		path := r.dir + "/packed-refs"	73		path := r.dir + "/packed-refs"
71		sig, err := statSig(path)	74		sig, err := statSig(path)

@@ -77,9 +80,17 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
77		}	80		}
78		r.pmu.Lock()	81		r.pmu.Lock()
79		defer r.pmu.Unlock()	82		defer r.pmu.Unlock()
			83	+	cur := r.packed.Load()
			84	+	if !force && cur != nil && (cur.sig.read > sig.read \|\| cur.sig.same(sig)) {
			85	+	// Another caller parsed the file while we waited for the lock. It read
			86	+	// the file after we looked at it, so its result is at least as fresh
			87	+	// as ours would be.
			88	+	return cur, nil
			89	+	}
			90	+	read := time.Now().UnixNano()
80		f, err := os.Open(path)	91		f, err := os.Open(path)
81		if errors.Is(err, fs.ErrNotExist) {	92		if errors.Is(err, fs.ErrNotExist) {
82	-	p := &packedRefs{}	93	+	p := &packedRefs{sig: fileSig{read: read}}
83		r.packed.Store(p)	94		r.packed.Store(p)
84		return p, nil	95		return p, nil
85		}	96		}

@@ -98,6 +109,13 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
98		if _, err := io.ReadFull(f, data); err != nil {	109		if _, err := io.ReadFull(f, data); err != nil {
99		return nil, err	110		return nil, err
100		}	111		}
			112	+	if !force && cur != nil && cur.raw == string(data) {
			113	+	// Racy stat data, same content (Git's racy rule): keep the parse
			114	+	// and record that the file was read now.
			115	+	p := &packedRefs{sig: sigOf(st, read), raw: cur.raw, refs: cur.refs}
			116	+	r.packed.Store(p)
			117	+	return p, nil
			118	+	}
101		p, err := parsePackedRefs(string(data))	119		p, err := parsePackedRefs(string(data))
102		if err != nil {	120		if err != nil {
103		return nil, err	121		return nil, err

@@ -107,7 +125,7 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
107		}); i < len(p.refs) && strings.HasPrefix(p.refs[i].name, "refs/replace/") {	125		}); i < len(p.refs) && strings.HasPrefix(p.refs[i].name, "refs/replace/") {
108		return nil, unsupported("replace refs (refs/replace in packed-refs)")	126		return nil, unsupported("replace refs (refs/replace in packed-refs)")
109		}	127		}
110	-	p.sig = sigOf(st)	128	+	p.sig = sigOf(st, read)
111		r.packed.Store(p)	129		r.packed.Store(p)
112		return p, nil	130		return p, nil
113		}	131		}

@@ -115,7 +133,7 @@ func (r Repo) loadPacked(force bool) (packedRefs, error) {
115		// parsePackedRefs parses a packed-refs file. The ref names are substrings of	133		// parsePackedRefs parses a packed-refs file. The ref names are substrings of
116		// data.	134		// data.
117		func parsePackedRefs(data string) (*packedRefs, error) {	135		func parsePackedRefs(data string) (*packedRefs, error) {
118	-	p := &packedRefs{}	136	+	p := &packedRefs{raw: data}
119		fullyPeeled, peeled := false, false	137		fullyPeeled, peeled := false, false
120		first := true	138		first := true
121		for len(data) > 0 {	139		for len(data) > 0 {

@@ -276,52 +294,100 @@ func validRootRef(name string) bool {
276		// validRef accepts the names lookupRef may read.	294		// validRef accepts the names lookupRef may read.
277		func validRef(name string) bool { return validRefName(name) \|\| validRootRef(name) }	295		func validRef(name string) bool { return validRefName(name) \|\| validRootRef(name) }
278			296
279	-	// lookupRef resolves name through symbolic refs. loose refs win over packed	297	+	// refWalk is a ref lookup that went through the loose files and may still
280	-	// ones. The loose file is read before packed-refs, so a concurrent pack-refs	298	+	// need packed-refs.
281	-	// (which writes packed-refs first and deletes the loose file after) is never	299	+	type refWalk struct {
282	-	// missed.	300	+	ref Ref
283	-	func (r *Repo) lookupRef(name string) (Ref, bool, error) {	301	+	cur string // the name to look up in packed-refs when !done
			302	+	done bool // the lookup is complete: ok tells whether it found a ref
			303	+	ok bool
			304	+	}
			305	+
			306	+	// isRootRef reports whether name is a pseudo-ref of the Git directory
			307	+	// (HEAD, ORIG_HEAD...) rather than a name under refs/.
			308	+	func isRootRef(name string) bool { return !strings.HasPrefix(name, "refs/") }
			309	+
			310	+	// lookupLoose follows name through the loose files and symbolic refs. It
			311	+	// stops at a loose ref that holds an id, or at the first name that has no
			312	+	// loose file: packed-refs decides about that one (see finish).
			313	+	func (r *Repo) lookupLoose(name string) (refWalk, error) {
284		if !validRef(name) {	314		if !validRef(name) {
285	-	return Ref{}, false, nil	315	+	return refWalk{done: true}, nil
286		}	316		}
287	-	ref := Ref{Name: name}	317	+	w := refWalk{ref: Ref{Name: name}, cur: name}
288	-	cur := name
289		for depth := 0; ; depth++ {	318		for depth := 0; ; depth++ {
290		if depth > maxSymrefDepth {	319		if depth > maxSymrefDepth {
291	-	return Ref{}, false, corrupt("ref %s: symbolic ref loop", name)	320	+	return refWalk{}, corrupt("ref %s: symbolic ref loop", name)
292		}	321		}
293	-	lr, found, err := r.readLooseRef(cur)	322	+	lr, found, err := r.readLooseRef(w.cur)
294		if err != nil {	323		if err != nil {
295	-	return Ref{}, false, err	324	+	if errors.Is(err, ErrCorrupt) && isRootRef(w.cur) && w.cur != "HEAD" {
			325	+	// A pseudo-ref such as FETCH_HEAD or MERGE_HEAD can hold
			326	+	// anything (FETCH_HEAD has one line per fetched ref), and
			327	+	// a revision like "ABCDEF" may name such a file by
			328	+	// accident: it is not a ref, so not found. A broken HEAD
			329	+	// stays an error.
			330	+	return refWalk{done: true}, nil
			331	+	}
			332	+	return refWalk{}, err
296		}	333		}
297		if found && lr.symref != "" {	334		if found && lr.symref != "" {
298		if depth == 0 {	335		if depth == 0 {
299	-	ref.Symref = lr.symref	336	+	w.ref.Symref = lr.symref
300		}	337		}
301		if !validRef(lr.symref) {	338		if !validRef(lr.symref) {
302	-	return Ref{}, false, nil	339	+	return refWalk{done: true}, nil
303		}	340		}
304	-	cur = lr.symref	341	+	w.cur = lr.symref
305		continue	342		continue
306		}	343		}
307		if found {	344		if found {
308	-	ref.ID = lr.id	345	+	w.ref.ID = lr.id
309	-	return ref, true, nil	346	+	w.done, w.ok = true, true
			347	+	return w, nil
310		}	348		}
311	-	p, err := r.loadPacked(false)	349	+	return w, nil
312	-	if err != nil {	350	+	}
313	-	return Ref{}, false, err	351	+	}
314	-	}	352	+
315	-	e, ok := p.find(cur)	353	+	// finish completes a lookup with the content of packed-refs. The loose files
316	-	if !ok {	354	+	// have to be read before packed-refs is loaded: a concurrent pack-refs writes
317	-	return Ref{}, false, nil	355	+	// packed-refs first and deletes the loose file after, so it is never missed.
318	-	}	356	+	func (w refWalk) finish(p packedRefs) (Ref, bool) {
319	-	ref.ID = e.id	357	+	if w.done {
320	-	if depth == 0 {	358	+	return w.ref, w.ok
321	-	ref.Peeled, ref.PeeledKnown = e.peeled, e.peeledKnown	359	+	}
322	-	}	360	+	e, ok := p.find(w.cur)
323	-	return ref, true, nil	361	+	if !ok {
			362	+	return Ref{}, false
			363	+	}
			364	+	w.ref.ID = e.id
			365	+	if w.cur == w.ref.Name {
			366	+	w.ref.Peeled, w.ref.PeeledKnown = e.peeled, e.peeledKnown
			367	+	}
			368	+	return w.ref, true
			369	+	}
			370	+
			371	+	// lookupRef resolves name through symbolic refs. loose refs win over packed
			372	+	// ones.
			373	+	func (r *Repo) lookupRef(name string) (Ref, bool, error) {
			374	+	if r.snap.Load() == nil {
			375	+	return Ref{}, false, ErrClosed
			376	+	}
			377	+	w, err := r.lookupLoose(name)
			378	+	if err != nil {
			379	+	return Ref{}, false, err
			380	+	}
			381	+	if w.done {
			382	+	ref, ok := w.finish(nil)
			383	+	return ref, ok, nil
324		}	384		}
			385	+	p, err := r.loadPacked(false)
			386	+	if err != nil {
			387	+	return Ref{}, false, err
			388	+	}
			389	+	ref, ok := w.finish(p)
			390	+	return ref, ok, nil
325		}	391		}
326			392
327		// ResolveRef returns the object the ref with this full name (such as	393		// ResolveRef returns the object the ref with this full name (such as

@@ -356,6 +422,9 @@ func (r *Repo) LookupRef(name string) (Ref, error) {
356		// yet) is not an error: id is then the zero ObjectID. For a detached HEAD	422		// yet) is not an error: id is then the zero ObjectID. For a detached HEAD
357		// target is empty and id is the commit.	423		// target is empty and id is the commit.
358		func (r *Repo) Head() (target string, id ObjectID, err error) {	424		func (r *Repo) Head() (target string, id ObjectID, err error) {
			425	+	if r.snap.Load() == nil {
			426	+	return "", ObjectID{}, ErrClosed
			427	+	}
359		lr, found, err := r.readLooseRef("HEAD")	428		lr, found, err := r.readLooseRef("HEAD")
360		if err != nil {	429		if err != nil {
361		return "", ObjectID{}, err	430		return "", ObjectID{}, err

@@ -397,7 +466,9 @@ func (r *Repo) Refs(prefix string) iter.Seq2[Ref, error] {
397		}	466		}
398			467
399		func (r *Repo) collectRefs(prefix string) ([]Ref, error) {	468		func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
400	-	// Loose refs first, packed-refs after: see lookupRef.	469	+	if r.snap.Load() == nil {
			470	+	return nil, ErrClosed
			471	+	}
401		var dir string	472		var dir string
402		switch {	473		switch {
403		case strings.HasPrefix(prefix, "refs/"):	474		case strings.HasPrefix(prefix, "refs/"):

@@ -407,15 +478,33 @@ func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
407		default:	478		default:
408		return nil, nil	479		return nil, nil
409		}	480		}
			481	+	// Loose refs first, packed-refs after: see refWalk.finish.
410		var loose []string	482		var loose []string
411		if err := r.walkLoose(dir, prefix, &loose, 0); err != nil {	483		if err := r.walkLoose(dir, prefix, &loose, 0); err != nil {
412		return nil, err	484		return nil, err
413		}	485		}
			486	+	slices.Sort(loose)
			487	+	loose = slices.Compact(loose)
			488	+	type item struct {
			489	+	name string
			490	+	w refWalk
			491	+	}
			492	+	items := make([]item, 0, len(loose))
			493	+	for _, n := range loose {
			494	+	w, err := r.lookupLoose(n)
			495	+	if err != nil {
			496	+	if errors.Is(err, ErrCorrupt) {
			497	+	continue // Git ignores broken refs when listing
			498	+	}
			499	+	return nil, err
			500	+	}
			501	+	items = append(items, item{name: n, w: w})
			502	+	}
			503	+	// One load of packed-refs serves every name.
414		p, err := r.loadPacked(false)	504		p, err := r.loadPacked(false)
415		if err != nil {	505		if err != nil {
416		return nil, err	506		return nil, err
417		}	507		}
418	-	names := loose
419		start, _ := slices.BinarySearchFunc(p.refs, prefix, func(e packedRef, pre string) int {	508		start, _ := slices.BinarySearchFunc(p.refs, prefix, func(e packedRef, pre string) int {
420		if strings.HasPrefix(e.name, pre) {	509		if strings.HasPrefix(e.name, pre) {
421		return 0	510		return 0

@@ -427,23 +516,25 @@ func (r *Repo) collectRefs(prefix string) ([]Ref, error) {
427		for start > 0 && strings.HasPrefix(p.refs[start-1].name, prefix) {	516		for start > 0 && strings.HasPrefix(p.refs[start-1].name, prefix) {
428		start--	517		start--
429		}	518		}
			519	+	nLoose := len(items)
430		for i := start; i < len(p.refs) && strings.HasPrefix(p.refs[i].name, prefix); i++ {	520		for i := start; i < len(p.refs) && strings.HasPrefix(p.refs[i].name, prefix); i++ {
431	-	names = append(names, p.refs[i].name)	521	+	n := p.refs[i].name
432	-	}	522	+	if _, ok := slices.BinarySearch(loose, n); ok {
433	-	slices.Sort(names)	523	+	continue // the loose file wins; its walk is already in items
434	-	names = slices.Compact(names)
435	-
436	-	out := make([]Ref, 0, len(names))
437	-	for _, n := range names {
438	-	ref, ok, err := r.lookupRef(n)
439	-	if err != nil {
440	-	if errors.Is(err, ErrCorrupt) {
441	-	continue // Git ignores broken refs when listing
442	-	}
443	-	return nil, err
444		}	524		}
445	-	if ok {	525	+	if !validRef(n) {
446	-	ref.Name = n	526	+	continue
			527	+	}
			528	+	items = append(items, item{name: n, w: refWalk{ref: Ref{Name: n}, cur: n}})
			529	+	}
			530	+	if len(items) > nLoose {
			531	+	slices.SortFunc(items, func(a, b item) int { return strings.Compare(a.name, b.name) })
			532	+	}
			533	+	out := make([]Ref, 0, len(items))
			534	+	for i := range items {
			535	+	it := &items[i]
			536	+	if ref, ok := it.w.finish(p); ok {
			537	+	ref.Name = it.name
447		out = append(out, ref)	538		out = append(out, ref)
448		}	539		}
449		}	540		}

@@ -500,11 +591,25 @@ func (r *Repo) PeelRef(ref Ref) (ObjectID, error) {
500		}	591		}
501			592
502		// Resolve turns a revision name into an object id the way "git rev-parse"	593		// Resolve turns a revision name into an object id the way "git rev-parse"
503	-	// does for plain names: a full 40 digit id, "HEAD", a full ref name, a short	594	+	// does for plain names. The result is not peeled; use PeelToCommit. Revision
504	-	// ref name tried as "<n>", "refs/<n>", "refs/tags/<n>", "refs/heads/<n>",	595	+	// operators (~, ^, @{}, :path) are not supported.
505	-	// "refs/remotes/<n>" and "refs/remotes/<n>/HEAD" in that order, and finally	596	+	//
506	-	// an abbreviated id of 4 to 39 digits. Revision operators (~, ^, @{}) are not	597	+	// The name is tried in git's order, and the first match wins:
507	-	// supported. The result is not peeled; use PeelToCommit.	598	+	//
			599	+	// 1. a full 40 digit id. It is taken as an id even when a ref has that name
			600	+	// (git warns that the ref is ambiguous and ignores it), and it must exist
			601	+	// in the repository, which "git rev-parse" does not check;
			602	+	// 2. a ref, as "<n>", "refs/<n>", "refs/tags/<n>", "refs/heads/<n>",
			603	+	// "refs/remotes/<n>" and "refs/remotes/<n>/HEAD", in that order. A tag
			604	+	// therefore wins over a branch of the same name, whereas "git checkout"
			605	+	// prefers the branch;
			606	+	// 3. an abbreviated id of 4 to 39 digits, which loses against any ref of
			607	+	// that name. It matches any object, whatever its type, and fails with
			608	+	// ErrAmbiguous if several do; see ResolveCommitPrefix.
			609	+	//
			610	+	// A caller that wants a branch or a tag, or a commit, should not use Resolve:
			611	+	// ResolveRef takes the full ref name and ResolveCommitPrefix only considers
			612	+	// commits. Resolve returns an error wrapping ErrNotFound if nothing matches.
508		func (r *Repo) Resolve(rev string) (ObjectID, error) {	613		func (r *Repo) Resolve(rev string) (ObjectID, error) {
509		if rev == "" {	614		if rev == "" {
510		return ObjectID{}, notFound("empty revision")	615		return ObjectID{}, notFound("empty revision")

A refs_cache_test.go +241 -0

@@ -0,0 +1,241 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"fmt"
	7	+	"os"
	8	+	"strings"
	9	+	"testing"
	10	+	"time"
	11	+	)
	12	+
	13	+	// writePackedRefs writes n branches, all on id, to packed-refs, in the format
	14	+	// of "git pack-refs --all".
	15	+	func writePackedRefs(t testing.TB, dir string, id ObjectID, n int) {
	16	+	t.Helper()
	17	+	var sb strings.Builder
	18	+	sb.WriteString("# pack-refs with: peeled fully-peeled sorted \n")
	19	+	for i := 0; i < n; i++ {
	20	+	fmt.Fprintf(&sb, "%s refs/heads/b%05d\n", id, i)
	21	+	}
	22	+	if err := os.WriteFile(dir+"/packed-refs", []byte(sb.String()), 0o644); err != nil {
	23	+	t.Fatal(err)
	24	+	}
	25	+	}
	26	+
	27	+	// Right after packed-refs was rewritten its stat data cannot be trusted, but
	28	+	// listing the refs must still parse it once, not once per ref.
	29	+	func TestRefsRacyPackedRefsParsedOnce(t *testing.T) {
	30	+	f := newFx(t)
	31	+	f.write("a", []byte("a"), 0o644)
	32	+	id := mustID(t, f.commit("one"))
	33	+	const n = 5000
	34	+	writePackedRefs(t, f.dir, id, n)
	35	+	r := f.open()
	36	+	first, err := r.loadPacked(false)
	37	+	if err != nil {
	38	+	t.Fatal(err)
	39	+	}
	40	+	if time.Now().UnixNano()-first.sig.mtime > int64(racyWindow) {
	41	+	t.Skip("slow machine: the file is no longer racy")
	42	+	}
	43	+	start := time.Now()
	44	+	count := 0
	45	+	for ref, err := range r.Refs("refs/heads/") {
	46	+	if err != nil {
	47	+	t.Fatal(err)
	48	+	}
	49	+	if ref.ID != id {
	50	+	t.Fatalf("%s = %s", ref.Name, ref.ID)
	51	+	}
	52	+	count++
	53	+	}
	54	+	took := time.Since(start)
	55	+	if count != n+1 { // plus the branch of the fixture
	56	+	t.Fatalf("%d refs, want %d", count, n+1)
	57	+	}
	58	+	again, err := r.loadPacked(false)
	59	+	if err != nil {
	60	+	t.Fatal(err)
	61	+	}
	62	+	if &again.refs[0] != &first.refs[0] {
	63	+	t.Error("packed-refs was parsed again although its content did not change")
	64	+	}
	65	+	// Before the fix every ref re-read and re-parsed the file: 0.7 s and more.
	66	+	if !raceEnabled && took > 250*time.Millisecond {
	67	+	t.Errorf("Refs over %d packed refs took %v right after a rewrite", n, took)
	68	+	}
	69	+	// A real change inside the racy window is seen.
	70	+	writePackedRefs(t, f.dir, id, n-1)
	71	+	changed, err := r.loadPacked(false)
	72	+	if err != nil {
	73	+	t.Fatal(err)
	74	+	}
	75	+	if len(changed.refs) != n-1 {
	76	+	t.Errorf("rewritten packed-refs has %d refs, want %d", len(changed.refs), n-1)
	77	+	}
	78	+	}
	79	+
	80	+	func TestLoadPackedConcurrentShareOneParse(t *testing.T) {
	81	+	f := newFx(t)
	82	+	f.write("a", []byte("a"), 0o644)
	83	+	id := mustID(t, f.commit("one"))
	84	+	writePackedRefs(t, f.dir, id, 2000)
	85	+	r := f.open()
	86	+	base, err := r.loadPacked(false)
	87	+	if err != nil {
	88	+	t.Fatal(err)
	89	+	}
	90	+	writePackedRefs(t, f.dir, id, 1999) // a different size
	91	+	res := make(chan *packedRefs, 16)
	92	+	for range 16 {
	93	+	go func() {
	94	+	p, err := r.loadPacked(false)
	95	+	if err != nil {
	96	+	t.Error(err)
	97	+	}
	98	+	res <- p
	99	+	}()
	100	+	}
	101	+	var got *packedRefs
	102	+	for range 16 {
	103	+	p := <-res
	104	+	if p == nil {
	105	+	continue
	106	+	}
	107	+	if len(p.refs) != 1999 \|\| p == base {
	108	+	t.Fatalf("stale packed-refs: %d refs", len(p.refs))
	109	+	}
	110	+	if got != nil && &p.refs[0] != &got.refs[0] {
	111	+	t.Fatal("concurrent callers parsed the file more than once")
	112	+	}
	113	+	got = p
	114	+	}
	115	+	}
	116	+
	117	+	// The racy rule runs from the time the file was read, not from now.
	118	+	func TestFileSigRacy(t *testing.T) {
	119	+	mtime := time.Now().Add(-time.Hour).UnixNano()
	120	+	w := int64(racyWindow)
	121	+	cases := []struct {
	122	+	name string
	123	+	read int64 // relative to mtime
	124	+	same bool
	125	+	}{
	126	+	{"read just after the write", 1, false},
	127	+	{"read inside the window", w / 2, false},
	128	+	{"read at the end of the window", w, false},
	129	+	{"read after the window", w + 1, true},
	130	+	}
	131	+	for _, c := range cases {
	132	+	a := fileSig{exists: true, size: 10, mtime: mtime, ino: 7, read: mtime + c.read}
	133	+	b := a
	134	+	b.read = time.Now().UnixNano() // when the new stat was taken: irrelevant
	135	+	// The check happens an hour after the read: the old code, which
	136	+	// measured from "now", trusted every one of these.
	137	+	if got := a.same(b); got != c.same {
	138	+	t.Errorf("%s: same = %v, want %v", c.name, got, c.same)
	139	+	}
	140	+	}
	141	+	a := fileSig{exists: true, size: 10, mtime: mtime, read: mtime + 10*w}
	142	+	for _, b := range []fileSig{
	143	+	{exists: true, size: 11, mtime: mtime},
	144	+	{exists: true, size: 10, mtime: mtime + 1},
	145	+	{exists: true, size: 10, mtime: mtime, ino: 1},
	146	+	{},
	147	+	} {
	148	+	if a.same(b) {
	149	+	t.Errorf("%+v taken for %+v", b, a)
	150	+	}
	151	+	}
	152	+	if gone := (fileSig{read: 1}); !gone.same(fileSig{read: 99}) {
	153	+	t.Error("a missing file is not the same as a missing file")
	154	+	}
	155	+	// A recorded signature carries the time of the read.
	156	+	f := newFx(t)
	157	+	writeFile(t, f.dir+"/probe", "x")
	158	+	before := time.Now().UnixNano()
	159	+	sig, err := statSig(f.dir + "/probe")
	160	+	if err != nil {
	161	+	t.Fatal(err)
	162	+	}
	163	+	if sig.read < before \|\| sig.read > time.Now().UnixNano() {
	164	+	t.Errorf("read = %d, want a time between %d and now", sig.read, before)
	165	+	}
	166	+	}
	167	+
	168	+	// A pack directory that was racy when the list was built is trusted again
	169	+	// once a reload has seen it unchanged after the window.
	170	+	func TestPackDirRacyUntilReread(t *testing.T) {
	171	+	f := newFx(t)
	172	+	f.write("a", []byte("a"), 0o644)
	173	+	f.commit("one")
	174	+	f.git("repack", "-adq")
	175	+	r := f.open()
	176	+	s := r.snap.Load()
	177	+	// Pretend the directory was modified right before the list was read.
	178	+	r.mu.Lock()
	179	+	s.dirSig.mtime = s.dirSig.read - int64(time.Millisecond)
	180	+	r.mu.Unlock()
	181	+	if s.dirSig.same(s.dirSig) {
	182	+	t.Fatal("a signature read 1ms after the change must not be trusted")
	183	+	}
	184	+	before := s.dirSig.read
	185	+	// The stat data is the real one, but the stored mtime was faked: the
	186	+	// reload sees a different directory and installs a new snapshot, which
	187	+	// carries the time of its own read.
	188	+	if _, err := r.reloadPacks(false); err != nil {
	189	+	t.Fatal(err)
	190	+	}
	191	+	if got := r.snap.Load().dirSig.read; got <= before {
	192	+	t.Errorf("dirSig.read = %d, was %d: not refreshed", got, before)
	193	+	}
	194	+	}
	195	+
	196	+	// BenchmarkRefsPackedRefs lists 5000 packed refs with a steady packed-refs
	197	+	// and right after it was rewritten (inside the racy window).
	198	+	func BenchmarkRefsPackedRefs(b *testing.B) {
	199	+	requireGit(b)
	200	+	dir := b.TempDir()
	201	+	runGit(b, dir, nil, nil, "init", "-q")
	202	+	runGit(b, dir, nil, nil, "commit", "-q", "--allow-empty", "-m", "x")
	203	+	id := mustID(b, strings.TrimSpace(string(runGit(b, dir, nil, nil, "rev-parse", "HEAD"))))
	204	+	const n = 5000
	205	+	writePackedRefs(b, dir+"/.git", id, n)
	206	+	r, err := Open(dir)
	207	+	if err != nil {
	208	+	b.Fatal(err)
	209	+	}
	210	+	defer func() { _ = r.Close() }()
	211	+	list := func(b *testing.B) {
	212	+	count := 0
	213	+	for _, err := range r.Refs("refs/heads/") {
	214	+	if err != nil {
	215	+	b.Fatal(err)
	216	+	}
	217	+	count++
	218	+	}
	219	+	if count != n+1 {
	220	+	b.Fatalf("%d refs", count)
	221	+	}
	222	+	}
	223	+	for _, c := range []struct {
	224	+	name string
	225	+	age time.Duration
	226	+	}{{"steady", time.Hour}, {"just-rewritten", 0}} {
	227	+	b.Run(c.name, func(b *testing.B) {
	228	+	mt := time.Now().Add(-c.age)
	229	+	if err := os.Chtimes(dir+"/.git/packed-refs", mt, mt); err != nil {
	230	+	b.Fatal(err)
	231	+	}
	232	+	if _, err := r.loadPacked(true); err != nil {
	233	+	b.Fatal(err)
	234	+	}
	235	+	b.ReportAllocs()
	236	+	for b.Loop() {
	237	+	list(b)
	238	+	}
	239	+	})
	240	+	}
	241	+	}

@@ -0,0 +1,241 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"fmt"
	7	+	"os"
	8	+	"strings"
	9	+	"testing"
	10	+	"time"
	11	+	)
	12	+
	13	+	// writePackedRefs writes n branches, all on id, to packed-refs, in the format
	14	+	// of "git pack-refs --all".
	15	+	func writePackedRefs(t testing.TB, dir string, id ObjectID, n int) {
	16	+	t.Helper()
	17	+	var sb strings.Builder
	18	+	sb.WriteString("# pack-refs with: peeled fully-peeled sorted \n")
	19	+	for i := 0; i < n; i++ {
	20	+	fmt.Fprintf(&sb, "%s refs/heads/b%05d\n", id, i)
	21	+	}
	22	+	if err := os.WriteFile(dir+"/packed-refs", []byte(sb.String()), 0o644); err != nil {
	23	+	t.Fatal(err)
	24	+	}
	25	+	}
	26	+
	27	+	// Right after packed-refs was rewritten its stat data cannot be trusted, but
	28	+	// listing the refs must still parse it once, not once per ref.
	29	+	func TestRefsRacyPackedRefsParsedOnce(t *testing.T) {
	30	+	f := newFx(t)
	31	+	f.write("a", []byte("a"), 0o644)
	32	+	id := mustID(t, f.commit("one"))
	33	+	const n = 5000
	34	+	writePackedRefs(t, f.dir, id, n)
	35	+	r := f.open()
	36	+	first, err := r.loadPacked(false)
	37	+	if err != nil {
	38	+	t.Fatal(err)
	39	+	}
	40	+	if time.Now().UnixNano()-first.sig.mtime > int64(racyWindow) {
	41	+	t.Skip("slow machine: the file is no longer racy")
	42	+	}
	43	+	start := time.Now()
	44	+	count := 0
	45	+	for ref, err := range r.Refs("refs/heads/") {
	46	+	if err != nil {
	47	+	t.Fatal(err)
	48	+	}
	49	+	if ref.ID != id {
	50	+	t.Fatalf("%s = %s", ref.Name, ref.ID)
	51	+	}
	52	+	count++
	53	+	}
	54	+	took := time.Since(start)
	55	+	if count != n+1 { // plus the branch of the fixture
	56	+	t.Fatalf("%d refs, want %d", count, n+1)
	57	+	}
	58	+	again, err := r.loadPacked(false)
	59	+	if err != nil {
	60	+	t.Fatal(err)
	61	+	}
	62	+	if &again.refs[0] != &first.refs[0] {
	63	+	t.Error("packed-refs was parsed again although its content did not change")
	64	+	}
	65	+	// Before the fix every ref re-read and re-parsed the file: 0.7 s and more.
	66	+	if !raceEnabled && took > 250*time.Millisecond {
	67	+	t.Errorf("Refs over %d packed refs took %v right after a rewrite", n, took)
	68	+	}
	69	+	// A real change inside the racy window is seen.
	70	+	writePackedRefs(t, f.dir, id, n-1)
	71	+	changed, err := r.loadPacked(false)
	72	+	if err != nil {
	73	+	t.Fatal(err)
	74	+	}
	75	+	if len(changed.refs) != n-1 {
	76	+	t.Errorf("rewritten packed-refs has %d refs, want %d", len(changed.refs), n-1)
	77	+	}
	78	+	}
	79	+
	80	+	func TestLoadPackedConcurrentShareOneParse(t *testing.T) {
	81	+	f := newFx(t)
	82	+	f.write("a", []byte("a"), 0o644)
	83	+	id := mustID(t, f.commit("one"))
	84	+	writePackedRefs(t, f.dir, id, 2000)
	85	+	r := f.open()
	86	+	base, err := r.loadPacked(false)
	87	+	if err != nil {
	88	+	t.Fatal(err)
	89	+	}
	90	+	writePackedRefs(t, f.dir, id, 1999) // a different size
	91	+	res := make(chan *packedRefs, 16)
	92	+	for range 16 {
	93	+	go func() {
	94	+	p, err := r.loadPacked(false)
	95	+	if err != nil {
	96	+	t.Error(err)
	97	+	}
	98	+	res <- p
	99	+	}()
	100	+	}
	101	+	var got *packedRefs
	102	+	for range 16 {
	103	+	p := <-res
	104	+	if p == nil {
	105	+	continue
	106	+	}
	107	+	if len(p.refs) != 1999 \|\| p == base {
	108	+	t.Fatalf("stale packed-refs: %d refs", len(p.refs))
	109	+	}
	110	+	if got != nil && &p.refs[0] != &got.refs[0] {
	111	+	t.Fatal("concurrent callers parsed the file more than once")
	112	+	}
	113	+	got = p
	114	+	}
	115	+	}
	116	+
	117	+	// The racy rule runs from the time the file was read, not from now.
	118	+	func TestFileSigRacy(t *testing.T) {
	119	+	mtime := time.Now().Add(-time.Hour).UnixNano()
	120	+	w := int64(racyWindow)
	121	+	cases := []struct {
	122	+	name string
	123	+	read int64 // relative to mtime
	124	+	same bool
	125	+	}{
	126	+	{"read just after the write", 1, false},
	127	+	{"read inside the window", w / 2, false},
	128	+	{"read at the end of the window", w, false},
	129	+	{"read after the window", w + 1, true},
	130	+	}
	131	+	for _, c := range cases {
	132	+	a := fileSig{exists: true, size: 10, mtime: mtime, ino: 7, read: mtime + c.read}
	133	+	b := a
	134	+	b.read = time.Now().UnixNano() // when the new stat was taken: irrelevant
	135	+	// The check happens an hour after the read: the old code, which
	136	+	// measured from "now", trusted every one of these.
	137	+	if got := a.same(b); got != c.same {
	138	+	t.Errorf("%s: same = %v, want %v", c.name, got, c.same)
	139	+	}
	140	+	}
	141	+	a := fileSig{exists: true, size: 10, mtime: mtime, read: mtime + 10*w}
	142	+	for _, b := range []fileSig{
	143	+	{exists: true, size: 11, mtime: mtime},
	144	+	{exists: true, size: 10, mtime: mtime + 1},
	145	+	{exists: true, size: 10, mtime: mtime, ino: 1},
	146	+	{},
	147	+	} {
	148	+	if a.same(b) {
	149	+	t.Errorf("%+v taken for %+v", b, a)
	150	+	}
	151	+	}
	152	+	if gone := (fileSig{read: 1}); !gone.same(fileSig{read: 99}) {
	153	+	t.Error("a missing file is not the same as a missing file")
	154	+	}
	155	+	// A recorded signature carries the time of the read.
	156	+	f := newFx(t)
	157	+	writeFile(t, f.dir+"/probe", "x")
	158	+	before := time.Now().UnixNano()
	159	+	sig, err := statSig(f.dir + "/probe")
	160	+	if err != nil {
	161	+	t.Fatal(err)
	162	+	}
	163	+	if sig.read < before \|\| sig.read > time.Now().UnixNano() {
	164	+	t.Errorf("read = %d, want a time between %d and now", sig.read, before)
	165	+	}
	166	+	}
	167	+
	168	+	// A pack directory that was racy when the list was built is trusted again
	169	+	// once a reload has seen it unchanged after the window.
	170	+	func TestPackDirRacyUntilReread(t *testing.T) {
	171	+	f := newFx(t)
	172	+	f.write("a", []byte("a"), 0o644)
	173	+	f.commit("one")
	174	+	f.git("repack", "-adq")
	175	+	r := f.open()
	176	+	s := r.snap.Load()
	177	+	// Pretend the directory was modified right before the list was read.
	178	+	r.mu.Lock()
	179	+	s.dirSig.mtime = s.dirSig.read - int64(time.Millisecond)
	180	+	r.mu.Unlock()
	181	+	if s.dirSig.same(s.dirSig) {
	182	+	t.Fatal("a signature read 1ms after the change must not be trusted")
	183	+	}
	184	+	before := s.dirSig.read
	185	+	// The stat data is the real one, but the stored mtime was faked: the
	186	+	// reload sees a different directory and installs a new snapshot, which
	187	+	// carries the time of its own read.
	188	+	if _, err := r.reloadPacks(false); err != nil {
	189	+	t.Fatal(err)
	190	+	}
	191	+	if got := r.snap.Load().dirSig.read; got <= before {
	192	+	t.Errorf("dirSig.read = %d, was %d: not refreshed", got, before)
	193	+	}
	194	+	}
	195	+
	196	+	// BenchmarkRefsPackedRefs lists 5000 packed refs with a steady packed-refs
	197	+	// and right after it was rewritten (inside the racy window).
	198	+	func BenchmarkRefsPackedRefs(b *testing.B) {
	199	+	requireGit(b)
	200	+	dir := b.TempDir()
	201	+	runGit(b, dir, nil, nil, "init", "-q")
	202	+	runGit(b, dir, nil, nil, "commit", "-q", "--allow-empty", "-m", "x")
	203	+	id := mustID(b, strings.TrimSpace(string(runGit(b, dir, nil, nil, "rev-parse", "HEAD"))))
	204	+	const n = 5000
	205	+	writePackedRefs(b, dir+"/.git", id, n)
	206	+	r, err := Open(dir)
	207	+	if err != nil {
	208	+	b.Fatal(err)
	209	+	}
	210	+	defer func() { _ = r.Close() }()
	211	+	list := func(b *testing.B) {
	212	+	count := 0
	213	+	for _, err := range r.Refs("refs/heads/") {
	214	+	if err != nil {
	215	+	b.Fatal(err)
	216	+	}
	217	+	count++
	218	+	}
	219	+	if count != n+1 {
	220	+	b.Fatalf("%d refs", count)
	221	+	}
	222	+	}
	223	+	for _, c := range []struct {
	224	+	name string
	225	+	age time.Duration
	226	+	}{{"steady", time.Hour}, {"just-rewritten", 0}} {
	227	+	b.Run(c.name, func(b *testing.B) {
	228	+	mt := time.Now().Add(-c.age)
	229	+	if err := os.Chtimes(dir+"/.git/packed-refs", mt, mt); err != nil {
	230	+	b.Fatal(err)
	231	+	}
	232	+	if _, err := r.loadPacked(true); err != nil {
	233	+	b.Fatal(err)
	234	+	}
	235	+	b.ReportAllocs()
	236	+	for b.Loop() {
	237	+	list(b)
	238	+	}
	239	+	})
	240	+	}
	241	+	}

A refs_fix_test.go +84 -0

@@ -0,0 +1,84 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"testing"
	8	+	)
	9	+
	10	+	func TestCorruptRootRefIsNotFound(t *testing.T) {
	11	+	f := newFx(t)
	12	+	f.write("a", []byte("a"), 0o644)
	13	+	c := mustID(t, f.commit("one"))
	14	+	writeFile(t, f.dir+"/GARBAGE", "this is not a ref\n")
	15	+	writeFile(t, f.dir+"/GOOD_REF", c.String()+"\n")
	16	+	writeFile(t, f.dir+"/refs/heads/sym", "ref: GARBAGE\n")
	17	+	r := f.open()
	18	+	if _, err := r.ResolveRef("GARBAGE"); !errors.Is(err, ErrNotFound) {
	19	+	t.Errorf("ResolveRef(GARBAGE) = %v, want ErrNotFound", err)
	20	+	}
	21	+	if _, err := r.LookupRef("GARBAGE"); !errors.Is(err, ErrNotFound) {
	22	+	t.Errorf("LookupRef(GARBAGE) = %v, want ErrNotFound", err)
	23	+	}
	24	+	if _, err := r.Resolve("GARBAGE"); !errors.Is(err, ErrNotFound) {
	25	+	t.Errorf("Resolve(GARBAGE) = %v, want ErrNotFound", err)
	26	+	}
	27	+	if got, err := r.ResolveRef("GOOD_REF"); err != nil \|\| got != c {
	28	+	t.Errorf("ResolveRef(GOOD_REF) = %s, %v", got, err)
	29	+	}
	30	+	// A symbolic ref to a broken pseudo-ref designates nothing; it does not
	31	+	// break the listing.
	32	+	n := 0
	33	+	for ref, err := range r.Refs("refs/heads/") {
	34	+	if err != nil {
	35	+	t.Fatal(err)
	36	+	}
	37	+	if ref.Name == "refs/heads/sym" {
	38	+	t.Errorf("sym resolved to %s", ref.ID)
	39	+	}
	40	+	n++
	41	+	}
	42	+	if n != 1 {
	43	+	t.Errorf("%d branches listed, want 1", n)
	44	+	}
	45	+	// A broken loose ref under refs/ stays corrupt, and HEAD too.
	46	+	writeFile(t, f.dir+"/refs/heads/broken", "nonsense\n")
	47	+	if _, err := r.ResolveRef("refs/heads/broken"); !errors.Is(err, ErrCorrupt) {
	48	+	t.Errorf("ResolveRef(broken) = %v, want ErrCorrupt", err)
	49	+	}
	50	+	writeFile(t, f.dir+"/HEAD", "nonsense\n")
	51	+	if _, err := r.ResolveRef("HEAD"); !errors.Is(err, ErrCorrupt) {
	52	+	t.Errorf("ResolveRef(HEAD) = %v, want ErrCorrupt", err)
	53	+	}
	54	+	}
	55	+
	56	+	func TestClosedRepoRefs(t *testing.T) {
	57	+	f := newFx(t)
	58	+	f.write("a", []byte("a"), 0o644)
	59	+	f.commit("one")
	60	+	r := f.open()
	61	+	_ = r.Close()
	62	+	if _, _, err := r.Head(); !errors.Is(err, ErrClosed) {
	63	+	t.Errorf("Head = %v", err)
	64	+	}
	65	+	if _, err := r.ResolveRef("HEAD"); !errors.Is(err, ErrClosed) {
	66	+	t.Errorf("ResolveRef = %v", err)
	67	+	}
	68	+	if _, err := r.LookupRef("refs/heads/main"); !errors.Is(err, ErrClosed) {
	69	+	t.Errorf("LookupRef = %v", err)
	70	+	}
	71	+	if _, err := r.Resolve("main"); !errors.Is(err, ErrClosed) {
	72	+	t.Errorf("Resolve = %v", err)
	73	+	}
	74	+	n := 0
	75	+	for _, err := range r.Refs("") {
	76	+	n++
	77	+	if !errors.Is(err, ErrClosed) {
	78	+	t.Errorf("Refs = %v", err)
	79	+	}
	80	+	}
	81	+	if n != 1 {
	82	+	t.Errorf("Refs yielded %d times, want one error", n)
	83	+	}
	84	+	}

@@ -0,0 +1,84 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"testing"
	8	+	)
	9	+
	10	+	func TestCorruptRootRefIsNotFound(t *testing.T) {
	11	+	f := newFx(t)
	12	+	f.write("a", []byte("a"), 0o644)
	13	+	c := mustID(t, f.commit("one"))
	14	+	writeFile(t, f.dir+"/GARBAGE", "this is not a ref\n")
	15	+	writeFile(t, f.dir+"/GOOD_REF", c.String()+"\n")
	16	+	writeFile(t, f.dir+"/refs/heads/sym", "ref: GARBAGE\n")
	17	+	r := f.open()
	18	+	if _, err := r.ResolveRef("GARBAGE"); !errors.Is(err, ErrNotFound) {
	19	+	t.Errorf("ResolveRef(GARBAGE) = %v, want ErrNotFound", err)
	20	+	}
	21	+	if _, err := r.LookupRef("GARBAGE"); !errors.Is(err, ErrNotFound) {
	22	+	t.Errorf("LookupRef(GARBAGE) = %v, want ErrNotFound", err)
	23	+	}
	24	+	if _, err := r.Resolve("GARBAGE"); !errors.Is(err, ErrNotFound) {
	25	+	t.Errorf("Resolve(GARBAGE) = %v, want ErrNotFound", err)
	26	+	}
	27	+	if got, err := r.ResolveRef("GOOD_REF"); err != nil \|\| got != c {
	28	+	t.Errorf("ResolveRef(GOOD_REF) = %s, %v", got, err)
	29	+	}
	30	+	// A symbolic ref to a broken pseudo-ref designates nothing; it does not
	31	+	// break the listing.
	32	+	n := 0
	33	+	for ref, err := range r.Refs("refs/heads/") {
	34	+	if err != nil {
	35	+	t.Fatal(err)
	36	+	}
	37	+	if ref.Name == "refs/heads/sym" {
	38	+	t.Errorf("sym resolved to %s", ref.ID)
	39	+	}
	40	+	n++
	41	+	}
	42	+	if n != 1 {
	43	+	t.Errorf("%d branches listed, want 1", n)
	44	+	}
	45	+	// A broken loose ref under refs/ stays corrupt, and HEAD too.
	46	+	writeFile(t, f.dir+"/refs/heads/broken", "nonsense\n")
	47	+	if _, err := r.ResolveRef("refs/heads/broken"); !errors.Is(err, ErrCorrupt) {
	48	+	t.Errorf("ResolveRef(broken) = %v, want ErrCorrupt", err)
	49	+	}
	50	+	writeFile(t, f.dir+"/HEAD", "nonsense\n")
	51	+	if _, err := r.ResolveRef("HEAD"); !errors.Is(err, ErrCorrupt) {
	52	+	t.Errorf("ResolveRef(HEAD) = %v, want ErrCorrupt", err)
	53	+	}
	54	+	}
	55	+
	56	+	func TestClosedRepoRefs(t *testing.T) {
	57	+	f := newFx(t)
	58	+	f.write("a", []byte("a"), 0o644)
	59	+	f.commit("one")
	60	+	r := f.open()
	61	+	_ = r.Close()
	62	+	if _, _, err := r.Head(); !errors.Is(err, ErrClosed) {
	63	+	t.Errorf("Head = %v", err)
	64	+	}
	65	+	if _, err := r.ResolveRef("HEAD"); !errors.Is(err, ErrClosed) {
	66	+	t.Errorf("ResolveRef = %v", err)
	67	+	}
	68	+	if _, err := r.LookupRef("refs/heads/main"); !errors.Is(err, ErrClosed) {
	69	+	t.Errorf("LookupRef = %v", err)
	70	+	}
	71	+	if _, err := r.Resolve("main"); !errors.Is(err, ErrClosed) {
	72	+	t.Errorf("Resolve = %v", err)
	73	+	}
	74	+	n := 0
	75	+	for _, err := range r.Refs("") {
	76	+	n++
	77	+	if !errors.Is(err, ErrClosed) {
	78	+	t.Errorf("Refs = %v", err)
	79	+	}
	80	+	}
	81	+	if n != 1 {
	82	+	t.Errorf("Refs yielded %d times, want one error", n)
	83	+	}
	84	+	}

M snapshot.go +30 -10


@@ -13,40 +13,55 @@ import (
13	13		"time"
14	14		)
15	15
16		-	// fileSig identifies a version of a file or directory by stat data.
	16	+	// fileSig identifies a version of a file or directory by stat data, and
	17	+	// remembers when that data was read.
17	18		type fileSig struct {
18	19		exists bool
19	20		size int64
20	21		mtime int64 // unix nanoseconds
21	22		ino uint64
	23	+	read int64 // unix nanoseconds: when the file was stat'ed or read
22	24		}
23	25
24		-	func sigOf(fi os.FileInfo) fileSig {
25		-	return fileSig{exists: true, size: fi.Size(), mtime: fi.ModTime().UnixNano(), ino: fileIno(fi)}
	26	+	// sigOf returns the signature of fi, which was obtained at read (unix
	27	+	// nanoseconds). Take read before reading the content: a change that happens
	28	+	// later is then never mistaken for the version that was read.
	29	+	func sigOf(fi os.FileInfo, read int64) fileSig {
	30	+	return fileSig{exists: true, size: fi.Size(), mtime: fi.ModTime().UnixNano(), ino: fileIno(fi), read: read}
26	31		}
27	32
28	33		func statSig(path string) (fileSig, error) {
	34	+	read := time.Now().UnixNano()
29	35		fi, err := os.Stat(path)
30	36		if errors.Is(err, fs.ErrNotExist) {
31		-	return fileSig{}, nil
	37	+	return fileSig{read: read}, nil
32	38		}
33	39		if err != nil {
34	40		return fileSig{}, err
35	41		}
36		-	return sigOf(fi), nil
	42	+	return sigOf(fi, read), nil
37	43		}
38	44
39	45		// racyWindow is how long after its modification time a file may still change
40	46		// without its stat data changing, because of coarse file system clocks.
41	47		const racyWindow = 2 * time.Second
42	48
	49	+	// sameStat reports whether a and b have the same stat data, whenever they
	50	+	// were read.
	51	+	func (a fileSig) sameStat(b fileSig) bool {
	52	+	return a.exists == b.exists && a.size == b.size && a.mtime == b.mtime && a.ino == b.ino
	53	+	}
	54	+
43	55		// same reports whether b is the same version as a and can be trusted to be
44		-	// unchanged.
	56	+	// unchanged. This is Git's racy rule: a was read at a.read, and a write that
	57	+	// follows within the clock granularity of the previous modification leaves
	58	+	// the stat data as it was. So the stat data only proves that nothing
	59	+	// changed when the file was read more than racyWindow after its mtime.
45	60		func (a fileSig) same(b fileSig) bool {
46		-	if a != b {
	61	+	if !a.sameStat(b) {
47	62		return false
48	63		}
49		-	return !a.exists \|\| time.Since(time.Unix(0, a.mtime)) > racyWindow
	64	+	return !a.exists \|\| a.read-a.mtime > int64(racyWindow)
50	65		}
51	66
52	67		func ignoreMissing(err error) error {

@@ -162,7 +177,7 @@ func (r *Repo) reloadPacks(force bool) (bool, error) {
162	177		if err != nil {
163	178		return false, err
164	179		}
165		-	if p := old[name]; p != nil && p.sig == isig {
	180	+	if p := old[name]; p != nil && p.sig.sameStat(isig) {
166	181		p.retain()
167	182		packs = append(packs, p)
168	183		continue

@@ -199,7 +214,12 @@ func (r *Repo) reloadPacks(force bool) (bool, error) {
199	214		for _, p := range packs {
200	215		p.release()
201	216		}
202		-	if cur.dirSig == dsig {
	217	+	if cur.dirSig.sameStat(dsig) {
	218	+	// The listing above was made after dsig was taken: record it, so
	219	+	// that a directory that was racy when first read stops being
	220	+	// rescanned once its window has passed. dirSig is only used
	221	+	// with r.mu held.
	222	+	cur.dirSig = dsig
203	223		return false, nil
204	224		}
205	225		packs = cur.packs

 	"time"
 )
-// fileSig identifies a version of a file or directory by stat data.
+// fileSig identifies a version of a file or directory by stat data, and
+// remembers when that data was read.
 type fileSig struct {
 	exists bool
 	size   int64
 	mtime  int64 // unix nanoseconds
 	ino    uint64
+	read   int64 // unix nanoseconds: when the file was stat'ed or read
 }
-func sigOf(fi os.FileInfo) fileSig {
+// sigOf returns the signature of fi, which was obtained at read (unix
-	return fileSig{exists: true, size: fi.Size(), mtime: fi.ModTime().UnixNano(), ino: fileIno(fi)}
+// nanoseconds). Take read before reading the content: a change that happens
+// later is then never mistaken for the version that was read.
+func sigOf(fi os.FileInfo, read int64) fileSig {
+	return fileSig{exists: true, size: fi.Size(), mtime: fi.ModTime().UnixNano(), ino: fileIno(fi), read: read}
 }
 func statSig(path string) (fileSig, error) {
+	read := time.Now().UnixNano()
 	fi, err := os.Stat(path)
 	if errors.Is(err, fs.ErrNotExist) {
-		return fileSig{}, nil
+		return fileSig{read: read}, nil
 	}
 	if err != nil {
 		return fileSig{}, err
 	}
-	return sigOf(fi), nil
+	return sigOf(fi, read), nil
 }
 // racyWindow is how long after its modification time a file may still change
 // without its stat data changing, because of coarse file system clocks.
 const racyWindow = 2 * time.Second
+// sameStat reports whether a and b have the same stat data, whenever they
+// were read.
+func (a fileSig) sameStat(b fileSig) bool {
+	return a.exists == b.exists && a.size == b.size && a.mtime == b.mtime && a.ino == b.ino
+}
 // same reports whether b is the same version as a and can be trusted to be
-// unchanged.
+// unchanged. This is Git's racy rule: a was read at a.read, and a write that
+// follows within the clock granularity of the previous modification leaves
+// the stat data as it was. So the stat data only proves that nothing
+// changed when the file was read more than racyWindow after its mtime.
 func (a fileSig) same(b fileSig) bool {
-	if a != b {
+	if !a.sameStat(b) {
 		return false
 	}
-	return !a.exists || time.Since(time.Unix(0, a.mtime)) > racyWindow
+	return !a.exists || a.read-a.mtime > int64(racyWindow)
 }
 func ignoreMissing(err error) error {
 		if err != nil {
 			return false, err
 		}
-		if p := old[name]; p != nil && p.sig == isig {
+		if p := old[name]; p != nil && p.sig.sameStat(isig) {
 			p.retain()
 			packs = append(packs, p)
 			continue
 		for _, p := range packs {
 			p.release()
 		}
-		if cur.dirSig == dsig {
+		if cur.dirSig.sameStat(dsig) {
+			// The listing above was made after dsig was taken: record it, so
+			// that a directory that was racy when first read stops being
+			// rescanned once its window has passed. dirSig is only used
+			// with r.mu held.
+			cur.dirSig = dsig
 			return false, nil
 		}
 		packs = cur.packs