From 78d7d1492d903e3380628f0cb1e52d3b36732143 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV Date: Fri, 10 Jan 2025 11:12:11 -0800 Subject: [PATCH 1/4] Make show command more resilient when resolving references Currently the show command can print internal object, which requires a local environment. This goes against the sql migration expectations that there is no environment. This change only makes the situation less bad. Splitting out the admin operations into another command is the right approach. --- go/cmd/dolt/commands/show.go | 60 +++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/go/cmd/dolt/commands/show.go b/go/cmd/dolt/commands/show.go index 00e66d093e..b82e5a938b 100644 --- a/go/cmd/dolt/commands/show.go +++ b/go/cmd/dolt/commands/show.go @@ -127,6 +127,25 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d defer closeFunc() } + resolvedRefs := make([]string, 0, len(opts.specRefs)) + for _, specRef := range opts.specRefs { + if !hashRegex.MatchString(specRef) && !strings.EqualFold(specRef, "HEAD") { + // Call "dolt_hashof" to resolve the ref to a hash. the --no-pretty flag gets around the commit requirement, but + // requires the full object name so it will match the hashRegex and never his this code block. + h, err2 := getHashOf(queryist, sqlCtx, specRef) + if err2 != nil { + cli.PrintErrln("error: failed to resolve ref to commit: %s ", specRef) + return 1 + } + resolvedRefs = append(resolvedRefs, h) + } else { + resolvedRefs = append(resolvedRefs, specRef) + } + } + if len(resolvedRefs) == 0 { + resolvedRefs = []string{"HEAD"} + } + // There are two response formats: // - "pretty", which shows commits in a human-readable fashion // - "raw", which shows the underlying SerialMessage @@ -134,19 +153,14 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d // The pretty format is preferred unless the --no-pretty flag is provided. // But only commits are supported in the "pretty" format. // Thus, if the --no-pretty flag is not set, then we require that either all the references are commits, or none of them are. - - isDEnvRequired := false - if !opts.pretty { - isDEnvRequired = true - } - for _, specRef := range opts.specRefs { - if !hashRegex.MatchString(specRef) && !strings.EqualFold(specRef, "HEAD") { - isDEnvRequired = true + if dEnv == nil { + // Logic below requires a non-nil dEnv when --no-pretty is set. + // TODO: remove all usage of dEnv entirely from this command. + cli.PrintErrln("`\\show` is not fully supported in the SQL shell.") + return 1 } - } - if isDEnvRequired { // use dEnv instead of the SQL engine _, ok := queryist.(*engine.SqlEngine) if !ok { @@ -160,12 +174,7 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d } } - specRefs := opts.specRefs - if len(specRefs) == 0 { - specRefs = []string{"HEAD"} - } - - for _, specRef := range specRefs { + for _, specRef := range resolvedRefs { // If --no-pretty was supplied, always display the raw contents of the referenced object. if !opts.pretty { err := printRawValue(ctx, dEnv, specRef) @@ -176,8 +185,9 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d } // If the argument is a commit, display it in the "pretty" format. - // But if it's a hash, we don't know whether it's a commit until we query the engine. - commitInfo, err := getCommitSpecPretty(queryist, sqlCtx, opts, specRef) + // But if it's a hash, we don't know whether it's a commit until we query the engine. If a non-commit was specified + // by the user, we are forced to display the raw contents of the object - which required a dEnv + commitInfo, err := getCommitSpecPretty(queryist, sqlCtx, specRef) if commitInfo == nil { // Hash is not a commit _, ok := queryist.(*engine.SqlEngine) @@ -185,11 +195,15 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d cli.PrintErrln("`dolt show (NON_COMMIT_HASH)` only supported in local mode.") return 1 } - - if !opts.pretty && !dEnv.DoltDB.Format().UsesFlatbuffers() { + if dEnv == nil { + cli.PrintErrln("`dolt show (NON_COMMIT_HASH)` requires a local environment. Not intended to common use.") + return 1 + } + if !dEnv.DoltDB.Format().UsesFlatbuffers() { cli.PrintErrln("`dolt show (NON_COMMIT_HASH)` is not supported when using old LD_1 storage format.") return 1 } + value, err := getValueFromRefSpec(ctx, dEnv, specRef) if err != nil { err = fmt.Errorf("error resolving spec ref '%s': %w", specRef, err) @@ -211,6 +225,8 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d return 0 } +// printRawValue prints the raw value of the object referenced by specRef. This function works directly on storage, and +// requires a non-nil dEnv. func printRawValue(ctx context.Context, dEnv *env.DoltEnv, specRef string) error { value, err := getValueFromRefSpec(ctx, dEnv, specRef) if err != nil { @@ -220,6 +236,8 @@ func printRawValue(ctx context.Context, dEnv *env.DoltEnv, specRef string) error return nil } +// getValueFromRefSpec returns the value of the object referenced by specRef. This +// function works directly on storage, and requires a non-nil dEnv. func getValueFromRefSpec(ctx context.Context, dEnv *env.DoltEnv, specRef string) (types.Value, error) { var refHash hash.Hash var err error @@ -303,7 +321,7 @@ func parseHashString(hashStr string) (hash.Hash, error) { return parsedHash, nil } -func getCommitSpecPretty(queryist cli.Queryist, sqlCtx *sql.Context, opts *showOpts, commitRef string) (commit *CommitInfo, err error) { +func getCommitSpecPretty(queryist cli.Queryist, sqlCtx *sql.Context, commitRef string) (commit *CommitInfo, err error) { if strings.HasPrefix(commitRef, "#") { commitRef = strings.TrimPrefix(commitRef, "#") } From d4b272262691628da9735c0e3b31197b01296d21 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV <46170177+macneale4@users.noreply.github.com> Date: Fri, 10 Jan 2025 11:17:49 -0800 Subject: [PATCH 2/4] Update go/cmd/dolt/commands/show.go --- go/cmd/dolt/commands/show.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/cmd/dolt/commands/show.go b/go/cmd/dolt/commands/show.go index b82e5a938b..9058c392e5 100644 --- a/go/cmd/dolt/commands/show.go +++ b/go/cmd/dolt/commands/show.go @@ -131,7 +131,7 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d for _, specRef := range opts.specRefs { if !hashRegex.MatchString(specRef) && !strings.EqualFold(specRef, "HEAD") { // Call "dolt_hashof" to resolve the ref to a hash. the --no-pretty flag gets around the commit requirement, but - // requires the full object name so it will match the hashRegex and never his this code block. + // requires the full object name so it will match the hashRegex and never hit this code block. h, err2 := getHashOf(queryist, sqlCtx, specRef) if err2 != nil { cli.PrintErrln("error: failed to resolve ref to commit: %s ", specRef) From c6cd6b441b9a0b361f1bf3c5a5ce36ef0b662317 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV Date: Fri, 10 Jan 2025 14:13:08 -0800 Subject: [PATCH 3/4] Handle WORKING and STAGED as --no-pretty options --- go/cmd/dolt/commands/show.go | 7 +++++-- integration-tests/bats/show.bats | 9 ++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/go/cmd/dolt/commands/show.go b/go/cmd/dolt/commands/show.go index 9058c392e5..fc4a90bdac 100644 --- a/go/cmd/dolt/commands/show.go +++ b/go/cmd/dolt/commands/show.go @@ -129,12 +129,15 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d resolvedRefs := make([]string, 0, len(opts.specRefs)) for _, specRef := range opts.specRefs { - if !hashRegex.MatchString(specRef) && !strings.EqualFold(specRef, "HEAD") { + if !hashRegex.MatchString(specRef) && + !strings.EqualFold(specRef, "HEAD") && + !strings.EqualFold(specRef, "WORKING") && + !strings.EqualFold(specRef, "STAGED") { // Call "dolt_hashof" to resolve the ref to a hash. the --no-pretty flag gets around the commit requirement, but // requires the full object name so it will match the hashRegex and never hit this code block. h, err2 := getHashOf(queryist, sqlCtx, specRef) if err2 != nil { - cli.PrintErrln("error: failed to resolve ref to commit: %s ", specRef) + cli.PrintErrln(fmt.Sprintf("branch not found: %s", specRef)) return 1 } resolvedRefs = append(resolvedRefs, h) diff --git a/integration-tests/bats/show.bats b/integration-tests/bats/show.bats index c1c57cac22..5f762e17ed 100644 --- a/integration-tests/bats/show.bats +++ b/integration-tests/bats/show.bats @@ -191,7 +191,7 @@ assert_has_key_value() { dolt add . dolt sql -q "create table table3 (pk int PRIMARY KEY)" dolt sql -q "insert into table1 values (7), (8), (9)" - run dolt show WORKING + run dolt show --no-pretty WORKING [ $status -eq 0 ] [[ "$output" =~ "table1" ]] || false [[ "$output" =~ "table2" ]] || false @@ -208,7 +208,7 @@ assert_has_key_value() { dolt add . dolt sql -q "create table table3 (pk int PRIMARY KEY)" dolt sql -q "insert into table1 values (7), (8), (9)" - run dolt show STAGED + run dolt show --no-pretty STAGED [ $status -eq 0 ] [[ "$output" =~ "table1" ]] || false [[ "$output" =~ "table2" ]] || false @@ -225,16 +225,15 @@ assert_has_key_value() { dolt add . dolt sql -q "create table table3 (pk int PRIMARY KEY)" dolt sql -q "insert into table1 values (7), (8), (9)" - workingRoot=$(dolt show WORKING) + workingRoot=$(dolt show --no-pretty WORKING) tableAddress=$(extract_value table1 "$workingRoot") - run dolt show $tableAddress + run dolt show --no-pretty $tableAddress assert_has_key Schema "$output" assert_has_key Violations "$output" assert_has_key Autoinc "$output" assert_has_key "Primary index" "$output" assert_has_key "Secondary indexes" "$output" - } @test "show: pretty commit from hash" { From 6016c4aac139ab1481a2af7e06e1a875bed34f48 Mon Sep 17 00:00:00 2001 From: Neil Macneale IV Date: Sun, 12 Jan 2025 16:36:40 -0800 Subject: [PATCH 4/4] PR Feedback --- go/cmd/dolt/commands/show.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/cmd/dolt/commands/show.go b/go/cmd/dolt/commands/show.go index fc4a90bdac..2e203aec45 100644 --- a/go/cmd/dolt/commands/show.go +++ b/go/cmd/dolt/commands/show.go @@ -160,7 +160,7 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d if dEnv == nil { // Logic below requires a non-nil dEnv when --no-pretty is set. // TODO: remove all usage of dEnv entirely from this command. - cli.PrintErrln("`\\show` is not fully supported in the SQL shell.") + cli.PrintErrln("`\\show --no-pretty` is not supported in the SQL shell.") return 1 } @@ -199,7 +199,7 @@ func (cmd ShowCmd) Exec(ctx context.Context, commandStr string, args []string, d return 1 } if dEnv == nil { - cli.PrintErrln("`dolt show (NON_COMMIT_HASH)` requires a local environment. Not intended to common use.") + cli.PrintErrln("`dolt show (NON_COMMIT_HASH)` requires a local environment. Not intended for common use.") return 1 } if !dEnv.DoltDB.Format().UsesFlatbuffers() {