From 2c8903fd8ef6f2e3843f9373be268cf9136ba9e2 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Thu, 13 Aug 2026 09:33:00 +0300 Subject: [PATCH] browse: report an unparseable start hash as a missing ref --- browse/log.go | 20 ++++++++++-- browse/log_fromhash_test.go | 61 +++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 browse/log_fromhash_test.go diff --git a/browse/log.go b/browse/log.go index 4ba16a056f6e7d1706b81dbda972b1cf347e4d5b..93ac43ec53bea620d6e21b51c1fd078d03006bf0 100644 --- a/browse/log.go +++ b/browse/log.go @@ -82,6 +82,11 @@ func DefaultBranch(branches []Branch) string { // first are fetched: pass the nextHash returned by the previous call. nextHash // is the hash of the first commit of the following page, or "" when the last // page was returned. +// +// A caller may rely on errors.Is(err, ErrRefNotFound) to hold whenever refStr +// or fromHash names nothing this store can resolve — including a fromHash +// that does not even parse as a hash. Any other error means the store could +// not answer, not that the answer is "not found". func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]CommitInfo, string, error) { if limit <= 0 { return nil, "", fmt.Errorf("browse: log limit must be positive, got %d", limit) @@ -91,7 +96,7 @@ func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]Co if fromHash != "" { h, ok := hash.MaybeParse(fromHash) if !ok { - return nil, "", fmt.Errorf("browse: invalid from hash %q", fromHash) + return nil, "", fmt.Errorf("%w: invalid from hash %q", ErrRefNotFound, fromHash) } start = h } else { @@ -106,6 +111,11 @@ func (db *DB) Log(ctx context.Context, refStr, fromHash string, limit int) ([]Co } itr, err := commitwalk.GetTopologicalOrderIterator[context.Context](ctx, db.ddb, []hash.Hash{start}, nil) + if errors.Is(err, datas.ErrCommitNotFound) { + // A well-formed hash (parsed above, or the head of a resolved ref) + // that names no commit in this store is a miss, not a store failure. + return nil, "", fmt.Errorf("%w: %s", ErrRefNotFound, start.String()) + } if err != nil { return nil, "", fmt.Errorf("browse: topological iterator: %w", err) } @@ -173,7 +183,10 @@ func commitInfo(ctx context.Context, h hash.Hash, oc *doltdb.OptionalCommit, met } // resolveCommit resolves a ref string to a commit. It is tried first as a -// branch name, then as a commit hash. +// branch name, then as a commit hash. Every way refStr can fail to name a +// commit — not a hash, a hash with no matching commit, a hash that resolves +// only to a ghost — is reported via ErrRefNotFound; other errors mean the +// store itself could not answer. func (db *DB) resolveCommit(ctx context.Context, refStr string) (*doltdb.Commit, error) { if _, ok, err := db.ddb.HasBranch(ctx, refStr); err != nil { return nil, fmt.Errorf("browse: check branch %q: %w", refStr, err) @@ -187,6 +200,9 @@ func (db *DB) resolveCommit(ctx context.Context, refStr string) (*doltdb.Commit, if h, ok := hash.MaybeParse(refStr); ok { oc, err := db.ddb.ResolveHash(ctx, h) + if errors.Is(err, datas.ErrCommitNotFound) { + return nil, fmt.Errorf("%w: %s", ErrRefNotFound, refStr) + } if err != nil { return nil, fmt.Errorf("browse: resolve hash %q: %w", refStr, err) } diff --git a/browse/log_fromhash_test.go b/browse/log_fromhash_test.go new file mode 100644 index 0000000000000000000000000000000000000000..3552423403fc108b8106f32209f9bac090d08c9a --- /dev/null +++ b/browse/log_fromhash_test.go @@ -0,0 +1,61 @@ +package browse + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestLogFromHashGarbage covers the MCP get_commit_log case: a caller-supplied +// cursor that does not even parse as a dolt hash must be an ordinary "not +// found" (errors.Is(err, ErrRefNotFound)), not an opaque internal error. +func TestLogFromHashGarbage(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + _, _, err := db.Log(ctx, "main", "not-a-hash", 10) + require.Error(t, err) + assert.ErrorIs(t, err, ErrRefNotFound) +} + +// TestLogFromHashWellFormedButUnknown covers a fromHash that parses fine but +// names no commit this store has: same classification as a garbage string. +func TestLogFromHashWellFormedButUnknown(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + // 32 'a' characters is a syntactically valid (but non-existent) dolt hash. + _, _, err := db.Log(ctx, "main", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", 10) + require.Error(t, err) + assert.ErrorIs(t, err, ErrRefNotFound) +} + +// TestLogFromHashValid confirms the happy path is untouched: a real hash +// still starts the walk there and pages as before. +func TestLogFromHashValid(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + commits, next, err := db.Log(ctx, "main", commitHash(t, msgInsert), 100) + require.NoError(t, err) + assert.Empty(t, next) + + want := []string{msgInsert, msgAddUser, msgInitial} + require.Len(t, commits, len(want)) + for i, w := range want { + assert.Equal(t, w, commits[i].Message) + } +} + +// TestLogFromHashEmpty confirms "" still means "start from ref's head". +func TestLogFromHashEmpty(t *testing.T) { + db := openFixture(t) + ctx := context.Background() + + commits, _, err := db.Log(ctx, "main", "", 100) + require.NoError(t, err) + require.NotEmpty(t, commits) + assert.Equal(t, msgAddItem, commits[0].Message) +}