fix(object): return the pack reload error instead of not found (GO-003, GO-010)

finder.again discarded the error of reloadPacks, so a failed reload during a
gc turned into ErrNotFound or HasObject=false. It is now returned by
ReadObject, ObjectHeader, OpenBlob, ResolvePrefix and Resolve. HasObject
returns (bool, error) since the answer can be unknown; the test hook
reloadPacksHook makes the reload fail.

ResolveCommitPrefix only skips ErrWrongType candidates and returns any other
error.

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

4 files · +145 -26

M blob.go +3 -1


@@ -40,7 +40,9 @@ func (r *Repo) OpenBlob(id ObjectID) (int64, io.ReadCloser, error) {
40	40		if !errors.Is(err, ErrNotFound) {
41	41		return 0, nil, err
42	42		}
43		-	if !f.again() {
	43	+	if again, err := f.again(); err != nil {
	44	+	return 0, nil, err
	45	+	} else if !again {
44	46		return 0, nil, notFound("object %s", id)
45	47		}
46	48		}

 		if !errors.Is(err, ErrNotFound) {
 			return 0, nil, err
 		}
-		if !f.again() {
+		if again, err := f.again(); err != nil {
+			return 0, nil, err
+		} else if !again {
 			return 0, nil, notFound("object %s", id)
 		}
 	}

M object.go +59 -24


@@ -26,48 +26,63 @@ func (f *finder) close() { f.s.release() }
26	26
27	27		func (f finder) pack(id ObjectID) (*pack, uint64, error) { return f.s.find(id) }
28	28
	29	+	// reloadPacksHook, when set by a test, replaces the reload done by again.
	30	+	var reloadPacksHook func(r *Repo) (bool, error)
	31	+
29	32		// again reloads the pack list if it changed and reports whether the lookup
30	33		// is worth repeating, that is whether the current snapshot differs from the
31	34		// one this finder used. Another goroutine may already have installed the new
32	35		// list, so the answer of reloadPacks alone is not enough. It returns true at
33		-	// most once.
34		-	func (f *finder) again() bool {
	36	+	// most once. An error from the reload is returned: the object may well exist
	37	+	// in a pack that could not be listed, so "not found" would be a wrong answer.
	38	+	func (f *finder) again() (bool, error) {
35	39		if f.retried {
36		-	return false
	40	+	return false, nil
37	41		}
38	42		f.retried = true
39		-	if _, err := f.r.reloadPacks(false); err != nil {
40		-	return false
	43	+	reload := f.r.reloadPacks
	44	+	if hook := reloadPacksHook; hook != nil {
	45	+	reload = func(bool) (bool, error) { return hook(f.r) }
	46	+	}
	47	+	if _, err := reload(false); err != nil {
	48	+	return false, err
41	49		}
42	50		s, err := f.r.acquire()
43	51		if err != nil {
44		-	return false
	52	+	return false, err
45	53		}
46	54		if s == f.s {
47	55		s.release()
48		-	return false
	56	+	return false, nil
49	57		}
50	58		f.s.release()
51	59		f.s = s
52		-	return true
	60	+	return true, nil
53	61		}
54	62
55		-	// HasObject reports whether the object exists.
56		-	func (r *Repo) HasObject(id ObjectID) bool {
	63	+	// HasObject reports whether the object exists. The error is not nil when the
	64	+	// answer is unknown: the repository is closed, or the pack list could not be
	65	+	// reloaded while the object was missing from the current one (during a gc,
	66	+	// for example).
	67	+	func (r *Repo) HasObject(id ObjectID) (bool, error) {
57	68		f, err := r.finder()
58	69		if err != nil {
59		-	return false
	70	+	return false, err
60	71		}
61	72		defer f.close()
62	73		for {
63	74		if p, _, err := f.pack(&id); err != nil \|\| p != nil {
64		-	return err == nil
	75	+	return err == nil, err
65	76		}
66	77		if exists(r.loosePath(&id)) {
67		-	return true
	78	+	return true, nil
68	79		}
69		-	if !f.again() {
70		-	return false
	80	+	again, err := f.again()
	81	+	if err != nil {
	82	+	return false, err
	83	+	}
	84	+	if !again {
	85	+	return false, nil
71	86		}
72	87		}
73	88		}

@@ -100,7 +115,9 @@ func (r *Repo) ObjectHeader(id ObjectID) (Type, int64, error) {
100	115		if !errors.Is(err, ErrNotFound) {
101	116		return 0, 0, err
102	117		}
103		-	if !f.again() {
	118	+	if again, err := f.again(); err != nil {
	119	+	return 0, 0, err
	120	+	} else if !again {
104	121		return 0, 0, notFound("object %s", id.String())
105	122		}
106	123		}

@@ -134,7 +151,9 @@ func (r *Repo) ReadObject(id ObjectID, dst []byte) (Type, []byte, error) {
134	151		if !errors.Is(err, ErrNotFound) {
135	152		return 0, nil, err
136	153		}
137		-	if !f.again() {
	154	+	if again, err := f.again(); err != nil {
	155	+	return 0, nil, err
	156	+	} else if !again {
138	157		return 0, nil, notFound("object %s", id.String())
139	158		}
140	159		}

@@ -216,13 +235,16 @@ func (r *Repo) ResolvePrefix(hex string) (ObjectID, error) {
216	235		// that designate a commit: commits and tags that peel to a commit. It is what
217	236		// "git rev-parse <hex>^{commit}" does.
218	237		func (r *Repo) ResolveCommitPrefix(hex string) (ObjectID, error) {
219		-	return r.resolvePrefix(hex, func(id ObjectID) bool {
	238	+	return r.resolvePrefix(hex, func(id ObjectID) (bool, error) {
220	239		_, err := r.PeelToCommit(id)
221		-	return err == nil
	240	+	if errors.Is(err, ErrWrongType) {
	241	+	return false, nil
	242	+	}
	243	+	return err == nil, err
222	244		})
223	245		}
224	246
225		-	func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) bool) (ObjectID, error) {
	247	+	func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) (bool, error)) (ObjectID, error) {
226	248		n := len(hex)
227	249		if n < minPrefix \|\| n > 40 {
228	250		return ObjectID{}, fmt.Errorf("%w: %q must have %d to 40 hexadecimal digits", ErrInvalidID, truncate(hex), minPrefix)

@@ -249,6 +271,7 @@ func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) bool) (ObjectID,
249	271		for {
250	272		var found [2]ObjectID
251	273		count := 0
	274	+	var acceptErr error
252	275		add := func(id ObjectID) bool {
253	276		if count > 0 && found[0] == id {
254	277		return true

@@ -256,8 +279,15 @@ func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) bool) (ObjectID,
256	279		if count > 1 && found[1] == id {
257	280		return true
258	281		}
259		-	if accept != nil && !accept(id) {
260		-	return true
	282	+	if accept != nil {
	283	+	ok, err := accept(id)
	284	+	if err != nil {
	285	+	acceptErr = err
	286	+	return false
	287	+	}
	288	+	if !ok {
	289	+	return true
	290	+	}
261	291		}
262	292		found[count] = id
263	293		count++

@@ -269,18 +299,23 @@ func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) bool) (ObjectID,
269	299		break
270	300		}
271	301		}
272		-	if count < 2 {
	302	+	if count < 2 && acceptErr == nil {
273	303		if err := r.looseWithPrefix(hex, add); err != nil {
274	304		return ObjectID{}, err
275	305		}
276	306		}
	307	+	if acceptErr != nil {
	308	+	return ObjectID{}, acceptErr
	309	+	}
277	310		switch {
278	311		case count >= 2:
279	312		return ObjectID{}, fmt.Errorf("%w: %s matches several objects", ErrAmbiguous, hex)
280	313		case count == 1:
281	314		return found[0], nil
282	315		}
283		-	if !f.again() {
	316	+	if again, err := f.again(); err != nil {
	317	+	return ObjectID{}, err
	318	+	} else if !again {
284	319		return ObjectID{}, notFound("no object starts with %s", hex)
285	320		}
286	321		}

 func (f *finder) pack(id *ObjectID) (*pack, uint64, error) { return f.s.find(id) }
+// reloadPacksHook, when set by a test, replaces the reload done by again.
+var reloadPacksHook func(r *Repo) (bool, error)
 // again reloads the pack list if it changed and reports whether the lookup
 // is worth repeating, that is whether the current snapshot differs from the
 // one this finder used. Another goroutine may already have installed the new
 // list, so the answer of reloadPacks alone is not enough. It returns true at
-// most once.
+// most once. An error from the reload is returned: the object may well exist
-func (f *finder) again() bool {
+// in a pack that could not be listed, so "not found" would be a wrong answer.
+func (f *finder) again() (bool, error) {
 	if f.retried {
-		return false
+		return false, nil
 	}
 	f.retried = true
-	if _, err := f.r.reloadPacks(false); err != nil {
+	reload := f.r.reloadPacks
-		return false
+	if hook := reloadPacksHook; hook != nil {
+		reload = func(bool) (bool, error) { return hook(f.r) }
+	}
+	if _, err := reload(false); err != nil {
+		return false, err
 	}
 	s, err := f.r.acquire()
 	if err != nil {
-		return false
+		return false, err
 	}
 	if s == f.s {
 		s.release()
-		return false
+		return false, nil
 	}
 	f.s.release()
 	f.s = s
-	return true
+	return true, nil
 }
-// HasObject reports whether the object exists.
+// HasObject reports whether the object exists. The error is not nil when the
-func (r *Repo) HasObject(id ObjectID) bool {
+// answer is unknown: the repository is closed, or the pack list could not be
+// reloaded while the object was missing from the current one (during a gc,
+// for example).
+func (r *Repo) HasObject(id ObjectID) (bool, error) {
 	f, err := r.finder()
 	if err != nil {
-		return false
+		return false, err
 	}
 	defer f.close()
 	for {
 		if p, _, err := f.pack(&id); err != nil || p != nil {
-			return err == nil
+			return err == nil, err
 		}
 		if exists(r.loosePath(&id)) {
-			return true
+			return true, nil
 		}
-		if !f.again() {
+		again, err := f.again()
-			return false
+		if err != nil {
+			return false, err
+		}
+		if !again {
+			return false, nil
 		}
 	}
 }
 		if !errors.Is(err, ErrNotFound) {
 			return 0, 0, err
 		}
-		if !f.again() {
+		if again, err := f.again(); err != nil {
+			return 0, 0, err
+		} else if !again {
 			return 0, 0, notFound("object %s", id.String())
 		}
 	}
 		if !errors.Is(err, ErrNotFound) {
 			return 0, nil, err
 		}
-		if !f.again() {
+		if again, err := f.again(); err != nil {
+			return 0, nil, err
+		} else if !again {
 			return 0, nil, notFound("object %s", id.String())
 		}
 	}
 // that designate a commit: commits and tags that peel to a commit. It is what
 // "git rev-parse <hex>^{commit}" does.
 func (r *Repo) ResolveCommitPrefix(hex string) (ObjectID, error) {
-	return r.resolvePrefix(hex, func(id ObjectID) bool {
+	return r.resolvePrefix(hex, func(id ObjectID) (bool, error) {
 		_, err := r.PeelToCommit(id)
-		return err == nil
+		if errors.Is(err, ErrWrongType) {
+			return false, nil
+		}
+		return err == nil, err
 	})
 }
-func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) bool) (ObjectID, error) {
+func (r *Repo) resolvePrefix(hex string, accept func(ObjectID) (bool, error)) (ObjectID, error) {
 	n := len(hex)
 	if n < minPrefix || n > 40 {
 		return ObjectID{}, fmt.Errorf("%w: %q must have %d to 40 hexadecimal digits", ErrInvalidID, truncate(hex), minPrefix)
 	for {
 		var found [2]ObjectID
 		count := 0
+		var acceptErr error
 		add := func(id ObjectID) bool {
 			if count > 0 && found[0] == id {
 				return true
 			if count > 1 && found[1] == id {
 				return true
 			}
-			if accept != nil && !accept(id) {
+			if accept != nil {
-				return true
+				ok, err := accept(id)
+				if err != nil {
+					acceptErr = err
+					return false
+				}
+				if !ok {
+					return true
+				}
 			}
 			found[count] = id
 			count++
 				break
 			}
 		}
-		if count < 2 {
+		if count < 2 && acceptErr == nil {
 			if err := r.looseWithPrefix(hex, add); err != nil {
 				return ObjectID{}, err
 			}
 		}
+		if acceptErr != nil {
+			return ObjectID{}, acceptErr
+		}
 		switch {
 		case count >= 2:
 			return ObjectID{}, fmt.Errorf("%w: %s matches several objects", ErrAmbiguous, hex)
 		case count == 1:
 			return found[0], nil
 		}
-		if !f.again() {
+		if again, err := f.again(); err != nil {
+			return ObjectID{}, err
+		} else if !again {
 			return ObjectID{}, notFound("no object starts with %s", hex)
 		}
 	}

M refs.go +5 -1


@@ -511,7 +511,11 @@ func (r *Repo) Resolve(rev string) (ObjectID, error) {
511	511		}
512	512		if len(rev) == 40 {
513	513		if id, err := ParseID(rev); err == nil {
514		-	if !r.HasObject(id) {
	514	+	ok, err := r.HasObject(id)
	515	+	if err != nil {
	516	+	return ObjectID{}, err
	517	+	}
	518	+	if !ok {
515	519		return ObjectID{}, notFound("object %s", id)
516	520		}
517	521		return id, nil

 	}
 	if len(rev) == 40 {
 		if id, err := ParseID(rev); err == nil {
-			if !r.HasObject(id) {
+			ok, err := r.HasObject(id)
+			if err != nil {
+				return ObjectID{}, err
+			}
+			if !ok {
 				return ObjectID{}, notFound("object %s", id)
 			}
 			return id, nil

A reload_test.go +78 -0

@@ -0,0 +1,78 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"strings"
	8	+	"testing"
	9	+	)
	10	+
	11	+	func TestReloadErrorIsNotNotFound(t *testing.T) {
	12	+	f := newFx(t)
	13	+	f.write("a", []byte("a"), 0o644)
	14	+	c := mustID(t, f.commit("one"))
	15	+	r := f.open()
	16	+	boom := errors.New("pack directory is being rewritten")
	17	+	reloadPacksHook = func(*Repo) (bool, error) { return false, boom }
	18	+	t.Cleanup(func() { reloadPacksHook = nil })
	19	+
	20	+	missing := mustID(t, strings.Repeat("ab", 20))
	21	+	check := func(name string, err error) {
	22	+	t.Helper()
	23	+	if !errors.Is(err, boom) \|\| errors.Is(err, ErrNotFound) {
	24	+	t.Errorf("%s = %v, want the reload error", name, err)
	25	+	}
	26	+	}
	27	+	_, _, err := r.ReadObject(missing, nil)
	28	+	check("ReadObject", err)
	29	+	_, _, err = r.ObjectHeader(missing)
	30	+	check("ObjectHeader", err)
	31	+	_, _, err = r.OpenBlob(missing)
	32	+	check("OpenBlob", err)
	33	+	_, err = r.ResolvePrefix("abab")
	34	+	check("ResolvePrefix", err)
	35	+	_, err = r.ResolveCommitPrefix("abab")
	36	+	check("ResolveCommitPrefix", err)
	37	+	ok, err := r.HasObject(missing)
	38	+	check("HasObject", err)
	39	+	if ok {
	40	+	t.Error("HasObject reports an object that does not exist")
	41	+	}
	42	+	_, err = r.Resolve(missing.String())
	43	+	check("Resolve", err)
	44	+	// Objects that are found never need the reload.
	45	+	if ok, err := r.HasObject(c); !ok \|\| err != nil {
	46	+	t.Errorf("HasObject(existing) = %v, %v", ok, err)
	47	+	}
	48	+	// Without the hook a missing object is a plain ErrNotFound.
	49	+	reloadPacksHook = nil
	50	+	if _, _, err := r.ReadObject(missing, nil); !errors.Is(err, ErrNotFound) {
	51	+	t.Errorf("ReadObject(missing) = %v", err)
	52	+	}
	53	+	if ok, err := r.HasObject(missing); ok \|\| err != nil {
	54	+	t.Errorf("HasObject(missing) = %v, %v", ok, err)
	55	+	}
	56	+	}
	57	+
	58	+	func TestResolveCommitPrefixReturnsErrors(t *testing.T) {
	59	+	f := newFx(t)
	60	+	f.write("a", []byte("a"), 0o644)
	61	+	f.commit("one")
	62	+	// A tag whose content cannot be parsed: skipping it would turn a broken
	63	+	// repository into "not found".
	64	+	bad := mustID(t, f.gitIn([]byte("object zz\ntype commit\ntag t\ntagger T <t@x> 1 +0000\n\nmsg\n"), "hash-object", "-t", "tag", "--literally", "-w", "--stdin"))
	65	+	// A tag of a tree is merely not a commit.
	66	+	tree := f.git("rev-parse", "HEAD^{tree}")
	67	+	ofTree := mustID(t, f.gitIn([]byte("object "+tree+"\ntype tree\ntag t\ntagger T <t@x> 1 +0000\n\nmsg\n"), "hash-object", "-t", "tag", "-w", "--stdin"))
	68	+	r := f.open()
	69	+	if _, err := r.ResolveCommitPrefix(bad.String()[:10]); !errors.Is(err, ErrCorrupt) {
	70	+	t.Errorf("broken tag: %v, want ErrCorrupt", err)
	71	+	}
	72	+	if _, err := r.ResolveCommitPrefix(ofTree.String()[:10]); !errors.Is(err, ErrNotFound) {
	73	+	t.Errorf("tag of a tree: %v, want ErrNotFound", err)
	74	+	}
	75	+	if got, err := r.ResolvePrefix(bad.String()[:10]); err != nil \|\| got != bad {
	76	+	t.Errorf("ResolvePrefix = %s, %v", got, err)
	77	+	}
	78	+	}

@@ -0,0 +1,78 @@
	1	+	// SPDX-License-Identifier: MIT
	2	+
	3	+	package git
	4	+
	5	+	import (
	6	+	"errors"
	7	+	"strings"
	8	+	"testing"
	9	+	)
	10	+
	11	+	func TestReloadErrorIsNotNotFound(t *testing.T) {
	12	+	f := newFx(t)
	13	+	f.write("a", []byte("a"), 0o644)
	14	+	c := mustID(t, f.commit("one"))
	15	+	r := f.open()
	16	+	boom := errors.New("pack directory is being rewritten")
	17	+	reloadPacksHook = func(*Repo) (bool, error) { return false, boom }
	18	+	t.Cleanup(func() { reloadPacksHook = nil })
	19	+
	20	+	missing := mustID(t, strings.Repeat("ab", 20))
	21	+	check := func(name string, err error) {
	22	+	t.Helper()
	23	+	if !errors.Is(err, boom) \|\| errors.Is(err, ErrNotFound) {
	24	+	t.Errorf("%s = %v, want the reload error", name, err)
	25	+	}
	26	+	}
	27	+	_, _, err := r.ReadObject(missing, nil)
	28	+	check("ReadObject", err)
	29	+	_, _, err = r.ObjectHeader(missing)
	30	+	check("ObjectHeader", err)
	31	+	_, _, err = r.OpenBlob(missing)
	32	+	check("OpenBlob", err)
	33	+	_, err = r.ResolvePrefix("abab")
	34	+	check("ResolvePrefix", err)
	35	+	_, err = r.ResolveCommitPrefix("abab")
	36	+	check("ResolveCommitPrefix", err)
	37	+	ok, err := r.HasObject(missing)
	38	+	check("HasObject", err)
	39	+	if ok {
	40	+	t.Error("HasObject reports an object that does not exist")
	41	+	}
	42	+	_, err = r.Resolve(missing.String())
	43	+	check("Resolve", err)
	44	+	// Objects that are found never need the reload.
	45	+	if ok, err := r.HasObject(c); !ok \|\| err != nil {
	46	+	t.Errorf("HasObject(existing) = %v, %v", ok, err)
	47	+	}
	48	+	// Without the hook a missing object is a plain ErrNotFound.
	49	+	reloadPacksHook = nil
	50	+	if _, _, err := r.ReadObject(missing, nil); !errors.Is(err, ErrNotFound) {
	51	+	t.Errorf("ReadObject(missing) = %v", err)
	52	+	}
	53	+	if ok, err := r.HasObject(missing); ok \|\| err != nil {
	54	+	t.Errorf("HasObject(missing) = %v, %v", ok, err)
	55	+	}
	56	+	}
	57	+
	58	+	func TestResolveCommitPrefixReturnsErrors(t *testing.T) {
	59	+	f := newFx(t)
	60	+	f.write("a", []byte("a"), 0o644)
	61	+	f.commit("one")
	62	+	// A tag whose content cannot be parsed: skipping it would turn a broken
	63	+	// repository into "not found".
	64	+	bad := mustID(t, f.gitIn([]byte("object zz\ntype commit\ntag t\ntagger T <t@x> 1 +0000\n\nmsg\n"), "hash-object", "-t", "tag", "--literally", "-w", "--stdin"))
	65	+	// A tag of a tree is merely not a commit.
	66	+	tree := f.git("rev-parse", "HEAD^{tree}")
	67	+	ofTree := mustID(t, f.gitIn([]byte("object "+tree+"\ntype tree\ntag t\ntagger T <t@x> 1 +0000\n\nmsg\n"), "hash-object", "-t", "tag", "-w", "--stdin"))
	68	+	r := f.open()
	69	+	if _, err := r.ResolveCommitPrefix(bad.String()[:10]); !errors.Is(err, ErrCorrupt) {
	70	+	t.Errorf("broken tag: %v, want ErrCorrupt", err)
	71	+	}
	72	+	if _, err := r.ResolveCommitPrefix(ofTree.String()[:10]); !errors.Is(err, ErrNotFound) {
	73	+	t.Errorf("tag of a tree: %v, want ErrNotFound", err)
	74	+	}
	75	+	if got, err := r.ResolvePrefix(bad.String()[:10]); err != nil \|\| got != bad {
	76	+	t.Errorf("ResolvePrefix = %s, %v", got, err)
	77	+	}
	78	+	}