diff --git a/go/cmd/dolt/commands/cherry-pick.go b/go/cmd/dolt/commands/cherry-pick.go index 796ab29759e..4dec0666d46 100644 --- a/go/cmd/dolt/commands/cherry-pick.go +++ b/go/cmd/dolt/commands/cherry-pick.go @@ -16,17 +16,21 @@ package commands import ( "context" + "fmt" + "strings" + "github.com/dolthub/go-mysql-server/sql" + "github.com/gocraft/dbr/v2" + "github.com/gocraft/dbr/v2/dialect" "gopkg.in/src-d/go-errors.v1" "github.com/dolthub/dolt/go/cmd/dolt/cli" "github.com/dolthub/dolt/go/cmd/dolt/errhand" eventsapi "github.com/dolthub/dolt/go/gen/proto/dolt/services/eventsapi/v1alpha1" - "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/branch_control" "github.com/dolthub/dolt/go/libraries/doltcore/env" - "github.com/dolthub/dolt/go/libraries/doltcore/merge" - "github.com/dolthub/dolt/go/libraries/doltcore/table/editor" "github.com/dolthub/dolt/go/libraries/utils/argparser" + "github.com/dolthub/dolt/go/store/util/outputpager" ) var cherryPickDocs = cli.CommandDocumentationContent{ @@ -80,25 +84,25 @@ func (cmd CherryPickCmd) Exec(ctx context.Context, commandStr string, args []str ap := cli.CreateCherryPickArgParser() help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, cherryPickDocs, ap)) apr := cli.ParseArgsOrDie(ap, args, help) - if dEnv.IsLocked() { - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(env.ErrActiveServerLock.New(dEnv.LockFile())), help) + + queryist, sqlCtx, closeFunc, err := cliCtx.QueryEngine(ctx) + if err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + if closeFunc != nil { + defer closeFunc() } - // This command creates a commit, so we need user identity - if !cli.CheckUserNameAndEmail(cliCtx.Config()) { + + // dolt_cherry_pick performs this check as well. Check performed early here to short circuit the operation. + err = branch_control.CheckAccess(sqlCtx, branch_control.Permissions_Write) + if err != nil { + cli.Println(err.Error()) return 1 } if apr.Contains(cli.AbortParam) { - ws, err := dEnv.WorkingSet(ctx) - if err != nil { - return HandleVErrAndExitCode(errhand.BuildDError("fatal: unable to load working set: %v", err).Build(), nil) - } - - if !ws.MergeActive() { - return HandleVErrAndExitCode(errhand.BuildDError("error: There is no cherry-pick merge to abort").Build(), nil) - } - - return HandleVErrAndExitCode(abortMerge(ctx, dEnv), usage) + err = cherryPickAbort(queryist, sqlCtx) + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) } // TODO : support single commit cherry-pick only for now @@ -109,173 +113,124 @@ func (cmd CherryPickCmd) Exec(ctx context.Context, commandStr string, args []str return HandleVErrAndExitCode(errhand.BuildDError("cherry-picking multiple commits is not supported yet").SetPrintUsage().Build(), usage) } + err = cherryPick(queryist, sqlCtx, apr) + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) +} + +func cherryPick(queryist cli.Queryist, sqlCtx *sql.Context, apr *argparser.ArgParseResults) error { cherryStr := apr.Arg(0) if len(cherryStr) == 0 { - verr := errhand.BuildDError("error: cannot cherry-pick empty string").Build() - return HandleVErrAndExitCode(verr, usage) + return fmt.Errorf("error: cannot cherry-pick empty string") } - verr := cherryPick(ctx, dEnv, cliCtx, cherryStr) - return HandleVErrAndExitCode(verr, usage) -} - -// cherryPick returns error if any step of cherry-picking fails. It receives cherry-picked commit and performs cherry-picking and commits. -func cherryPick(ctx context.Context, dEnv *env.DoltEnv, cliCtx cli.CliContext, cherryStr string) errhand.VerboseError { - // check for clean working state - headRoot, err := dEnv.HeadRoot(ctx) - if err != nil { - return errhand.VerboseErrorFromError(err) - } - workingRoot, err := dEnv.WorkingRoot(ctx) - if err != nil { - return errhand.VerboseErrorFromError(err) - } - stagedRoot, err := dEnv.StagedRoot(ctx) + hasStagedChanges, hasUnstagedChanges, err := hasStagedAndUnstagedChanged(queryist, sqlCtx) if err != nil { - return errhand.VerboseErrorFromError(err) + return fmt.Errorf("error: failed to check for staged and unstaged changes: %w", err) } - headHash, err := headRoot.HashOf() - if err != nil { - return errhand.VerboseErrorFromError(err) + if hasStagedChanges { + return fmt.Errorf("Please commit your staged changes before using cherry-pick.") } - workingHash, err := workingRoot.HashOf() - if err != nil { - return errhand.VerboseErrorFromError(err) - } - stagedHash, err := stagedRoot.HashOf() - if err != nil { - return errhand.VerboseErrorFromError(err) - } - - if !headHash.Equal(stagedHash) { - return errhand.BuildDError("Please commit your staged changes before using cherry-pick.").Build() - } - - if !headHash.Equal(workingHash) { - return errhand.BuildDError("error: your local changes would be overwritten by cherry-pick.\nhint: commit your changes (dolt commit -am \"\") or reset them (dolt reset --hard) to proceed.").Build() + if hasUnstagedChanges { + return fmt.Errorf(`error: your local changes would be overwritten by cherry-pick. +hint: commit your changes (dolt commit -am \"\") or reset them (dolt reset --hard) to proceed.`) } - mergeResult, commitMsg, err := mergeCherryPickedCommit(ctx, dEnv, workingRoot, cherryStr) + _, err = GetRowsForSql(queryist, sqlCtx, "set @@dolt_allow_commit_conflicts = 1") if err != nil { - return errhand.VerboseErrorFromError(err) + return fmt.Errorf("error: failed to set @@dolt_allow_commit_conflicts: %w", err) } - newWorkingRoot := mergeResult.Root - mapOfMergeStats := mergeResult.Stats - workingHash, err = newWorkingRoot.HashOf() + _, err = GetRowsForSql(queryist, sqlCtx, "set @@dolt_force_transaction_commit = 1") if err != nil { - return errhand.VerboseErrorFromError(err) + return fmt.Errorf("error: failed to set @@dolt_force_transaction_commit: %w", err) } - if headHash.Equal(workingHash) { - cli.Println("No changes were made.") - return nil - } - - err = dEnv.UpdateWorkingRoot(ctx, newWorkingRoot) + q, err := dbr.InterpolateForDialect("call dolt_cherry_pick(?)", []interface{}{cherryStr}, dialect.MySQL) if err != nil { - return errhand.VerboseErrorFromError(err) + return fmt.Errorf("error: failed to interpolate query: %w", err) } - - // Stage all tables that don't have merge conflicts or constraint violations - for tableName, mergeStats := range mapOfMergeStats { - if !mergeStats.HasArtifacts() { - res := AddCmd{}.Exec(ctx, "add", []string{tableName}, dEnv, cliCtx) - if res != 0 { - return errhand.BuildDError("dolt add failed").AddCause(err).Build() - } + rows, err := GetRowsForSql(queryist, sqlCtx, q) + if err != nil { + errorText := err.Error() + switch { + case strings.Contains("nothing to commit", errorText): + cli.Println("No changes were made.") + return nil + default: + return err } } - printSuccessStats(mapOfMergeStats) - - if mergeResult.HasMergeArtifacts() { - return errhand.VerboseErrorFromError(ErrCherryPickConflictsOrViolations.New()) - } else { - commitParams := []string{"-m", commitMsg} - res := CommitCmd{}.Exec(ctx, "commit", commitParams, dEnv, cliCtx) - if res != 0 { - return errhand.BuildDError("dolt commit failed").AddCause(err).Build() - } + if len(rows) != 1 { + return fmt.Errorf("error: unexpected number of rows returned from dolt_cherry_pick: %d", len(rows)) } - return nil -} + succeeded := false + commitHash := "" + for _, row := range rows { + commitHash = row[0].(string) + dataConflicts, err := getInt64ColAsInt64(row[1]) + if err != nil { + return fmt.Errorf("Unable to parse data_conflicts column: %w", err) + } + schemaConflicts, err := getInt64ColAsInt64(row[2]) + if err != nil { + return fmt.Errorf("Unable to parse schema_conflicts column: %w", err) + } + constraintViolations, err := getInt64ColAsInt64(row[3]) + if err != nil { + return fmt.Errorf("Unable to parse constraint_violations column: %w", err) + } -// mergeCherryPickedCommit executes a merge to cherry-pick the specified ref specification from -// |cherryStr| and apply it to the specified |workingRoot| in this |dEnv|. The MergeResult is -// returned, along with the commit message for the specified |cherryStr|, and any error encountered. -func mergeCherryPickedCommit(ctx context.Context, dEnv *env.DoltEnv, workingRoot *doltdb.RootValue, cherryStr string) (*merge.Result, string, error) { - tmpDir, err := dEnv.TempTableFilesDir() - if err != nil { - return nil, "", err + // if we have a hash and all 0s, then the cherry-pick succeeded + if len(commitHash) > 0 && dataConflicts == 0 && schemaConflicts == 0 && constraintViolations == 0 { + succeeded = true + } } - opts := editor.Options{Deaf: dEnv.BulkDbEaFactory(), Tempdir: tmpDir} - cherrySpec, err := doltdb.NewCommitSpec(cherryStr) - if err != nil { - return nil, "", err - } - headRef, err := dEnv.RepoStateReader().CWBHeadRef() - if err != nil { - return nil, "", err - } - cherryCm, err := dEnv.DoltDB.Resolve(ctx, cherrySpec, headRef) - if err != nil { - return nil, "", err - } + if succeeded { + // on success, print the commit info + commit, err := getCommitInfo(queryist, sqlCtx, commitHash) + if err != nil { + return fmt.Errorf("error: failed to get commit metadata for ref '%s': %v", commitHash, err) + } - if len(cherryCm.DatasParents()) > 1 { - return nil, "", errhand.BuildDError("cherry-picking a merge commit is not supported.").Build() - } - if len(cherryCm.DatasParents()) == 0 { - return nil, "", errhand.BuildDError("cherry-picking a commit without parents is not supported.").Build() - } + cli.ExecuteWithStdioRestored(func() { + pager := outputpager.Start() + defer pager.Stop() - cherryCM, err := cherryCm.GetCommitMeta(ctx) - if err != nil { - return nil, "", err - } - commitMsg := cherryCM.Description + printCommitInfo(pager, 0, false, "auto", commit) + }) - cherryRoot, err := cherryCm.GetRootValue(ctx) - if err != nil { - return nil, "", err + return nil + } else { + // this failure could only have been caused by constraint violations or conflicts during cherry-pick + return ErrCherryPickConflictsOrViolations.New() } +} - parentCm, err := dEnv.DoltDB.ResolveParent(ctx, cherryCm, 0) +func cherryPickAbort(queryist cli.Queryist, sqlCtx *sql.Context) error { + query := "call dolt_cherry_pick('--abort')" + _, err := GetRowsForSql(queryist, sqlCtx, query) if err != nil { - return nil, "", err - } - parentRoot, err := parentCm.GetRootValue(ctx) - if err != nil { - return nil, "", err + errorText := err.Error() + switch errorText { + case "fatal: There is no merge to abort": + return fmt.Errorf("error: There is no cherry-pick merge to abort") + default: + return err + } } + return nil +} - // use parent of cherry-pick as ancestor to merge - mo := merge.MergeOpts{IsCherryPick: true} - result, err := merge.MergeRoots(ctx, workingRoot, cherryRoot, parentRoot, cherryCm, parentCm, opts, mo) +func hasStagedAndUnstagedChanged(queryist cli.Queryist, sqlCtx *sql.Context) (hasStagedChanges bool, hasUnstagedChanges bool, err error) { + stagedTables, unstagedTables, err := GetDoltStatus(queryist, sqlCtx) if err != nil { - return nil, "", err - } - - // If any of the merge stats show a data or schema conflict or a constraint - // violation, record that a merge is in progress. - for _, stats := range result.Stats { - if stats.HasArtifacts() { - ws, err := dEnv.WorkingSet(ctx) - if err != nil { - return nil, "", err - } - newWorkingSet := ws.StartMerge(cherryCm, cherryStr) - err = dEnv.UpdateWorkingSet(ctx, newWorkingSet) - if err != nil { - return nil, "", err - } - - break - } + return false, false, fmt.Errorf("error: failed to get dolt status: %w", err) } - return result, commitMsg, nil + hasStagedChanges = len(stagedTables) > 0 + hasUnstagedChanges = len(unstagedTables) > 0 + return hasStagedChanges, hasUnstagedChanges, nil } diff --git a/go/cmd/dolt/commands/status.go b/go/cmd/dolt/commands/status.go index bfa08a7a667..e2b230304fd 100644 --- a/go/cmd/dolt/commands/status.go +++ b/go/cmd/dolt/commands/status.go @@ -17,7 +17,6 @@ package commands import ( "context" "fmt" - "strconv" "strings" "github.com/dolthub/go-mysql-server/sql" @@ -671,23 +670,3 @@ func getJsonDocumentColAsString(sqlCtx *sql.Context, col interface{}) (string, e return "", fmt.Errorf("unexpected type %T, was expecting JSONDocument or string", v) } } - -// getInt64ColAsInt64 returns the value of an int64 column as a string -// This is necessary because Queryist may return an int64 column as an int64 (when using SQLEngine) -// or as a string (when using ConnectionQueryist). -func getInt64ColAsInt64(col interface{}) (int64, error) { - switch v := col.(type) { - case uint64: - return int64(v), nil - case int64: - return v, nil - case string: - iv, err := strconv.ParseInt(v, 10, 64) - if err != nil { - return 0, err - } - return iv, nil - default: - return 0, fmt.Errorf("unexpected type %T, was expecting int64, uint64 or string", v) - } -} diff --git a/go/cmd/dolt/commands/utils.go b/go/cmd/dolt/commands/utils.go index bcbee9469d7..8e2a27a699f 100644 --- a/go/cmd/dolt/commands/utils.go +++ b/go/cmd/dolt/commands/utils.go @@ -20,6 +20,7 @@ import ( "fmt" "net" "path/filepath" + "strconv" "time" "github.com/dolthub/go-mysql-server/sql" @@ -309,6 +310,28 @@ func GetTinyIntColAsBool(col interface{}) (bool, error) { } } +// getInt64ColAsInt64 returns the value of an int64 column as a string +// This is necessary because Queryist may return an int64 column as an int64 (when using SQLEngine) +// or as a string (when using ConnectionQueryist). +func getInt64ColAsInt64(col interface{}) (int64, error) { + switch v := col.(type) { + case int: + return int64(v), nil + case uint64: + return int64(v), nil + case int64: + return v, nil + case string: + iv, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return 0, err + } + return iv, nil + default: + return 0, fmt.Errorf("unexpected type %T, was expecting int64, uint64 or string", v) + } +} + func getActiveBranchName(sqlCtx *sql.Context, queryEngine cli.Queryist) (string, error) { query := "SELECT active_branch()" rows, err := GetRowsForSql(queryEngine, sqlCtx, query) @@ -399,3 +422,54 @@ func buildAuthResponse(salt []byte, password string) []byte { return shaPwd } + +// GetDoltStatus retrieves the status of the current working set of changes in the working set, and returns two +// lists of modified tables: staged and unstaged. If both lists are empty, there are no changes in the working set. +// The list of unstaged tables does not include tables that are ignored, as configured by the dolt_ignore table. +func GetDoltStatus(queryist cli.Queryist, sqlCtx *sql.Context) (stagedChangedTables map[string]bool, unstagedChangedTables map[string]bool, err error) { + stagedChangedTables = make(map[string]bool) + unstagedChangedTables = make(map[string]bool) + err = nil + + ignoredPatterns, err := getIgnoredTablePatternsFromSql(queryist, sqlCtx) + if err != nil { + return stagedChangedTables, unstagedChangedTables, fmt.Errorf("error: failed to get ignored table patterns: %w", err) + } + + var statusRows []sql.Row + statusRows, err = GetRowsForSql(queryist, sqlCtx, "select * from dolt_status;") + if err != nil { + return stagedChangedTables, unstagedChangedTables, fmt.Errorf("error: failed to get dolt status: %w", err) + } + + for _, row := range statusRows { + tableName := row[0].(string) + staged := row[1] + var isStaged bool + isStaged, err = GetTinyIntColAsBool(staged) + if err != nil { + return + } + if isStaged { + stagedChangedTables[tableName] = true + } else { + // filter out ignored tables from untracked tables + ignored, err := ignoredPatterns.IsTableNameIgnored(tableName) + if conflict := doltdb.AsDoltIgnoreInConflict(err); conflict != nil { + continue + } else if err != nil { + return stagedChangedTables, unstagedChangedTables, fmt.Errorf("error: failed to check if table '%s' is ignored: %w", tableName, err) + } else if ignored == doltdb.DontIgnore { + // no-op + } else if ignored == doltdb.Ignore { + continue + } else { + return stagedChangedTables, unstagedChangedTables, fmt.Errorf("unrecognized ignore result value: %v", ignored) + } + + unstagedChangedTables[tableName] = true + } + } + + return stagedChangedTables, unstagedChangedTables, nil +} diff --git a/integration-tests/bats/cherry-pick.bats b/integration-tests/bats/cherry-pick.bats index 18753ddfe5d..9703afd5ef6 100644 --- a/integration-tests/bats/cherry-pick.bats +++ b/integration-tests/bats/cherry-pick.bats @@ -4,7 +4,10 @@ load $BATS_TEST_DIRNAME/helper/common.bash setup() { setup_common - dolt sql -q "CREATE TABLE test(pk BIGINT PRIMARY KEY, v varchar(10), index(v))" + dolt sql <>> $output" + [ $status -eq 0 ] + [[ ! $output =~ "Changes to be committed" ]] || false + [[ $output =~ "Unmerged paths" ]] || false + [[ $output =~ "both modified: other" ]] || false + [[ $output =~ "both modified: test" ]] || false + + dolt conflicts resolve --theirs . + + run dolt sql -q "select * from other" + [ $status -eq 0 ] + [[ $output =~ "1 | 2" ]] || false + + run dolt sql -q "select * from test" + [ $status -eq 0 ] + [[ $output =~ "4 | f" ]] || false + + dolt commit -m "committing cherry-picked change" +} + + @test "sql-cherry-pick: commit with CREATE TABLE" { dolt sql -q "CREATE TABLE table_a (pk BIGINT PRIMARY KEY, v varchar(10))" dolt add . diff --git a/integration-tests/bats/sql-local-remote.bats b/integration-tests/bats/sql-local-remote.bats index 085c4c1df75..723469de3dc 100644 --- a/integration-tests/bats/sql-local-remote.bats +++ b/integration-tests/bats/sql-local-remote.bats @@ -736,3 +736,73 @@ SQL [[ "$remoteOutput" == "$localOutput" ]] || false } + +@test "sql-local-remote: verify dolt cherry-pick behavior" { + cd altDB + + # setup for cherry-pick.bats + dolt clean + dolt sql -q "CREATE TABLE test(pk BIGINT PRIMARY KEY, v varchar(10), index(v))" + dolt add . + dolt commit -am "Created table" + dolt checkout -b branch1 + dolt sql -q "INSERT INTO test VALUES (1, 'a')" + dolt commit -am "Inserted 1" + dolt sql -q "INSERT INTO test VALUES (2, 'b')" + dolt commit -am "Inserted 2" + dolt sql -q "INSERT INTO test VALUES (3, 'c')" + dolt commit -am "Inserted 3" + run dolt sql -q "SELECT * FROM test" -r csv + [[ "$output" =~ "1,a" ]] || false + [[ "$output" =~ "2,b" ]] || false + [[ "$output" =~ "3,c" ]] || false + + # setup for "cherry-pick: schema change, with data conflict" test + dolt checkout main + dolt sql -q "CREATE TABLE other (pk int primary key, c1 int, c2 int)" + dolt sql -q "INSERT INTO other VALUES (1, 2, 3)" + dolt commit -Am "add other table (on main)" + # Create two commits on branch2: one to assert does NOT get included, and one to cherry pick + dolt checkout -b branch2 + dolt sql -q "INSERT INTO other VALUES (100, 200, 300);" + dolt commit -am "add row 100 to other (on branch2)" + # This ALTER TABLE statement modifies other rows that aren't included in the cherry-picked + # commit – row (100, 200, 300) is modified to (100, 300). This shows up as a conflict + # in the cherry-pick (modified row on one side, row doesn't exist on the other side). + dolt sql -q "ALTER TABLE other DROP COLUMN c1;" + dolt sql -q "INSERT INTO other VALUES (10, 30);" + dolt sql -q "INSERT INTO test VALUES (100, 'q');" + dolt commit -am "alter table, add row 10 to other, add row 100 to test (on branch2)" + + # actual cherry-pick test + dolt checkout main + run dolt cherry-pick branch2 + [ $status -eq 1 ] + [[ $output =~ "Unable to apply commit cleanly due to conflicts or constraint violations" ]] || false + localCherryPickOutput=$output + + # Assert that table 'test' is staged, but table 'other' is not staged, since it had conflicts + run dolt sql -q "SELECT table_name, case when staged = 0 then 'staged' else 'working' end as location, status from dolt_status;" + [ $status -eq 0 ] + [[ $output =~ "| test | working | modified |" ]] || false + [[ $output =~ "| other | staged | modified |" ]] || false + + # setup for remote test + dolt checkout main + dolt reset --hard main + + # start server + start_sql_server altDB + + run dolt cherry-pick branch2 + [ $status -eq 1 ] + [[ $output =~ "Unable to apply commit cleanly due to conflicts or constraint violations" ]] || false + remoteCherryPickOutput=$output + # Assert that table 'test' is staged, but table 'other' is not staged, since it had conflicts + run dolt sql -q "SELECT table_name, case when staged = 0 then 'staged' else 'working' end as location, status from dolt_status;" + [ $status -eq 0 ] + [[ $output =~ "| test | working | modified |" ]] || false + [[ $output =~ "| other | staged | modified |" ]] || false + + [[ "$localCherryPickOutput" == "$remoteCherryPickOutput" ]] || false +}