Name the conflicting ref in ExistingRefError - #11437
Open
asadbek0512 wants to merge 1 commit into
Open
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11434
ExistingRefError.Error()returned a barealready exists. A user seeing it learns neither which ref was taken nor what kind it was. TheTODO(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 existingErrBranchExistswording.CreateTagOnDBandCreateWorkspaceOnDBreturn those instead of&doltdb.ExistingRefError{...}.ExistingRefError.Error()now returnsref 'refs/heads/br' already exists; theTODOis gone.Why tags and workspaces return their error at the source
BranchExistsErrorexists becausefailOnCaseConflictraisesExistingRefErrordeep 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 explicitddb.HasRefcheck that already has the name in scope, and neither path runsfailOnCaseConflict. An unwrapper for them would only re-derive a name the raising function held, so they return the typed error directly.CreateWorkspacecurrently has no callers, which also makes an unwrapper dead code.That leaves
ExistingRefErrorraised only on branch paths, whose callers already wrap it. Its own message is the fallback for a caller that does not, so it usesRef.String()rather thanGetPath(): 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.gomatches on the error type andRef.GetPath(), both unchanged. No enginetest or bats case asserts a tag- or workspace-exists message; the bats suites that grep foralready existscover tables, foreign keys, and files, and the new messages still contain that substring.Testing
go testpasses for./libraries/doltcore/doltdb/...,./libraries/doltcore/env/..., and-run 'TestDoltTag|TestDoltBranch|TestDoltCheckout'in./libraries/doltcore/sqle/enginetest/.go vetis clean on the touched packages. AddedTestExistingRefErrorNamesTheRefto pin the message across branch, tag, and workspace refs.