Skip to content

Name the conflicting ref in ExistingRefError - #11437

Open
asadbek0512 wants to merge 1 commit into
dolthub:mainfrom
asadbek0512:fix/existing-ref-error-names-ref
Open

Name the conflicting ref in ExistingRefError#11437
asadbek0512 wants to merge 1 commit into
dolthub:mainfrom
asadbek0512:fix/existing-ref-error-names-ref

Conversation

@asadbek0512

Copy link
Copy Markdown

Fixes #11434

ExistingRefError.Error() returned a bare already exists. A user seeing it learns neither which ref was taken nor what kind it was. The TODO(elianddb) on it said the ref could only be named once tags and workspaces used their own errors, since they surfaced this message verbatim.

Changes

  • actions.ErrTagExists / actions.ErrWorkspaceExists, mirroring the existing ErrBranchExists wording.
  • CreateTagOnDB and CreateWorkspaceOnDB return those instead of &doltdb.ExistingRefError{...}.
  • ExistingRefError.Error() now returns ref 'refs/heads/br' already exists; the TODO is gone.

Why tags and workspaces return their error at the source

BranchExistsError exists because failOnCaseConflict raises ExistingRefError deep inside branch creation, where the ref that actually collides differs from the one the caller asked for; the caller has to unwrap to learn the real name. Tag and workspace creation instead detect the duplicate with an explicit ddb.HasRef check that already has the name in scope, and neither path runs failOnCaseConflict. An unwrapper for them would only re-derive a name the raising function held, so they return the typed error directly. CreateWorkspace currently has no callers, which also makes an unwrapper dead code.

That leaves ExistingRefError raised only on branch paths, whose callers already wrap it. Its own message is the fallback for a caller that does not, so it uses Ref.String() rather than GetPath(): a name can exist as more than one ref type, and an unwrapped error should not be ambiguous about which one it means.

Compatibility

No test asserted the bare string. branch_case_test.go matches on the error type and Ref.GetPath(), both unchanged. No enginetest or bats case asserts a tag- or workspace-exists message; the bats suites that grep for already exists cover tables, foreign keys, and files, and the new messages still contain that substring.

Testing

go test passes for ./libraries/doltcore/doltdb/..., ./libraries/doltcore/env/..., and -run 'TestDoltTag|TestDoltBranch|TestDoltCheckout' in ./libraries/doltcore/sqle/enginetest/. go vet is clean on the touched packages. Added TestExistingRefErrorNamesTheRef to pin the message across branch, tag, and workspace refs.

ExistingRefError.Error() returned a bare "already exists", which tells a
user neither which ref was taken nor what kind it was. The message stayed
minimal because tag and workspace creation surfaced this error verbatim,
so any detail added here would have been wrong for them.

Give tags and workspaces their own errors, mirroring ErrBranchExists, and
name the ref in ExistingRefError. Both detect the duplicate with an
explicit HasRef check that has the name in hand, so they return their
error at the source rather than through a BranchExistsError-style
unwrapper; branches need that unwrapper only because failOnCaseConflict
raises the error deep in the creation path, where the colliding name
differs from the requested one.

ExistingRefError is now raised only for branches, whose callers already
wrap it. Its own message is the fallback for a caller that does not, and
it carries the full ref path so a name held by more than one ref type
stays unambiguous.

Fixes dolthub#11434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExistingRefError.Error() omits the ref name and type

2 participants