Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Error Handling for Tags #8382

Merged
merged 10 commits into from
Sep 25, 2024
Merged

Improve Error Handling for Tags #8382

merged 10 commits into from
Sep 25, 2024

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Sep 24, 2024

Changes:

  • Prevent cloning from a tag through --branch option
  • Return detached head error for checking out branch through cli

Fixes: #8377

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
8ed8710 ok 5937457
version total_tests
8ed8710 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
cde716f ok 5937457
version total_tests
cde716f 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
685c7a5 ok 5937457
version total_tests
685c7a5 5937457
correctness_percentage
100.0

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

return "", err
} else if isTag && apr.Contains(cli.MoveFlag) {
return "", fmt.Errorf(`dolt does not support a detached head state. To checkout a tag, run:
dolt checkout %s -b {new_branch_namee}`, branchName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dolt checkout %s -b {new_branch_namee}`, branchName)
dolt checkout %s -b {new_branch_name}`, branchName)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice error message btw! Really, really helpful to tell users exactly what command they need to run whenever we can.

@@ -290,11 +290,15 @@ func checkoutRemoteBranch(ctx *sql.Context, dSess *dsess.DoltSession, dbName str
}

if len(remoteRefs) == 0 {
if isTag, err := actions.IsTag(ctx, dbData.Ddb, branchName); err != nil {
return "", err
} else if isTag && apr.Contains(cli.MoveFlag) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to only execute this logic to when cli.MoveFlag is present? My understanding is that we don't document the MoveFlag, but the CLI uses it when it calls dolt_checkout() to move the working set contents? It seems like we'd want to return the same error message if you tried to check out a tag from the SQL interface, too, right? I may just be missing something on how the MoveFlag is used though.

},
{
Query: "call dolt_checkout('v1');",
ExpectedErrStr: "error: could not find v1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) Might be nice if there was a more prescriptive error message here, like we use in dolt checkout to tell the user exactly what they need to run in order to checkout a tag as a new branch.

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
1b36adf ok 5937457
version total_tests
1b36adf 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
d309e8d ok 5937457
version total_tests
d309e8d 5937457
correctness_percentage
100.0

@jycor jycor merged commit 8f8f08b into main Sep 25, 2024
21 checks passed
@jycor jycor deleted the james/tag branch September 25, 2024 20:38
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.

Dolt clone on tag name results in multiple issues
3 participants