diff --git a/go/cmd/dolt/commands/cherry-pick.go b/go/cmd/dolt/commands/cherry-pick.go index 5cc82c175c4..735883d0d64 100644 --- a/go/cmd/dolt/commands/cherry-pick.go +++ b/go/cmd/dolt/commands/cherry-pick.go @@ -26,6 +26,7 @@ import ( "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/branch_control" "github.com/dolthub/dolt/go/libraries/doltcore/env" + "github.com/dolthub/dolt/go/libraries/doltcore/env/actions" "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/store/util/outputpager" eventsapi "github.com/dolthub/eventsapi_schema/dolt/services/eventsapi/v1alpha1" @@ -51,6 +52,12 @@ var ErrCherryPickConflictsOrViolations = errors.NewKind("error: Unable to apply "To undo all changes from this cherry-pick operation, use `dolt cherry-pick --abort`.\n" + "For more information on handling conflicts, see: https://docs.dolthub.com/concepts/dolt/git/conflicts") +var ErrCherryPickVerificationFailed = errors.NewKind("error: Commit verification failed. Your changes are staged " + + "in the working set. Fix the failing tests, use `dolt add` to stage your changes, then " + + "`dolt cherry-pick --continue` to complete the cherry-pick.\n" + + "To undo all changes from this cherry-pick operation, use `dolt cherry-pick --abort`.\n" + + "Run `dolt sql -q 'select * from dolt_test_run()` to see which tests are failing.") + type CherryPickCmd struct{} // Name returns the name of the Dolt cli command. This is what is used on the command line to invoke the command. @@ -166,6 +173,10 @@ hint: commit your changes (dolt commit -am \"\") or reset them (dolt re } rows, err := cli.GetRowsForSql(queryist, sqlCtx, q) if err != nil { + if actions.ErrCommitVerificationFailed.Is(err) { + cli.PrintErrln(err.Error()) + return ErrCherryPickVerificationFailed.New() + } errorText := err.Error() switch { case strings.Contains("nothing to commit", errorText): @@ -250,20 +261,27 @@ func cherryPickContinue(sqlCtx *sql.Context, queryist cli.Queryist) error { query := "call dolt_cherry_pick('--continue')" rows, err := cli.GetRowsForSql(queryist, sqlCtx, query) if err != nil { + if actions.ErrCommitVerificationFailed.Is(err) { + cli.PrintErrln(err.Error()) + return ErrCherryPickVerificationFailed.New() + } return err } if len(rows) != 1 { return fmt.Errorf("error: unexpected number of rows returned from dolt_cherry_pick: %d", len(rows)) } - if len(rows[0]) != 4 { + + // No version of dolt_cherry_pick has ever returned less than 4 columns. We don't set an upper bound here to + // allow for servers to add more columns in the future without breaking compatibility with older clients. + if len(rows[0]) < 4 { return fmt.Errorf("error: unexpected number of columns returned from dolt_cherry_pick: %d", len(rows[0])) } row := rows[0] // We expect to get an error if there were problems, but we also could get any of the conflicts and - // vacation counts being greater than 0 if there were problems. If we got here without an error, + // violation counts being greater than 0 if there were problems. If we got here without an error, // but we have conflicts or violations, we should report and stop. dataConflicts, err := getInt64ColAsInt64(row[1]) if err != nil { diff --git a/go/cmd/dolt/commands/merge.go b/go/cmd/dolt/commands/merge.go index db5aea5ab02..9c65f8c3c5c 100644 --- a/go/cmd/dolt/commands/merge.go +++ b/go/cmd/dolt/commands/merge.go @@ -33,6 +33,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/diff" "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" "github.com/dolthub/dolt/go/libraries/doltcore/env" + "github.com/dolthub/dolt/go/libraries/doltcore/env/actions" "github.com/dolthub/dolt/go/libraries/doltcore/merge" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dprocedures" "github.com/dolthub/dolt/go/libraries/utils/argparser" @@ -165,6 +166,11 @@ func (cmd MergeCmd) Exec(ctx context.Context, commandStr string, args []string, return 1 } + if msg := getMergeMessage(mergeResultRow); strings.HasPrefix(msg, actions.CommitVerificationFailedPrefix) { + cli.Println(msg) + return 1 + } + if !apr.Contains(cli.AbortParam) { //todo: refs with the `remotes/` prefix will fail to get a hash headHash, headHashErr := getHashOf(queryist.Queryist, queryist.Context, "HEAD") @@ -751,3 +757,17 @@ func everythingUpToDate(row sql.Row) (bool, error) { return false, nil } + +// getMergeMessage extracts the message column from a merge result row. +func getMergeMessage(row sql.Row) string { + if len(row) == 3 { + if msg, ok := row[2].(string); ok { + return msg + } + } else if len(row) == 4 { + if msg, ok := row[3].(string); ok { + return msg + } + } + return "" +} diff --git a/go/cmd/dolt/commands/rebase.go b/go/cmd/dolt/commands/rebase.go index ea8ec288a1e..cce2010b26c 100644 --- a/go/cmd/dolt/commands/rebase.go +++ b/go/cmd/dolt/commands/rebase.go @@ -28,6 +28,7 @@ import ( "github.com/dolthub/dolt/go/cmd/dolt/errhand" "github.com/dolthub/dolt/go/libraries/doltcore/dconfig" "github.com/dolthub/dolt/go/libraries/doltcore/env" + "github.com/dolthub/dolt/go/libraries/doltcore/env/actions" "github.com/dolthub/dolt/go/libraries/doltcore/rebase" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dprocedures" "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" @@ -429,7 +430,8 @@ func syncCliBranchToSqlSessionBranch(ctx *sql.Context, dEnv *env.DoltEnv) error } // isRebaseConflictError checks if the given error represents a rebase pause condition -// (data conflicts) that should not abort the rebase but instead allow the user to resolve/continue. +// (data conflicts or verification failure) that should not abort the rebase but instead +// allow the user to resolve/continue. func isRebaseConflictError(err error) bool { if err == nil { return false @@ -439,8 +441,16 @@ func isRebaseConflictError(err error) bool { if dprocedures.ErrRebaseDataConflict.Is(err) { return true } + if dprocedures.ErrRebaseVerificationFailed.Is(err) { + return true + } + if actions.ErrCommitVerificationFailed.Is(err) { + return true + } // For over-the-wire errors that lose their type, match against error message patterns errMsg := err.Error() - return strings.HasPrefix(errMsg, dprocedures.RebaseDataConflictPrefix) + return strings.HasPrefix(errMsg, dprocedures.RebaseDataConflictPrefix) || + strings.HasPrefix(errMsg, dprocedures.RebaseVerificationFailedPrefix) || + strings.HasPrefix(errMsg, actions.CommitVerificationFailedPrefix) } diff --git a/go/cmd/dolt/commands/remote_test.go b/go/cmd/dolt/commands/remote_test.go index af0c626f1f4..bc1c8eafc9e 100644 --- a/go/cmd/dolt/commands/remote_test.go +++ b/go/cmd/dolt/commands/remote_test.go @@ -156,7 +156,7 @@ func TestRemoteAdd_StoresNormalizedGitUrl(t *testing.T) { normalized, ok, err := env.NormalizeGitRemoteUrl(original) assert.NoError(t, err) assert.True(t, ok) - assert.Equal(t, "git+ssh://git@github.com/timsehn/dolt-in-git.git", normalized) + assert.Equal(t, "git+ssh://git@github.com/./timsehn/dolt-in-git.git", normalized) urlToStore := original if strings.HasPrefix(strings.ToLower(scheme), "git+") { diff --git a/go/go.mod b/go/go.mod index ce44b2ce7e6..29f78b3fe66 100644 --- a/go/go.mod +++ b/go/go.mod @@ -64,7 +64,7 @@ require ( github.com/dolthub/dolt-mcp v0.3.4 github.com/dolthub/eventsapi_schema v0.0.0-20260205214132-a7a3c84c84a1 github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2 - github.com/dolthub/go-mysql-server v0.20.1-0.20260305220757-48a0f17d12e3 + github.com/dolthub/go-mysql-server v0.20.1-0.20260306002943-3e8a45211974 github.com/dolthub/gozstd v0.0.0-20240423170813-23a2903bca63 github.com/edsrzf/mmap-go v1.2.0 github.com/esote/minmaxheap v1.0.0 diff --git a/go/go.sum b/go/go.sum index eaa370099d9..38936d080be 100644 --- a/go/go.sum +++ b/go/go.sum @@ -212,8 +212,8 @@ github.com/dolthub/fslock v0.0.0-20251215194149-ef20baba2318 h1:n+vdH5G5Db+1qnDC github.com/dolthub/fslock v0.0.0-20251215194149-ef20baba2318/go.mod h1:QWql+P17oAAMLnL4HGB5tiovtDuAjdDTPbuqx7bYfa0= github.com/dolthub/go-icu-regex v0.0.0-20250916051405-78a38d478790 h1:zxMsH7RLiG+dlZ/y0LgJHTV26XoiSJcuWq+em6t6VVc= github.com/dolthub/go-icu-regex v0.0.0-20250916051405-78a38d478790/go.mod h1:F3cnm+vMRK1HaU6+rNqQrOCyR03HHhR1GWG2gnPOqaE= -github.com/dolthub/go-mysql-server v0.20.1-0.20260305220757-48a0f17d12e3 h1:vRxMCZVdCiH5CY46UX2xxVqerKLdXu99nz1M4dR2bLY= -github.com/dolthub/go-mysql-server v0.20.1-0.20260305220757-48a0f17d12e3/go.mod h1:8zqJ7NgJ7GEISN7VkYEmrrW3DodmHHMCEjIH72BkoEU= +github.com/dolthub/go-mysql-server v0.20.1-0.20260306002943-3e8a45211974 h1:f2v7C2iXTe/qHGPQLom7MrVe6zXwkl8eDIZ3CWDLIz4= +github.com/dolthub/go-mysql-server v0.20.1-0.20260306002943-3e8a45211974/go.mod h1:8zqJ7NgJ7GEISN7VkYEmrrW3DodmHHMCEjIH72BkoEU= github.com/dolthub/gozstd v0.0.0-20240423170813-23a2903bca63 h1:OAsXLAPL4du6tfbBgK0xXHZkOlos63RdKYS3Sgw/dfI= github.com/dolthub/gozstd v0.0.0-20240423170813-23a2903bca63/go.mod h1:lV7lUeuDhH5thVGDCKXbatwKy2KW80L4rMT46n+Y2/Q= github.com/dolthub/ishell v0.0.0-20240701202509-2b217167d718 h1:lT7hE5k+0nkBdj/1UOSFwjWpNxf+LCApbRHgnCA17XE= diff --git a/go/libraries/doltcore/cherry_pick/cherry_pick.go b/go/libraries/doltcore/cherry_pick/cherry_pick.go index b7172dc3819..95ea09f3cd0 100644 --- a/go/libraries/doltcore/cherry_pick/cherry_pick.go +++ b/go/libraries/doltcore/cherry_pick/cherry_pick.go @@ -82,6 +82,14 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str return "", nil, fmt.Errorf("failed to get roots for current session") } + // Capture the working set BEFORE applying cherry-pick changes. If commit verification + // fails later, we need to set the merge state with preMergeWorking = original working + // root so that --abort properly restores the pre-cherry-pick state. + preApplyWs, wsErr := doltSession.WorkingSet(ctx, dbName) + if wsErr != nil { + return "", nil, wsErr + } + mergeResult, commitMsg, originalCommit, err := cherryPick(ctx, doltSession, roots, dbName, commit, options.EmptyCommitHandling) if err != nil { return "", mergeResult, err @@ -131,6 +139,20 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, *commitProps) if err != nil { + // If verification failed, set merge state and return via the result (not as a Go error). + // Returning a Go error would roll back the transaction, undoing all working set changes. + // Returning nil error instead lets CommitTransaction persist the staged changes and merge + // state, so the user can fix the failing tests and --continue. + if actions.ErrCommitVerificationFailed.Is(err) { + newWs := preApplyWs.StartCherryPick(originalCommit, commit). + WithWorkingRoot(roots.Working). + WithStagedRoot(roots.Staged) + if wsErr := doltSession.SetWorkingSet(ctx, dbName, newWs); wsErr != nil { + return "", nil, wsErr + } + mergeResult.CommitVerificationErr = err + return "", mergeResult, nil + } return "", nil, err } if pendingCommit == nil { @@ -231,9 +253,10 @@ func AbortCherryPick(ctx *sql.Context, dbName string) error { return doltSession.SetWorkingSet(ctx, dbName, newWs) } -// ContinueCherryPick continues a cherry-pick merge that was paused due to conflicts. -// It checks that conflicts have been resolved and creates the final commit with the -// original commit's metadata. +// ContinueCherryPick continues a cherry-pick merge that was paused due to conflicts or +// commit verification failure. It checks that conflicts have been resolved and creates +// the final commit with the original commit's metadata. Returns (hash, dataConflicts, +// schemaConflicts, constraintViolations, verificationFailures, error). func ContinueCherryPick(ctx *sql.Context, dbName string) (string, int, int, int, error) { doltSession := dsess.DSessFromSess(ctx.Session) @@ -310,6 +333,9 @@ func ContinueCherryPick(ctx *sql.Context, dbName string) (string, int, int, int, pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, commitProps) if err != nil { + if actions.ErrCommitVerificationFailed.Is(err) { + return "", 0, 0, 0, err + } return "", 0, 0, 0, fmt.Errorf("error: failed to create pending commit: %w", err) } if pendingCommit == nil { diff --git a/go/libraries/doltcore/dbfactory/git_remote.go b/go/libraries/doltcore/dbfactory/git_remote.go index 1fe1e73e3fd..deafd3ff484 100644 --- a/go/libraries/doltcore/dbfactory/git_remote.go +++ b/go/libraries/doltcore/dbfactory/git_remote.go @@ -134,10 +134,11 @@ func (fact GitRemoteFactory) CreateDB(ctx context.Context, nbf *types.NomsBinFor } // Ensure the configured git remote exists and points to the underlying git remote URL. - if err := ensureGitRemoteURL(ctx, cacheRepo, remoteName, remoteURL.String()); err != nil { + gitURL := gitRemoteURLString(remoteURL) + if err := ensureGitRemoteURL(ctx, cacheRepo, remoteName, gitURL); err != nil { return nil, nil, nil, err } - if err := ensureRemoteHasBranches(ctx, cacheRepo, remoteName, remoteURL.String()); err != nil { + if err := ensureRemoteHasBranches(ctx, cacheRepo, remoteName, gitURL); err != nil { return nil, nil, nil, err } @@ -186,6 +187,23 @@ func parseGitRemoteFactoryURL(urlObj *url.URL, params map[string]interface{}) (r return &cp, ref, nil } +// gitRemoteURLString converts a parsed remote URL back to a string suitable for +// passing to git commands. If the URL has an ssh scheme and the path starts with +// "/./", it was originally an SCP-style relative path (e.g. git@host:path.git). +// We reconstruct SCP-style format so git treats the path as relative to the SSH +// user's home directory. +func gitRemoteURLString(u *url.URL) string { + if strings.ToLower(u.Scheme) == "ssh" && strings.HasPrefix(u.Path, "/./") { + // Reconstruct SCP-style: [user@]host:relativePath + host := u.Host + if u.User != nil { + host = u.User.String() + "@" + u.Host + } + return host + ":" + strings.TrimPrefix(u.Path, "/./") + } + return u.String() +} + func resolveGitRemoteRef(params map[string]interface{}) string { // Prefer an explicit remote parameter (e.g. from `--ref`). if params != nil { diff --git a/go/libraries/doltcore/dbfactory/git_remote_test.go b/go/libraries/doltcore/dbfactory/git_remote_test.go index 618dbc02738..cd7c586ce73 100644 --- a/go/libraries/doltcore/dbfactory/git_remote_test.go +++ b/go/libraries/doltcore/dbfactory/git_remote_test.go @@ -18,6 +18,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "net/url" "os" "os/exec" "path/filepath" @@ -49,6 +50,43 @@ func shortTempDir(t *testing.T) string { return dir } +func TestGitRemoteURLString(t *testing.T) { + tests := []struct { + name string + rawURL string + expected string + }{ + { + name: "ssh relative path reconstructs SCP-style", + rawURL: "ssh://git@myhost/./relative/repo.git", + expected: "git@myhost:relative/repo.git", + }, + { + name: "ssh absolute path unchanged", + rawURL: "ssh://git@myhost/abs/repo.git", + expected: "ssh://git@myhost/abs/repo.git", + }, + { + name: "https unchanged", + rawURL: "https://example.com/org/repo.git", + expected: "https://example.com/org/repo.git", + }, + { + name: "ssh no user relative path", + rawURL: "ssh://myhost/./relative/repo.git", + expected: "myhost:relative/repo.git", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse(tt.rawURL) + require.NoError(t, err) + got := gitRemoteURLString(u) + require.Equal(t, tt.expected, got) + }) + } +} + func TestGitRemoteFactory_GitFile_RequiresGitCacheRootParam(t *testing.T) { ctx := context.Background() _, _, _, err := CreateDB(ctx, types.Format_Default, "git+file:///tmp/remote.git", map[string]interface{}{}) diff --git a/go/libraries/doltcore/env/actions/commit.go b/go/libraries/doltcore/env/actions/commit.go index 15b1a619e2d..c85ee4497e2 100644 --- a/go/libraries/doltcore/env/actions/commit.go +++ b/go/libraries/doltcore/env/actions/commit.go @@ -22,12 +22,17 @@ import ( gms "github.com/dolthub/go-mysql-server" "github.com/dolthub/go-mysql-server/sql" + goerrors "gopkg.in/src-d/go-errors.v1" "github.com/dolthub/dolt/go/libraries/doltcore/diff" "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" "github.com/dolthub/dolt/go/store/datas" ) +const CommitVerificationFailedPrefix = "commit verification failed:" + +var ErrCommitVerificationFailed = goerrors.NewKind(CommitVerificationFailedPrefix + " %s") + type CommitStagedProps struct { Message string Date time.Time @@ -45,10 +50,10 @@ const ( DoltCommitVerificationGroups = "dolt_commit_verification_groups" ) -// GetCommitRunTestGroups returns the test groups to run for commit operations +// getCommitRunTestGroups returns the test groups to run for commit operations // Returns empty slice if no tests should be run, ["*"] if all tests should be run, // or specific group names if only those groups should be run -func GetCommitRunTestGroups() []string { +func getCommitRunTestGroups() []string { _, val, ok := sql.SystemVariables.GetGlobal(DoltCommitVerificationGroups) if !ok { return nil @@ -147,7 +152,7 @@ func GetCommitStaged( } if !props.SkipVerification { - testGroups := GetCommitRunTestGroups() + testGroups := getCommitRunTestGroups() if len(testGroups) > 0 { err := runCommitVerification(ctx, testGroups) if err != nil { @@ -164,6 +169,9 @@ func GetCommitStaged( return db.NewPendingCommit(ctx, roots, mergeParents, props.Amend, meta) } +// runCommitVerification runs the commit verification tests for the given test groups. +// If any tests fail, it returns ErrCommitVerificationFailed wrapping the failure details. +// Callers can use errors.Is(err, ErrCommitVerificationFailed) to detect this case. func runCommitVerification(ctx *sql.Context, testGroups []string) error { type sessionInterface interface { sql.Session @@ -216,7 +224,7 @@ func runTestsUsingDtablefunctions(ctx *sql.Context, engine *gms.Engine, testGrou } if len(allFailures) > 0 { - return fmt.Errorf("commit verification failed: %s", strings.Join(allFailures, ", ")) + return ErrCommitVerificationFailed.New(strings.Join(allFailures, ", ")) } return nil diff --git a/go/libraries/doltcore/env/git_remote_url.go b/go/libraries/doltcore/env/git_remote_url.go index 0cb588855e0..c014e5f5cb1 100644 --- a/go/libraries/doltcore/env/git_remote_url.go +++ b/go/libraries/doltcore/env/git_remote_url.go @@ -74,7 +74,15 @@ func NormalizeGitRemoteUrl(urlArg string) (normalized string, ok bool, err error // scp-like ssh: [user@]host:path/repo.git (no scheme, no ://) if isScpLikeGitRemote(urlArg) { host, p := splitScpLike(urlArg) - ssh := "git+ssh://" + host + "/" + strings.TrimPrefix(p, "/") + var pathPart string + if strings.HasPrefix(p, "/") { + // Absolute path: git@host:/abs/repo.git → /abs/repo.git + pathPart = p + } else { + // Relative path: git@host:path.git → /./path.git + pathPart = "/./" + p + } + ssh := "git+ssh://" + host + pathPart u, err := url.Parse(ssh) if err != nil { return "", false, err diff --git a/go/libraries/doltcore/env/git_remote_url_test.go b/go/libraries/doltcore/env/git_remote_url_test.go index 122451291ab..e764d64de17 100644 --- a/go/libraries/doltcore/env/git_remote_url_test.go +++ b/go/libraries/doltcore/env/git_remote_url_test.go @@ -43,11 +43,18 @@ func TestNormalizeGitRemoteUrl(t *testing.T) { require.Equal(t, "git+https://example.com/org/repo.git", got) }) - t.Run("scp-style becomes git+ssh", func(t *testing.T) { + t.Run("scp-style relative becomes git+ssh with dot marker", func(t *testing.T) { got, ok, err := NormalizeGitRemoteUrl("git@github.com:org/repo.git") require.NoError(t, err) require.True(t, ok) - require.Equal(t, "git+ssh://git@github.com/org/repo.git", got) + require.Equal(t, "git+ssh://git@github.com/./org/repo.git", got) + }) + + t.Run("scp-style absolute becomes git+ssh without dot marker", func(t *testing.T) { + got, ok, err := NormalizeGitRemoteUrl("git@host:/abs/repo.git") + require.NoError(t, err) + require.True(t, ok) + require.Equal(t, "git+ssh://git@host/abs/repo.git", got) }) t.Run("schemeless host/path defaults to git+https", func(t *testing.T) { diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index ec84f0f3e66..c8f066f47b2 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -87,9 +87,10 @@ func MergeCommits(ctx *sql.Context, tableResolver doltdb.TableResolver, commit, } type Result struct { - Root doltdb.RootValue - SchemaConflicts []SchemaConflict - Stats map[doltdb.TableName]*MergeStats + Root doltdb.RootValue + SchemaConflicts []SchemaConflict + Stats map[doltdb.TableName]*MergeStats + CommitVerificationErr error } func (r Result) HasSchemaConflicts() bool { diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go b/go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go index ce6833490a0..3623a193e7f 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go @@ -25,6 +25,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/branch_control" "github.com/dolthub/dolt/go/libraries/doltcore/cherry_pick" "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" ) var ErrEmptyCherryPick = errors.New("cannot cherry-pick empty string") @@ -64,6 +65,7 @@ func doltCherryPick(ctx *sql.Context, args ...string) (sql.RowIter, error) { // doDoltCherryPick attempts to perform a cherry-pick merge based on the arguments specified in |args| and returns // the new, created commit hash (if it was successful created), a count of the number of tables with data conflicts, // a count of the number of tables with schema conflicts, and a count of the number of tables with constraint violations. +// Verification failures are returned as errors. func doDoltCherryPick(ctx *sql.Context, args []string) (string, int, int, int, error) { // Get the information for the sql context. dbName := ctx.GetCurrentDatabase() @@ -119,6 +121,15 @@ func doDoltCherryPick(ctx *sql.Context, args []string) (string, int, int, int, e } if mergeResult != nil { + if mergeResult.CommitVerificationErr != nil { + // Commit the transaction to persist the dirty working set to the staged working set. + // This allows the user to address the verification failure and then `--continue` the cherry-pick. + doltSession := dsess.DSessFromSess(ctx.Session) + if txErr := doltSession.CommitTransaction(ctx, doltSession.GetTransaction()); txErr != nil { + return "", 0, 0, 0, txErr + } + return "", 0, 0, 0, mergeResult.CommitVerificationErr + } return "", mergeResult.CountOfTablesWithDataConflicts(), mergeResult.CountOfTablesWithSchemaConflicts(), diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_merge.go b/go/libraries/doltcore/sqle/dprocedures/dolt_merge.go index 1f68de8e792..083d8df48ff 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_merge.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_merge.go @@ -245,6 +245,10 @@ func performMerge( } ctx.Warn(DoltMergeWarningCode, "%s", err.Error()) return ws, "", hasConflictsOrViolations, threeWayMerge, "", err + } else if actions.ErrCommitVerificationFailed.Is(err) { + // We don't return an error here because that rolls back the transaction, while we actually need the + // staged data to allow the user to address the verification problem. + return ws, "", noConflictsOrViolations, threeWayMerge, err.Error(), nil } else if err != nil { return ws, "", noConflictsOrViolations, threeWayMerge, "", err } @@ -314,6 +318,9 @@ func performMerge( } commit, _, err = doDoltCommit(ctx, args) if err != nil { + if actions.ErrCommitVerificationFailed.Is(err) { + return ws, "", noConflictsOrViolations, threeWayMerge, err.Error(), nil + } return ws, commit, noConflictsOrViolations, threeWayMerge, "", err } } @@ -457,6 +464,9 @@ func executeNoFFMerge( SkipVerification: skipVerification, }) if err != nil { + if actions.ErrCommitVerificationFailed.Is(err) { + return ws, nil, err + } return nil, nil, err } diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index a770f6485f9..7d665f02402 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -144,6 +144,13 @@ var ErrRebaseDataConflict = goerrors.NewKind(RebaseDataConflictPrefix + " %s (%s "Resolve the conflicts and remove them from the dolt_conflicts_ tables, " + "then continue the rebase by calling dolt_rebase('--continue')") +const RebaseVerificationFailedPrefix = "commit verification failed while rebasing commit" + +// ErrRebaseVerificationFailed is used when commit verification tests fail while rebasing a commit. +// The workspace is left dirty so the user can fix the failing tests and retry using --continue. +var ErrRebaseVerificationFailed = goerrors.NewKind(RebaseVerificationFailedPrefix + " %s (%s): %s\n\n" + + "Fix the failing tests and stage your changes, then continue the rebase by calling dolt_rebase('--continue')") + var EditPausePrefix = "edit action paused at commit" // createEditPauseMessage creates a pause message for edit actions @@ -985,6 +992,19 @@ func handleRebaseCherryPick( return newRebaseError(ErrRebaseDataConflict.New(planStep.CommitHash, planStep.CommitMsg)) } + if mergeResult != nil && mergeResult.CommitVerificationErr != nil { + if doltSession.GetTransaction() == nil { + _, txErr := doltSession.StartTransaction(ctx, sql.ReadWrite) + if txErr != nil { + return newRebaseError(txErr) + } + } + if txErr := doltSession.CommitTransaction(ctx, doltSession.GetTransaction()); txErr != nil { + return newRebaseError(txErr) + } + return newRebaseError(mergeResult.CommitVerificationErr) + } + // If this is an edit action and no conflicts occurred, pause the rebase to allow user modifications if planStep.Action == rebase.RebaseActionEdit && err == nil { return newRebasePause(createEditPauseMessage(planStep.CommitHash, planStep.CommitMsg)) diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_commit_verification.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_commit_verification.go index 0a2251cc953..08eb75e1753 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_commit_verification.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_commit_verification.go @@ -188,7 +188,7 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, }, { - Name: "cherry-pick with test verification enabled - tests pass", + Name: "cherry-pick with test verification enabled - test pass, then test fails", SetUpScript: []string{ "SET GLOBAL dolt_commit_verification_groups = '*'", "CREATE TABLE users (id INT PRIMARY KEY, name VARCHAR(100) NOT NULL, email VARCHAR(100))", @@ -209,6 +209,7 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { + // Verify that tests _can_ pass by cherry-picking the first commit, which adds Bob and updates the test to expect 2 users. Query: "CALL dolt_cherry_pick(@commit_1_hash)", Expected: []sql.Row{{commitHash, int64(0), int64(0), int64(0)}}, }, @@ -216,6 +217,21 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ Query: "CALL dolt_cherry_pick(@commit_2_hash)", ExpectedErrStr: "commit verification failed: test_user_count_update (Assertion failed: expected_single_value equal to 2, got 3)", }, + { + // users table is staged (merged content with charlie added is preserved) + Query: "SELECT staged FROM dolt_status WHERE table_name = 'users'", + Expected: []sql.Row{{uint64(1)}}, + }, + { + // --abort restores clean state, with Bob in the table (from first cherry-pick). + Query: "CALL dolt_cherry_pick('--abort')", + SkipResultsCheck: true, + }, + { + Query: "SELECT COUNT(*) FROM users", + Expected: []sql.Row{{int64(2)}}, + }, + {}, { // Test harness bleeds GLOBAL variable changes across tests, so reset after each test. Query: "SET GLOBAL dolt_commit_verification_groups = ''", SkipResultsCheck: true, @@ -223,7 +239,7 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, }, { - Name: "cherry-pick with test verification enabled - tests fail, aborted", + Name: "cherry-pick with test verification enabled - tests fail, leaves dirty state", SetUpScript: []string{ "SET GLOBAL dolt_commit_verification_groups = '*'", "CREATE TABLE users (id INT PRIMARY KEY, name VARCHAR(100) NOT NULL, email VARCHAR(100))", @@ -240,18 +256,86 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { + // Verification fails; returns specific error, dirty state preserved via explicit CommitTransaction Query: "CALL dolt_cherry_pick(@commit_hash)", ExpectedErrStr: "commit verification failed: test_users_count (Assertion failed: expected_single_value equal to 1, got 2)", }, { + // users table should be staged (cherry-pick changes are preserved by the committed transaction) + Query: "SELECT staged FROM dolt_status WHERE table_name = 'users'", + Expected: []sql.Row{{uint64(1)}}, + }, + { + // --continue without fixing still fails with specific error, dirty state preserved + Query: "CALL dolt_cherry_pick('--continue')", + ExpectedErrStr: "commit verification failed: test_users_count (Assertion failed: expected_single_value equal to 1, got 2)", + }, + { + // Abort restores clean state + Query: "CALL dolt_cherry_pick('--abort')", + SkipResultsCheck: true, + }, + { + // After abort, workspace is clean + Query: "SELECT COUNT(*) FROM dolt_status", + Expected: []sql.Row{{int64(0)}}, + }, + { + // Now cherry-pick succeeds with --skip-verification Query: "CALL dolt_cherry_pick('--skip-verification', @commit_hash)", Expected: []sql.Row{{commitHash, int64(0), int64(0), int64(0)}}, }, + { // Test harness bleeds GLOBAL variable changes across tests, so reset after each test. + Query: "SET GLOBAL dolt_commit_verification_groups = ''", + SkipResultsCheck: true, + }, + }, + }, + { + Name: "cherry-pick with test verification enabled - fix data and --continue succeeds", + SetUpScript: []string{ + "SET GLOBAL dolt_commit_verification_groups = '*'", + "CREATE TABLE users (id INT PRIMARY KEY, name VARCHAR(100) NOT NULL, email VARCHAR(100))", + "INSERT INTO users VALUES (1, 'Alice', 'alice@example.com')", + "INSERT INTO dolt_tests (test_name, test_group, test_query, assertion_type, assertion_comparator, assertion_value) VALUES " + + "('test_users_count', 'unit', 'SELECT COUNT(*) FROM users', 'expected_single_value', '==', '1')", + "CALL dolt_add('.')", + "CALL dolt_commit('-m', 'Initial commit')", + "CALL dolt_checkout('-b', 'feature')", + "INSERT INTO users VALUES (2, 'Bob', 'bob@example.com')", + "CALL dolt_add('.')", + "call dolt_commit_hash_out(@commit_hash,'--skip-verification', '-m', 'Add Bob but dont update test')", + "CALL dolt_checkout('main')", + }, + Assertions: []queries.ScriptTestAssertion{ + { + // Verification fails; returns specific error, dirty state preserved via explicit CommitTransaction + Query: "CALL dolt_cherry_pick(@commit_hash)", + ExpectedErrStr: "commit verification failed: test_users_count (Assertion failed: expected_single_value equal to 1, got 2)", + }, { - Query: "select * from dolt_test_run('*')", - Expected: []sql.Row{ - {"test_users_count", "unit", "SELECT COUNT(*) FROM users", "FAIL", "Assertion failed: expected_single_value equal to 1, got 2"}, - }, + // Fix the test to match the new row count and stage + Query: "UPDATE dolt_tests SET assertion_value = '2' WHERE test_name = 'test_users_count'", + SkipResultsCheck: true, + }, + { + Query: "CALL dolt_add('.')", + SkipResultsCheck: true, + }, + { + // --continue now passes verification and creates the commit + Query: "CALL dolt_cherry_pick('--continue')", + Expected: []sql.Row{{commitHash, int64(0), int64(0), int64(0)}}, + }, + { + // Workspace is clean after successful --continue + Query: "SELECT COUNT(*) FROM dolt_status", + Expected: []sql.Row{{int64(0)}}, + }, + { + // Both users are present + Query: "SELECT COUNT(*) FROM users", + Expected: []sql.Row{{int64(2)}}, }, { // Test harness bleeds GLOBAL variable changes across tests, so reset after each test. Query: "SET GLOBAL dolt_commit_verification_groups = ''", @@ -470,7 +554,7 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, }, { - Name: "merge with test verification enabled - tests fail, merge aborted", + Name: "merge with test verification enabled - tests fail, leaves dirty state", SetUpScript: []string{ "SET GLOBAL dolt_commit_verification_groups = '*'", "CREATE TABLE users (id INT PRIMARY KEY, name VARCHAR(100) NOT NULL, email VARCHAR(100))", @@ -490,8 +574,81 @@ var DoltCommitVerificationScripts = []queries.ScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { - Query: "CALL dolt_merge('feature')", - ExpectedErrStr: "commit verification failed: test_will_fail (Assertion failed: expected_single_value equal to 999, got 3)", + // Verification fails; returns verification failure in message field with no SQL error + // so dirty merge state is preserved (transaction commits). + Query: "CALL dolt_merge('feature')", + Expected: []sql.Row{{"", int64(0), int64(0), "commit verification failed: test_will_fail (Assertion failed: expected_single_value equal to 999, got 3)"}}, + }, + { + // users table is staged (merged content with Bob added is preserved) + Query: "SELECT staged FROM dolt_status WHERE table_name = 'users'", + Expected: []sql.Row{{uint64(1)}}, + }, + { // Test harness bleeds GLOBAL variable changes across tests, so reset after each test. + Query: "SET GLOBAL dolt_commit_verification_groups = ''", + SkipResultsCheck: true, + }, + }, + }, + { + Name: "merge with test verification enabled - fix data and dolt_commit succeeds", + SetUpScript: []string{ + "SET GLOBAL dolt_commit_verification_groups = '*'", + "CREATE TABLE users (id INT PRIMARY KEY, name VARCHAR(100) NOT NULL, email VARCHAR(100))", + "INSERT INTO users VALUES (1, 'Alice', 'alice@example.com')", + "INSERT INTO dolt_tests (test_name, test_group, test_query, assertion_type, assertion_comparator, assertion_value) VALUES " + + "('test_users_count', 'unit', 'SELECT COUNT(*) FROM users', 'expected_single_value', '==', '1')", + "CALL dolt_add('.')", + "CALL dolt_commit('-m', 'Initial commit')", + "CALL dolt_checkout('-b', 'feature')", + "INSERT INTO users VALUES (2, 'Bob', 'bob@example.com')", + "CALL dolt_add('.')", + "CALL dolt_commit('--skip-verification', '-m', 'Add Bob but dont update test')", + "CALL dolt_checkout('main')", + "INSERT INTO users VALUES (3, 'Charlie', 'charlie@example.com')", + "CALL dolt_add('.')", + "CALL dolt_commit('--skip-verification', '-m', 'Add Charlie to force non-FF merge')", + }, + Assertions: []queries.ScriptTestAssertion{ + { + // Verification fails; returns verification failure in message field with no SQL error + // so dirty merge state is preserved (transaction commits). + Query: "CALL dolt_merge('feature')", + Expected: []sql.Row{{"", int64(0), int64(0), "commit verification failed: test_users_count (Assertion failed: expected_single_value equal to 1, got 3)"}}, + }, + { + // dolt_commit without fixing still fails with verification error; dirty state still preserved + Query: "CALL dolt_commit('-m', 'Complete merge without fixing')", + ExpectedErrStr: "commit verification failed: test_users_count (Assertion failed: expected_single_value equal to 1, got 3)", + }, + { + // users table is still staged (dirty state was not lost by the failed dolt_commit) + Query: "SELECT staged FROM dolt_status WHERE table_name = 'users'", + Expected: []sql.Row{{uint64(1)}}, + }, + { + // Fix the test expectation to match the merged user count + Query: "UPDATE dolt_tests SET assertion_value = '3' WHERE test_name = 'test_users_count'", + SkipResultsCheck: true, + }, + { + Query: "CALL dolt_add('.')", + SkipResultsCheck: true, + }, + { + // Now dolt_commit succeeds and creates a merge commit + Query: "CALL dolt_commit('-m', 'Complete merge after fixing test')", + Expected: []sql.Row{{commitHash}}, + }, + { + // Workspace is clean after successful merge commit + Query: "SELECT COUNT(*) FROM dolt_status", + Expected: []sql.Row{{int64(0)}}, + }, + { + // All three users are present after merge + Query: "SELECT COUNT(*) FROM users", + Expected: []sql.Row{{int64(3)}}, }, { // Test harness bleeds GLOBAL variable changes across tests, so reset after each test. Query: "SET GLOBAL dolt_commit_verification_groups = ''", diff --git a/integration-tests/bats/cherry-pick.bats b/integration-tests/bats/cherry-pick.bats index 6dd8a26e483..79b3d3c4c1e 100644 --- a/integration-tests/bats/cherry-pick.bats +++ b/integration-tests/bats/cherry-pick.bats @@ -730,4 +730,4 @@ teardown() { [ $status -eq 1 ] [[ $output =~ "Unable to apply commit cleanly due to conflicts" ]] || false -} \ No newline at end of file +} diff --git a/integration-tests/bats/commit_verification.bats b/integration-tests/bats/commit_verification.bats index 7a0d9b7b861..c76be36ef84 100644 --- a/integration-tests/bats/commit_verification.bats +++ b/integration-tests/bats/commit_verification.bats @@ -107,33 +107,37 @@ SQL [ "$status" -eq 0 ] } -@test "commit_verification: merge with tests enabled - tests fail, merge aborted" { +@test "commit_verification: merge with tests enabled - tests fail, can abort and retry with --skip-verification" { dolt sql -q "SET @@PERSIST.dolt_commit_verification_groups = '*'" - + dolt sql < "$BATS_TMPDIR/fakebin/git" <<'FAKEGIT' +#!/usr/bin/env bash +set -euo pipefail + +git_dir="" +if [[ "${1:-}" == "--git-dir" ]]; then + git_dir="${2:-}" + shift 2 +fi + +cmd="${1:-}" +shift || true + +case "$cmd" in + init) + if [[ "${1:-}" == "--bare" ]]; then + mkdir -p "$git_dir" + exit 0 + fi + ;; + remote) + sub="${1:-}"; shift || true + case "$sub" in + get-url) + shift || true # consume -- + name="${1:-}" + f="${git_dir}/remote_${name}_url" + if [[ -f "$f" ]]; then + cat "$f" + exit 0 + fi + echo "fatal: No such remote '$name'" >&2 + exit 2 + ;; + add|set-url) + shift || true # consume -- + name="${1:-}"; url="${2:-}" + mkdir -p "$git_dir" + printf "%s" "$url" > "${git_dir}/remote_${name}_url" + # Record URL for test assertions. + printf "%s\n" "$url" >> "${BATS_TMPDIR}/recorded_remote_urls" + exit 0 + ;; + esac + ;; + ls-remote) + # Return a dummy branch so ensureRemoteHasBranches succeeds, but the + # subsequent fetch will fail. That's fine — we only need the remote-add + # URL to be recorded. + echo "0000000000000000000000000000000000000000 refs/heads/main" + exit 0 + ;; + fetch) + echo "fatal: could not connect to 'myhost'" >&2 + exit 128 + ;; +esac + +echo "fatal: unknown command" >&2 +exit 128 +FAKEGIT + chmod +x "$BATS_TMPDIR/fakebin/git" + rm -f "$BATS_TMPDIR/recorded_remote_urls" +} + install_fake_git_auth_failure() { mkdir -p "$BATS_TMPDIR/fakebin" cat > "$BATS_TMPDIR/fakebin/git" <<'EOF'