From 272066d1632022d8f1ba7434ebdc8011be54cfc6 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Tue, 20 Aug 2024 12:21:48 -0700 Subject: [PATCH 1/5] Adding support for data conflict resolution with dolt_rebase --- go/gen/fb/serial/workingset.go | 32 +- .../doltcore/cherry_pick/cherry_pick.go | 60 +- go/libraries/doltcore/doltdb/workingset.go | 32 +- go/libraries/doltcore/rebase/rebase.go | 7 + .../doltcore/sqle/dprocedures/dolt_rebase.go | 351 ++++++-- go/libraries/doltcore/sqle/dsess/session.go | 25 + .../sqle/enginetest/dolt_queries_rebase.go | 779 +++++++++++++++++- go/serial/workingset.fbs | 9 + go/store/datas/dataset.go | 12 + go/store/datas/workingset.go | 6 +- 10 files changed, 1218 insertions(+), 95 deletions(-) diff --git a/go/gen/fb/serial/workingset.go b/go/gen/fb/serial/workingset.go index 424b406ac31..ec71849a2a4 100644 --- a/go/gen/fb/serial/workingset.go +++ b/go/gen/fb/serial/workingset.go @@ -555,7 +555,31 @@ func (rcv *RebaseState) MutateCommitBecomesEmptyHandling(n byte) bool { return rcv._tab.MutateByteSlot(12, n) } -const RebaseStateNumFields = 5 +func (rcv *RebaseState) LastAttemptedStep() float32 { + o := flatbuffers.UOffsetT(rcv._tab.Offset(14)) + if o != 0 { + return rcv._tab.GetFloat32(o + rcv._tab.Pos) + } + return 0.0 +} + +func (rcv *RebaseState) MutateLastAttemptedStep(n float32) bool { + return rcv._tab.MutateFloat32Slot(14, n) +} + +func (rcv *RebaseState) RebasingStarted() bool { + o := flatbuffers.UOffsetT(rcv._tab.Offset(16)) + if o != 0 { + return rcv._tab.GetBool(o + rcv._tab.Pos) + } + return false +} + +func (rcv *RebaseState) MutateRebasingStarted(n bool) bool { + return rcv._tab.MutateBoolSlot(16, n) +} + +const RebaseStateNumFields = 7 func RebaseStateStart(builder *flatbuffers.Builder) { builder.StartObject(RebaseStateNumFields) @@ -584,6 +608,12 @@ func RebaseStateAddEmptyCommitHandling(builder *flatbuffers.Builder, emptyCommit func RebaseStateAddCommitBecomesEmptyHandling(builder *flatbuffers.Builder, commitBecomesEmptyHandling byte) { builder.PrependByteSlot(4, commitBecomesEmptyHandling, 0) } +func RebaseStateAddLastAttemptedStep(builder *flatbuffers.Builder, lastAttemptedStep float32) { + builder.PrependFloat32Slot(5, lastAttemptedStep, 0.0) +} +func RebaseStateAddRebasingStarted(builder *flatbuffers.Builder, rebasingStarted bool) { + builder.PrependBoolSlot(6, rebasingStarted, false) +} func RebaseStateEnd(builder *flatbuffers.Builder) flatbuffers.UOffsetT { return builder.EndObject() } diff --git a/go/libraries/doltcore/cherry_pick/cherry_pick.go b/go/libraries/doltcore/cherry_pick/cherry_pick.go index 19527414054..edbee445782 100644 --- a/go/libraries/doltcore/cherry_pick/cherry_pick.go +++ b/go/libraries/doltcore/cherry_pick/cherry_pick.go @@ -108,29 +108,15 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str return "", mergeResult, nil } - commitProps := actions.CommitStagedProps{ - Date: ctx.QueryTime(), - Name: ctx.Client().User, - Email: fmt.Sprintf("%s@%s", ctx.Client().User, ctx.Client().Address), - Message: commitMsg, - } - - if options.CommitMessage != "" { - commitProps.Message = options.CommitMessage - } - if options.Amend { - commitProps.Amend = true - } - if options.EmptyCommitHandling == doltdb.KeepEmptyCommit { - commitProps.AllowEmpty = true + commitProps, err := CreateCommitStagedPropsFromCherryPickOptions(ctx, options) + if err != nil { + return "", nil, err } - if options.CommitBecomesEmptyHandling == doltdb.DropEmptyCommit { - commitProps.SkipEmpty = true - } else if options.CommitBecomesEmptyHandling == doltdb.KeepEmptyCommit { - commitProps.AllowEmpty = true - } else if options.CommitBecomesEmptyHandling == doltdb.StopOnEmptyCommit { - return "", nil, fmt.Errorf("stop on empty commit is not currently supported") + // If no commit message was explicitly provided in the cherry-pick options, + // use the commit message from the cherry-picked commit. + if commitProps.Message == "" { + commitProps.Message = commitMsg } // NOTE: roots are old here (after staging the tables) and need to be refreshed @@ -139,7 +125,7 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str return "", nil, fmt.Errorf("failed to get roots for current session") } - pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, commitProps) + pendingCommit, err := doltSession.NewPendingCommit(ctx, dbName, roots, *commitProps) if err != nil { return "", nil, err } @@ -164,6 +150,36 @@ func CherryPick(ctx *sql.Context, commit string, options CherryPickOptions) (str return h.String(), nil, nil } +// CreateCommitStagedPropsFromCherryPickOptions converts the specified cherry-pick |options| into a CommitStagedProps +// instance that can be used to create a pending commit. +func CreateCommitStagedPropsFromCherryPickOptions(ctx *sql.Context, options CherryPickOptions) (*actions.CommitStagedProps, error) { + commitProps := actions.CommitStagedProps{ + Date: ctx.QueryTime(), + Name: ctx.Client().User, + Email: fmt.Sprintf("%s@%s", ctx.Client().User, ctx.Client().Address), + } + + if options.CommitMessage != "" { + commitProps.Message = options.CommitMessage + } + if options.Amend { + commitProps.Amend = true + } + if options.EmptyCommitHandling == doltdb.KeepEmptyCommit { + commitProps.AllowEmpty = true + } + + if options.CommitBecomesEmptyHandling == doltdb.DropEmptyCommit { + commitProps.SkipEmpty = true + } else if options.CommitBecomesEmptyHandling == doltdb.KeepEmptyCommit { + commitProps.AllowEmpty = true + } else if options.CommitBecomesEmptyHandling == doltdb.StopOnEmptyCommit { + return nil, fmt.Errorf("stop on empty commit is not currently supported") + } + + return &commitProps, nil +} + func previousCommitMessage(ctx *sql.Context) (string, error) { doltSession := dsess.DSessFromSess(ctx.Session) headCommit, err := doltSession.GetHeadCommit(ctx, ctx.GetCurrentDatabase()) diff --git a/go/libraries/doltcore/doltdb/workingset.go b/go/libraries/doltcore/doltdb/workingset.go index 5c0a42ed07e..424b92a1f1c 100755 --- a/go/libraries/doltcore/doltdb/workingset.go +++ b/go/libraries/doltcore/doltdb/workingset.go @@ -66,6 +66,15 @@ type RebaseState struct { // emptyCommitHandling specifies how to handle empty commits that contain no changes. emptyCommitHandling EmptyCommitHandling + + // lastAttemptedStep records the last rebase plan step that was attempted, whether it completed successfully, or + // resulted in conflicts for the user to manually resolve. This field is not valid unless rebasingStarted is set + // to true. + lastAttemptedStep float32 + + // rebasingStarted is true once the rebase plan has been started to execute. Once rebasingStarted is true, the + // value in lastAttemptedStep has been initialized and is valid to read. + rebasingStarted bool } // Branch returns the name of the branch being actively rebased. This is the branch that will be updated to point @@ -93,6 +102,24 @@ func (rs RebaseState) CommitBecomesEmptyHandling() EmptyCommitHandling { return rs.commitBecomesEmptyHandling } +func (rs RebaseState) LastAttemptedStep() float32 { + return rs.lastAttemptedStep +} + +func (rs RebaseState) WithLastAttemptedStep(step float32) *RebaseState { + rs.lastAttemptedStep = step + return &rs +} + +func (rs RebaseState) RebasingStarted() bool { + return rs.rebasingStarted +} + +func (rs RebaseState) WithRebasingStarted(rebasingStarted bool) *RebaseState { + rs.rebasingStarted = rebasingStarted + return &rs +} + type MergeState struct { // the source commit commit *Commit @@ -517,6 +544,8 @@ func newWorkingSet(ctx context.Context, name string, vrw types.ValueReadWriter, branch: dsws.RebaseState.Branch(ctx), commitBecomesEmptyHandling: EmptyCommitHandling(dsws.RebaseState.CommitBecomesEmptyHandling(ctx)), emptyCommitHandling: EmptyCommitHandling(dsws.RebaseState.EmptyCommitHandling(ctx)), + lastAttemptedStep: dsws.RebaseState.LastAttemptedStep(ctx), + rebasingStarted: dsws.RebaseState.RebasingStarted(ctx), } } @@ -613,7 +642,8 @@ func (ws *WorkingSet) writeValues(ctx context.Context, db *DoltDB, meta *datas.W } rebaseState = datas.NewRebaseState(preRebaseWorking.TargetHash(), dCommit.Addr(), ws.rebaseState.branch, - uint8(ws.rebaseState.commitBecomesEmptyHandling), uint8(ws.rebaseState.emptyCommitHandling)) + uint8(ws.rebaseState.commitBecomesEmptyHandling), uint8(ws.rebaseState.emptyCommitHandling), + ws.rebaseState.lastAttemptedStep, ws.rebaseState.rebasingStarted) } return &datas.WorkingSetSpec{ diff --git a/go/libraries/doltcore/rebase/rebase.go b/go/libraries/doltcore/rebase/rebase.go index 13124887efd..16a0b577aae 100644 --- a/go/libraries/doltcore/rebase/rebase.go +++ b/go/libraries/doltcore/rebase/rebase.go @@ -62,6 +62,13 @@ type RebasePlanStep struct { CommitMsg string } +// RebaseOrderAsFloat returns the RebaseOrder as a float32. Float32 provides enough scale and precision to hold +// rebase order values, since they are limited to two decimal points of precision and six total digits. +func (rps *RebasePlanStep) RebaseOrderAsFloat() float32 { + f64, _ := rps.RebaseOrder.Float64() + return float32(f64) +} + // CreateDefaultRebasePlan creates and returns the default rebase plan for the commits between // |startCommit| and |upstreamCommit|, equivalent to the log of startCommit..upstreamCommit. The // default plan includes each of those commits, in the same order they were originally applied, and diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index d3296ed2a6d..b791e266380 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -81,11 +81,28 @@ var DoltRebaseSystemTableSchema = []*sql.Column{ // ErrRebaseUncommittedChanges is used when a rebase is started, but there are uncommitted (and not // ignored) changes in the working set. -var ErrRebaseUncommittedChanges = fmt.Errorf("cannot start a rebase with uncommitted changes") - -// ErrRebaseConflict is used when a merge conflict is detected while rebasing a commit. -var ErrRebaseConflict = goerrors.NewKind( - "merge conflict detected while rebasing commit %s. " + +var ErrRebaseUncommittedChanges = goerrors.NewKind("cannot start a rebase with uncommitted changes") + +// ErrRebaseUnresolvedConflicts is used when a rebase is continued, but there are +// unresolved conflicts still present. +var ErrRebaseUnresolvedConflicts = goerrors.NewKind( + "conflicts detected in tables %s; resolve conflicts before continuing the rebase") + +// ErrRebaseDataConflictWithAutocommit is used when data conflicts are detected in a rebase, but @@autocommit has not +// been disabled, so it's not possible to resolve the conflicts since they would get rolled back automatically. +var ErrRebaseDataConflictWithAutocommit = goerrors.NewKind( + "data conflicts from rebase, but @@autocommit has not been disabled. " + + "@@autocommit must be disabled to resolve conflicts. The rebase has been aborted. " + + "Set @@autocommit to 0 and try the rebase again to resolve the conflicts.") + +// ErrRebaseDataConflict is used when a data conflict is detected while rebasing a commit. +var ErrRebaseDataConflict = goerrors.NewKind("data conflict detected while rebasing commit %s (%s). \n\n" + + "Resolve the conflicts and remove them from the dolt_conflicts_ tables, " + + "then continue the rebase by calling dolt_rebase('--continue')") + +// ErrRebaseSchemaConflict is used when a schema conflict is detected while rebasing a commit. +var ErrRebaseSchemaConflict = goerrors.NewKind( + "schema conflict detected while rebasing commit %s. " + "the rebase has been automatically aborted") // ErrRebaseConflictWithAbortError is used when a merge conflict is detected while rebasing a commit, @@ -367,7 +384,7 @@ func validateWorkingSetCanStartRebase(ctx *sql.Context) error { return err } if !wsOnlyHasIgnoredTables { - return ErrRebaseUncommittedChanges + return ErrRebaseUncommittedChanges.New() } return nil @@ -419,46 +436,193 @@ func abortRebase(ctx *sql.Context) error { return doltSession.SwitchWorkingSet(ctx, ctx.GetCurrentDatabase(), wsRef) } -func continueRebase(ctx *sql.Context) (string, error) { - // TODO: Eventually, when we allow interactive-rebases to be stopped and started (e.g. with the break action, - // or for conflict resolution), we'll need to track what step we're at in the rebase plan. - - // Validate that we are in an interactive rebase +func validateActiveRebase(ctx *sql.Context) error { doltSession := dsess.DSessFromSess(ctx.Session) workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) if err != nil { - return "", err + return err } if !workingSet.RebaseActive() { - return "", fmt.Errorf("no rebase in progress") + return fmt.Errorf("no rebase in progress") + } + return nil +} + +func validateNoConflicts(ctx *sql.Context) error { + doltSession := dsess.DSessFromSess(ctx.Session) + workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) + if err != nil { + return err } + tablesWithDataConflicts, err := doltdb.TablesWithDataConflicts(ctx, workingSet.WorkingRoot()) + if err != nil { + return err + } + if len(tablesWithDataConflicts) > 0 { + return ErrRebaseUnresolvedConflicts.New( + strings.Join(tablesWithDataConflicts, ", ")) + } + + return nil +} + +// loadRebasePlan loads the rebase plan from the current database for the current session and validates it. +func loadRebasePlan(ctx *sql.Context) (*rebase.RebasePlan, error) { + doltSession := dsess.DSessFromSess(ctx.Session) db, err := doltSession.Provider().Database(ctx, ctx.GetCurrentDatabase()) if err != nil { - return "", err + return nil, err } rdb, ok := db.(rebase.RebasePlanDatabase) if !ok { - return "", fmt.Errorf("expected a dsess.RebasePlanDatabase implementation, but received a %T", db) + return nil, fmt.Errorf("expected a dsess.RebasePlanDatabase implementation, but received a %T", db) } rebasePlan, err := rdb.LoadRebasePlan(ctx) if err != nil { + return nil, err + } + err = rebase.ValidateRebasePlan(ctx, rebasePlan) + if err != nil { + return nil, err + } + + return rebasePlan, nil +} + +// isWorkingSetClean returns true if the working set for the current session doesn't contain any staged or +// working changes, other than any changes to ignored tables. +func isWorkingSetClean(ctx *sql.Context) (bool, error) { + doltSession := dsess.DSessFromSess(ctx.Session) + roots, ok := doltSession.GetRoots(ctx, ctx.GetCurrentDatabase()) + if !ok { + return false, fmt.Errorf("unable to get roots for current session") + } + + wsOnlyHasIgnoredTables, err := diff.WorkingSetContainsOnlyIgnoredTables(ctx, roots) + if err != nil { + return false, err + } + if !wsOnlyHasIgnoredTables { + return false, nil + } + + return true, nil +} + +func recordCurrentStep(ctx *sql.Context, step rebase.RebasePlanStep) error { + doltSession := dsess.DSessFromSess(ctx.Session) + if doltSession.GetTransaction() == nil { + _, err := doltSession.StartTransaction(ctx, sql.ReadWrite) + if err != nil { + panic(err) + } + } + + workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) + if err != nil { + return err + } + + // Update the current step in the working set, so that we can continue the merge if this hits a conflict + newWorkingSet := workingSet.WithRebaseState(workingSet.RebaseState(). + WithLastAttemptedStep(step.RebaseOrderAsFloat()). + WithRebasingStarted(true)) + err = doltSession.SetWorkingSet(ctx, ctx.GetCurrentDatabase(), newWorkingSet) + if err != nil { + return err + } + + // Commit the SQL transaction with the LastAttemptedStep update to set that in the branch head's working set + if doltSession.GetTransaction() != nil { + err = doltSession.CommitTransaction(ctx, doltSession.GetTransaction()) + if err != nil { + panic(err) + } + } + + return nil +} + +func continueRebase(ctx *sql.Context) (string, error) { + // Validate that we are in an interactive rebase + if err := validateActiveRebase(ctx); err != nil { return "", err } - err = rebase.ValidateRebasePlan(ctx, rebasePlan) + // If there are conflicts, stop the rebase with an error message about resolving the conflicts before continuing + if err := validateNoConflicts(ctx); err != nil { + return "", err + } + + rebasePlan, err := loadRebasePlan(ctx) if err != nil { return "", err } + doltSession := dsess.DSessFromSess(ctx.Session) for _, step := range rebasePlan.Steps { - err = processRebasePlanStep(ctx, &step, - workingSet.RebaseState().CommitBecomesEmptyHandling(), - workingSet.RebaseState().EmptyCommitHandling()) + workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) + if err != nil { + return "", err + } + + rebaseStepOrder := step.RebaseOrderAsFloat() + lastAttemptedStep := workingSet.RebaseState().LastAttemptedStep() + rebasingStarted := workingSet.RebaseState().RebasingStarted() + + changesCommited, err := isWorkingSetClean(ctx) if err != nil { return "", err } + + if !rebasingStarted && !changesCommited { + return "", ErrRebaseUncommittedChanges.New() + } + + // If we've already executed this step, move to the next plan step + if rebasingStarted && rebaseStepOrder < lastAttemptedStep { + continue + } + + if rebasingStarted && rebaseStepOrder == lastAttemptedStep && !changesCommited { + // If we've already executed this step, but the working set has changes, then we need + // to make the commit for the manual changes made for this step + if err = commitManualChangesForStep(ctx, step); err != nil { + return "", err + } + continue + } + + // If rebasing hasn't started yet or this step is greater than the last attempted step, + // go ahead and execute this step. + if !rebasingStarted || rebaseStepOrder > lastAttemptedStep { + if err = recordCurrentStep(ctx, step); err != nil { + return "", err + } + + doltSession := dsess.DSessFromSess(ctx.Session) + workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) + if err != nil { + return "", err + } + + err = processRebasePlanStep(ctx, &step, + workingSet.RebaseState().CommitBecomesEmptyHandling(), + workingSet.RebaseState().EmptyCommitHandling()) + if err != nil { + return "", err + } + } + + // Ensure a transaction has been started, so that the session is in sync with the latest changes + if doltSession.GetTransaction() == nil { + _, err = doltSession.StartTransaction(ctx, sql.ReadWrite) + if err != nil { + return "", err + } + } } // Update the branch being rebased to point to the same commit as our temporary working branch @@ -501,6 +665,11 @@ func continueRebase(ctx *sql.Context) (string, error) { return "", err } + // Start a new transaction so the session will see the changes to the branch pointer + if _, err = doltSession.StartTransaction(ctx, sql.ReadWrite); err != nil { + return "", err + } + // delete the temporary working branch dbData, ok = doltSession.GetDbData(ctx, ctx.GetCurrentDatabase()) if !ok { @@ -511,6 +680,48 @@ func continueRebase(ctx *sql.Context) (string, error) { }, doltSession.Provider(), nil) } +func commitManualChangesForStep(ctx *sql.Context, step rebase.RebasePlanStep) error { + doltSession := dsess.DSessFromSess(ctx.Session) + workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) + if err != nil { + panic(err) + } + + options, err := createCherryPickOptionsForRebaseStep(ctx, &step, workingSet.RebaseState().CommitBecomesEmptyHandling(), + workingSet.RebaseState().EmptyCommitHandling()) + + commitProps, err := cherry_pick.CreateCommitStagedPropsFromCherryPickOptions(ctx, *options) + if err != nil { + return err + } + + // If the commit message wasn't set when we created the cherry-pick options, then set it to the step's commit + // message. For fixup commits, we don't use their commit message, so we keep it empty, and let the amend commit + // codepath use the previous commit's message. + if commitProps.Message == "" && step.Action != rebase.RebaseActionFixup { + commitProps.Message = step.CommitMsg + } + + roots, ok := doltSession.GetRoots(ctx, ctx.GetCurrentDatabase()) + if !ok { + return fmt.Errorf("unable to get roots for current session") + } + roots.Staged = roots.Working + pendingCommit, err := doltSession.NewPendingCommit(ctx, ctx.GetCurrentDatabase(), roots, *commitProps) + if err != nil { + return err + } + + // Ensure a SQL transaction is set in the session + if doltSession.GetTransaction() == nil { + if _, err = doltSession.StartTransaction(ctx, sql.ReadWrite); err != nil { + return err + } + } + _, err = doltSession.DoltCommit(ctx, ctx.GetCurrentDatabase(), doltSession.GetTransaction(), pendingCommit) + return err +} + func processRebasePlanStep(ctx *sql.Context, planStep *rebase.RebasePlanStep, commitBecomesEmptyHandling doltdb.EmptyCommitHandling, emptyCommitHandling doltdb.EmptyCommitHandling) error { // Make sure we have a transaction opened for the session @@ -524,6 +735,20 @@ func processRebasePlanStep(ctx *sql.Context, planStep *rebase.RebasePlanStep, } } + // If the action is "drop", then we don't need to do anything + if planStep.Action == rebase.RebaseActionDrop { + return nil + } + + options, err := createCherryPickOptionsForRebaseStep(ctx, planStep, commitBecomesEmptyHandling, emptyCommitHandling) + if err != nil { + return err + } + + return handleRebaseCherryPick(ctx, planStep, *options) +} + +func createCherryPickOptionsForRebaseStep(ctx *sql.Context, planStep *rebase.RebasePlanStep, commitBecomesEmptyHandling doltdb.EmptyCommitHandling, emptyCommitHandling doltdb.EmptyCommitHandling) (*cherry_pick.CherryPickOptions, error) { // Override the default empty commit handling options for cherry-pick, since // rebase has slightly different defaults options := cherry_pick.NewCherryPickOptions() @@ -531,51 +756,67 @@ func processRebasePlanStep(ctx *sql.Context, planStep *rebase.RebasePlanStep, options.EmptyCommitHandling = emptyCommitHandling switch planStep.Action { - case rebase.RebaseActionDrop: - return nil + case rebase.RebaseActionDrop, rebase.RebaseActionPick: + // Nothing to do - case rebase.RebaseActionPick, rebase.RebaseActionReword: - if planStep.Action == rebase.RebaseActionReword { - options.CommitMessage = planStep.CommitMsg - } - return handleRebaseCherryPick(ctx, planStep.CommitHash, options) + case rebase.RebaseActionReword: + options.CommitMessage = planStep.CommitMsg - case rebase.RebaseActionSquash, rebase.RebaseActionFixup: + case rebase.RebaseActionSquash: options.Amend = true - if planStep.Action == rebase.RebaseActionSquash { - commitMessage, err := squashCommitMessage(ctx, planStep.CommitHash) - if err != nil { - return err - } - options.CommitMessage = commitMessage + commitMessage, err := squashCommitMessage(ctx, planStep.CommitHash) + if err != nil { + return nil, err } - return handleRebaseCherryPick(ctx, planStep.CommitHash, options) + options.CommitMessage = commitMessage + + case rebase.RebaseActionFixup: + options.Amend = true default: - return fmt.Errorf("rebase action '%s' is not supported", planStep.Action) + return nil, fmt.Errorf("rebase action '%s' is not supported", planStep.Action) } + + return &options, nil } // handleRebaseCherryPick runs a cherry-pick for the specified |commitHash|, using the specified -// cherry-pick |options| and checks the results for any errors or merge conflicts. The initial -// version of rebase doesn't support conflict resolution, so if any conflicts are detected, the -// rebase is aborted and an error is returned. -func handleRebaseCherryPick(ctx *sql.Context, commitHash string, options cherry_pick.CherryPickOptions) error { - _, mergeResult, err := cherry_pick.CherryPick(ctx, commitHash, options) +// cherry-pick |options| and checks the results for any errors or merge conflicts. If a data conflict +// is detected, then the ErrRebaseDataConflict error is returned. If a schema conflict is detected, +// then the ErrRebaseSchemaConflict error is returned. +func handleRebaseCherryPick(ctx *sql.Context, planStep *rebase.RebasePlanStep, options cherry_pick.CherryPickOptions) error { + _, mergeResult, err := cherry_pick.CherryPick(ctx, planStep.CommitHash, options) var schemaConflict merge.SchemaConflict isSchemaConflict := errors.As(err, &schemaConflict) - if (mergeResult != nil && mergeResult.HasMergeArtifacts()) || isSchemaConflict { - // TODO: rebase doesn't currently support conflict resolution, but ideally, when a conflict - // is detected, the rebase would be paused and the user would resolve the conflict just - // like any other conflict, and then call dolt_rebase --continue to keep going. - abortErr := abortRebase(ctx) - if abortErr != nil { - return ErrRebaseConflictWithAbortError.New(commitHash, abortErr) + if (mergeResult != nil && mergeResult.HasMergeArtifacts()) && !isSchemaConflict { + // Conflicts can't be resolved if @@autocommit is still enabled, so warn and abort the rebase + autocommitEnabled, err := isAutocommitEnabled(ctx) + if err != nil { + return err + } + if autocommitEnabled { + if abortErr := abortRebase(ctx); abortErr != nil { + return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) + } + return ErrRebaseDataConflictWithAutocommit.New() } - return ErrRebaseConflict.New(commitHash) + + // Otherwise, let the caller know about the conflict and how to resolve + return ErrRebaseDataConflict.New(planStep.CommitHash, planStep.CommitMsg) } + + // TODO: rebase doesn't support schema conflict resolution yet. Ideally, when a conflict is + // detected, the rebase would be paused and the user would resolve the conflict just + // like any other conflict, and then call dolt_rebase --continue to keep going. + if isSchemaConflict { + if abortErr := abortRebase(ctx); abortErr != nil { + return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) + } + return ErrRebaseSchemaConflict.New(planStep.CommitHash) + } + return err } @@ -632,3 +873,17 @@ func currentBranch(ctx *sql.Context) (string, error) { } return headRef.GetPath(), nil } + +// isAutocommitEnabled returns true if @@autocommit is enabled in the current session. +func isAutocommitEnabled(ctx *sql.Context) (bool, error) { + autocommitVal, err := ctx.GetSessionVariable(ctx, "autocommit") + if err != nil { + return false, err + } + autocommitBoolVal, _, err := types.Boolean.Convert(autocommitVal) + if err != nil { + return false, err + } + + return autocommitBoolVal == int8(1) || autocommitBoolVal == true, nil +} diff --git a/go/libraries/doltcore/sqle/dsess/session.go b/go/libraries/doltcore/sqle/dsess/session.go index 59d9a83eb64..9eda880fb5f 100644 --- a/go/libraries/doltcore/sqle/dsess/session.go +++ b/go/libraries/doltcore/sqle/dsess/session.go @@ -707,6 +707,31 @@ func (d *DoltSession) newPendingCommit(ctx *sql.Context, branchState *branchStat mergeParentCommits = append(mergeParentCommits, parentCommit) } + // If the commit message isn't set and we're amending the previous commit, + // go ahead and set the commit message from the current HEAD + if props.Message == "" && props.Amend { + cs, err := doltdb.NewCommitSpec("HEAD") + if err != nil { + return nil, err + } + + headRef, err := branchState.dbData.Rsr.CWBHeadRef() + if err != nil { + return nil, err + } + optCmt, err := branchState.dbData.Ddb.Resolve(ctx, cs, headRef) + commit, ok := optCmt.ToCommit() + if !ok { + return nil, doltdb.ErrGhostCommitEncountered + } + + meta, err := commit.GetCommitMeta(ctx) + if err != nil { + return nil, err + } + props.Message = meta.Description + } + // TODO: This is not the correct way to write this commit as an amend. While this commit is running // the branch head moves backwards and concurrency control here is not principled. newRoots, err := actions.ResetSoftToRef(ctx, branchState.dbData, "HEAD~1") diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go index d6565bf163c..12be7fd9a1a 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go @@ -62,16 +62,16 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ }, Assertions: []queries.ScriptTestAssertion{ { - Query: "call dolt_rebase('-i', 'main');", - ExpectedErrStr: dprocedures.ErrRebaseUncommittedChanges.Error(), + Query: "call dolt_rebase('-i', 'main');", + ExpectedErr: dprocedures.ErrRebaseUncommittedChanges, }, { Query: "call dolt_add('t');", Expected: []sql.Row{{0}}, }, { - Query: "call dolt_rebase('-i', 'main');", - ExpectedErrStr: dprocedures.ErrRebaseUncommittedChanges.Error(), + Query: "call dolt_rebase('-i', 'main');", + ExpectedErr: dprocedures.ErrRebaseUncommittedChanges, }, }, }, @@ -239,6 +239,133 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ }, }, }, + { + Name: "dolt_rebase errors: unresolved conflicts", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "insert into t values (1, 'one');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", + "update t set c1='uno' where pk=1;", + "call dolt_commit('-am', 'updating row 1 on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "delete from dolt_rebase where rebase_order=1;", + Expected: []sql.Row{{gmstypes.NewOkResult(1)}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"2", "pick", doltCommit, "updating row 1 on branch1"}, + }, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + Query: "select * from dolt_conflicts;", + Expected: []sql.Row{{"t", uint64(1)}}, + }, + { + // Trying to --continue a rebase when there are conflicts results in an error. + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUnresolvedConflicts, + }, + }, + }, + { + Name: "dolt_rebase errors: uncommitted changes before first --continue", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "insert into t values (1, 'one');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", + "update t set c1='uno' where pk=1;", + "call dolt_commit('-am', 'updating row 1 on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "insert into t values (100, 'hundo');", + Expected: []sql.Row{{gmstypes.NewOkResult(1)}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUncommittedChanges, + }, + }, + }, + { + Name: "dolt_rebase errors: data conflicts when @@autocommit is enabled", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "insert into t values (1, 'one');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", + "update t set c1='uno' where pk=1;", + "call dolt_commit('-am', 'updating row 1 on branch1');", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "update dolt_rebase set rebase_order=3 where rebase_order=1;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(1), Info: plan.UpdateInfo{ + Matched: 1, + Updated: 1, + }}}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"2", "pick", doltCommit, "updating row 1 on branch1"}, + {"3.00", "pick", doltCommit, "inserting row 1 on branch1"}, + }, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflictWithAutocommit, + }, + }, + }, { Name: "dolt_rebase: rebased commit becomes empty; --empty not specified", SetUpScript: []string{ @@ -537,6 +664,17 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Updated: 1, }}}}, }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"1", "pick", doltCommit, "inserting row 1"}, + {"2", "squash", doltCommit, "inserting row 10"}, + {"3", "squash", doltCommit, "inserting row 100"}, + {"4", "drop", doltCommit, "inserting row 1000"}, + {"5", "reword", doltCommit, "reworded!"}, + {"6.10", "fixup", doltCommit, "inserting row 100000"}, + }, + }, { Query: "call dolt_rebase('--continue');", Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, @@ -573,22 +711,22 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ }, }, { - Name: "dolt_rebase: data conflicts", + Name: "dolt_rebase: negative rebase order", SetUpScript: []string{ "create table t (pk int primary key, c1 varchar(100));", "call dolt_commit('-Am', 'creating table t');", "call dolt_branch('branch1');", "insert into t values (0, 'zero');", - "call dolt_commit('-am', 'inserting row 0');", + "call dolt_commit('-am', 'inserting row 0 on main');", "call dolt_checkout('branch1');", "insert into t values (1, 'one');", - "call dolt_commit('-am', 'inserting row 1');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", "update t set c1='uno' where pk=1;", - "call dolt_commit('-am', 'updating row 1');", + "call dolt_commit('-am', 'updating row 1 on branch1');", "update t set c1='ein' where pk=1;", - "call dolt_commit('-am', 'updating row 1');", + "call dolt_commit('-am', 'updating row 1, again, on branch1');", }, Assertions: []queries.ScriptTestAssertion{ { @@ -597,12 +735,79 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ "adjust the rebase plan in the dolt_rebase table, then " + "continue rebasing by calling dolt_rebase('--continue')"}}, }, + { + Query: "update dolt_rebase set rebase_order=rebase_order-12.34;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(3), Info: plan.UpdateInfo{ + Matched: 3, + Updated: 3, + }}}}, + }, { Query: "select * from dolt_rebase order by rebase_order ASC;", Expected: []sql.Row{ - {"1", "pick", doltCommit, "inserting row 1"}, - {"2", "pick", doltCommit, "updating row 1"}, - {"3", "pick", doltCommit, "updating row 1"}, + {"-11.34", "pick", doltCommit, "inserting row 1 on branch1"}, + {"-10.34", "pick", doltCommit, "updating row 1 on branch1"}, + {"-9.34", "pick", doltCommit, "updating row 1, again, on branch1"}, + }, + }, + { + Query: "call dolt_rebase('--continue');", + Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {1, "ein"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"updating row 1, again, on branch1"}, + {"updating row 1 on branch1"}, + {"inserting row 1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + }, + }, + { + Name: "dolt_rebase: data conflicts with pick", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "insert into t values (1, 'one');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", + "update t set c1='uno' where pk=1;", + "call dolt_commit('-am', 'updating row 1 on branch1');", + "update t set c1='ein' where pk=1;", + "call dolt_commit('-am', 'updating row 1, again, on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"1", "pick", doltCommit, "inserting row 1 on branch1"}, + {"2", "pick", doltCommit, "updating row 1 on branch1"}, + {"3", "pick", doltCommit, "updating row 1, again, on branch1"}, }, }, { @@ -613,24 +818,552 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ }}}}, }, { - // Encountering a conflict during a rebase returns an error and aborts the rebase Query: "call dolt_rebase('--continue');", - ExpectedErr: dprocedures.ErrRebaseConflict, + ExpectedErr: dprocedures.ErrRebaseDataConflict, }, { - // The rebase state has been cleared after hitting a conflict - Query: "call dolt_rebase('--continue');", - ExpectedErrStr: "no rebase in progress", + // We remain on the rebase working branch while resolving conflicts + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select * from dolt_conflicts;", + Expected: []sql.Row{{"t", uint64(1)}}, + }, + { + // The base of the cherry-picked commit has (1, "one"), but ours doesn't have that record (nil, nil) + // since we reordered the insert. The cherry-picked commit is trying to modify the row to (1, "uno"). + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{1, "one", nil, nil, "removed", 1, "uno", "modified"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Resolve conflicts, by accepting theirs, which inserts (1, "uno") into t + // When we continue the rebase, a Dolt commit will be created for these changes + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + // Our new commit shows up as the first commit on top of the latest commit from the upstream branch + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Table t includes the change from the tip of main (0, "zero"), as well as the conflict we just resolved + Query: "SELECT * FROM t;", + Expected: []sql.Row{{0, "zero"}, {1, "uno"}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {1, "ein"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"updating row 1, again, on branch1"}, + {"updating row 1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Now we're resolving a conflict from reordering the insert of (1, "one"). This was originally + // an insert, so the base has (nil, nil), ours is (1, "ein"). + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{nil, nil, 1, "ein", "added", 1, "one", "added"}}, + }, + { + // Accept the new values from the cherry-picked commit (1, "ein"). + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + // We can commit manually, or we can continue the rebase and let it commit for us + Query: "CALL DOLT_COMMIT('-am', 'OVERRIDDEN COMMIT MESSAGE');", + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "call dolt_rebase('--continue');", + Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {1, "one"}}, }, { - // We're back to the original branch Query: "select active_branch();", Expected: []sql.Row{{"branch1"}}, }, { - // The conflicts table should be empty, since the rebase was aborted - Query: "select * from dolt_conflicts;", - Expected: []sql.Row{}, + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"OVERRIDDEN COMMIT MESSAGE"}, + {"updating row 1, again, on branch1"}, + {"updating row 1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + }, + }, + { + Name: "dolt_rebase: data conflicts with squash", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "insert into t values (1, 'one');", + "call dolt_commit('-am', 'inserting row 1 on branch1');", + "update t set c1='uno' where pk=1;", + "call dolt_commit('-am', 'updating row 1 on branch1');", + "update t set c1='ein' where pk=1;", + "call dolt_commit('-am', 'updating row 1, again, on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"1", "pick", doltCommit, "inserting row 1 on branch1"}, + {"2", "pick", doltCommit, "updating row 1 on branch1"}, + {"3", "pick", doltCommit, "updating row 1, again, on branch1"}, + }, + }, + { + Query: "update dolt_rebase set rebase_order=3.5 where rebase_order=2;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(1), Info: plan.UpdateInfo{ + Matched: 1, + Updated: 1, + }}}}, + }, + { + Query: "update dolt_rebase set action='squash' where rebase_order=3;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(1), Info: plan.UpdateInfo{ + Matched: 1, + Updated: 1, + }}}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + // We remain on the rebase working branch while resolving conflicts + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select * from dolt_conflicts;", + Expected: []sql.Row{{"t", uint64(1)}}, + }, + { + // The base of the cherry-picked commit has (1, "uno"), but ours has (1, "one"), so this is a data + // conflict. The cherry-picked commit is trying to modify the row to (1, "ein"). + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{1, "uno", 1, "one", "modified", 1, "ein", "modified"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Resolve conflicts, by accepting theirs, which inserts (1, "ein") into t + // When we continue the rebase, a Dolt commit will be created for these changes + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + // Table t includes the change from the tip of main (0, "zero"), as well as the conflict we just resolved + Query: "SELECT * FROM t;", + Expected: []sql.Row{{0, "zero"}, {1, "ein"}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {1, "ein"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 1 on branch1\n\nupdating row 1, again, on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Now we're resolving a conflict from updating (1, "one") to (1, "uno"). Our side currently has + // (1, "ein"), so this is marked as a conflict. + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{1, "one", 1, "ein", "modified", 1, "uno", "modified"}}, + }, + { + // Accept the new values from the cherry-picked commit (1, "uno"). + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + Query: "CALL DOLT_COMMIT('-am', 'OVERRIDDEN COMMIT MESSAGE');", + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "call dolt_rebase('--continue');", + Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {1, "uno"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"OVERRIDDEN COMMIT MESSAGE"}, + {"inserting row 1 on branch1\n\nupdating row 1, again, on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + }, + }, + { + Name: "dolt_rebase: data conflicts with fixup", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "insert into t values (-1, 'negative');", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "update t set c1='-1' where pk=-1;", + "call dolt_commit('-am', 'updating row -1 on branch1');", + "delete from t where c1 = '-1';", + "call dolt_commit('-am', 'deleting -1 on branch1');", + "insert into t values (999, 'nines');", + "call dolt_commit('-am', 'inserting row 999 on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"1", "pick", doltCommit, "updating row -1 on branch1"}, + {"2", "pick", doltCommit, "deleting -1 on branch1"}, + {"3", "pick", doltCommit, "inserting row 999 on branch1"}, + }, + }, + { + Query: "update dolt_rebase set rebase_order=3.5, action='fixup' where rebase_order=1;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(1), Info: plan.UpdateInfo{ + Matched: 1, + Updated: 1, + }}}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"2", "pick", doltCommit, "deleting -1 on branch1"}, + {"3", "pick", doltCommit, "inserting row 999 on branch1"}, + {"3.50", "fixup", doltCommit, "updating row -1 on branch1"}, + }, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + // We remain on the rebase working branch while resolving conflicts + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select * from dolt_conflicts;", + Expected: []sql.Row{{"t", uint64(1)}}, + }, + { + // The base of the cherry-picked commit has (-1, "-1"), but ours has (-1, "negative"), so this is a + // data conflict. The cherry-picked commit is trying to delete the row, but can't find an exact match. + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{-1, "-1", -1, "negative", "modified", nil, nil, "removed"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Resolve conflicts, by accepting theirs, which inserts (1, "ein") into t + // When we continue the rebase, a Dolt commit will be created for these changes + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + Query: "SELECT * FROM t;", + Expected: []sql.Row{{0, "zero"}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {999, "nines"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 999 on branch1"}, + {"deleting -1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Now we're resolving a conflict where row -1 is updated, but it has already been deleted + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{-1, "negative", nil, nil, "removed", -1, "-1", "modified"}}, + }, + { + // Accept the new values from the cherry-picked commit (1, "uno"). + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + Query: "call dolt_rebase('--continue');", + Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{-1, "-1"}, {0, "zero"}, {999, "nines"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 999 on branch1"}, + {"deleting -1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + }, + }, + { + Name: "dolt_rebase: data conflicts with reword", + SetUpScript: []string{ + "create table t (pk int primary key, c1 varchar(100));", + "insert into t values (-1, 'negative');", + "call dolt_commit('-Am', 'creating table t');", + "call dolt_branch('branch1');", + "insert into t values (0, 'zero');", + "call dolt_commit('-am', 'inserting row 0 on main');", + + "call dolt_checkout('branch1');", + "update t set c1='-1' where pk=-1;", + "call dolt_commit('-am', 'updating row -1 on branch1');", + "delete from t where c1 = '-1';", + "call dolt_commit('-am', 'deleting -1 on branch1');", + "insert into t values (999, 'nines');", + "call dolt_commit('-am', 'inserting row 999 on branch1');", + + "set @@autocommit=0;", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_rebase('-i', 'main');", + Expected: []sql.Row{{0, "interactive rebase started on branch dolt_rebase_branch1; " + + "adjust the rebase plan in the dolt_rebase table, then " + + "continue rebasing by calling dolt_rebase('--continue')"}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"1", "pick", doltCommit, "updating row -1 on branch1"}, + {"2", "pick", doltCommit, "deleting -1 on branch1"}, + {"3", "pick", doltCommit, "inserting row 999 on branch1"}, + }, + }, + { + Query: "update dolt_rebase set rebase_order=3.5, action='reword', commit_message='reworded message!' where rebase_order=1;", + Expected: []sql.Row{{gmstypes.OkResult{RowsAffected: uint64(1), Info: plan.UpdateInfo{ + Matched: 1, + Updated: 1, + }}}}, + }, + { + Query: "select * from dolt_rebase order by rebase_order ASC;", + Expected: []sql.Row{ + {"2", "pick", doltCommit, "deleting -1 on branch1"}, + {"3", "pick", doltCommit, "inserting row 999 on branch1"}, + {"3.50", "reword", doltCommit, "reworded message!"}, + }, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + // We remain on the rebase working branch while resolving conflicts + Query: "select active_branch();", + Expected: []sql.Row{{"dolt_rebase_branch1"}}, + }, + { + Query: "select * from dolt_conflicts;", + Expected: []sql.Row{{"t", uint64(1)}}, + }, + { + // The base of the cherry-picked commit has (-1, "-1"), but ours has (-1, "negative"), so this is a + // data conflict. The cherry-picked commit is trying to delete the row, but can't find an exact match. + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{-1, "-1", -1, "negative", "modified", nil, nil, "removed"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Resolve conflicts, by accepting theirs, which inserts (1, "ein") into t + // When we continue the rebase, a Dolt commit will be created for these changes + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + Query: "SELECT * FROM t;", + Expected: []sql.Row{{0, "zero"}}, + }, + { + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseDataConflict, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{0, "zero"}, {999, "nines"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"inserting row 999 on branch1"}, + {"deleting -1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, + }, + { + // Now we're resolving a conflict where row -1 is updated, but it has already been deleted + Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", + Expected: []sql.Row{{-1, "negative", nil, nil, "removed", -1, "-1", "modified"}}, + }, + { + // Accept the new values from the cherry-picked commit (-1, "-1") + Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", + Expected: []sql.Row{{0}}, + }, + { + Query: "call dolt_rebase('--continue');", + Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, + }, + { + Query: "select * from t;", + Expected: []sql.Row{{-1, "-1"}, {0, "zero"}, {999, "nines"}}, + }, + { + Query: "select active_branch();", + Expected: []sql.Row{{"branch1"}}, + }, + { + Query: "select message from dolt_log;", + Expected: []sql.Row{ + {"reworded message!"}, + {"inserting row 999 on branch1"}, + {"deleting -1 on branch1"}, + {"inserting row 0 on main"}, + {"creating table t"}, + {"Initialize data repository"}, + }, }, }, }, @@ -677,7 +1410,7 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ { // Encountering a conflict during a rebase returns an error and aborts the rebase Query: "call dolt_rebase('--continue');", - ExpectedErr: dprocedures.ErrRebaseConflict, + ExpectedErr: dprocedures.ErrRebaseSchemaConflict, }, { // The rebase state has been cleared after hitting a conflict @@ -749,6 +1482,8 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Expected: []sql.Row{{gmstypes.NewOkResult(2)}}, }, { + // NOTE: This uses "pick", not reword, so we expect the commit message from the commit to be + // used, and not the custom commit message inserted into the table. Query: "insert into dolt_rebase values (2.12, 'pick', hashof('branch2'), 'inserting row 0');", Expected: []sql.Row{{gmstypes.NewOkResult(1)}}, }, diff --git a/go/serial/workingset.fbs b/go/serial/workingset.fbs index cee89b20a62..84b5ee75304 100644 --- a/go/serial/workingset.fbs +++ b/go/serial/workingset.fbs @@ -58,6 +58,15 @@ table RebaseState { // How to handle commits that become empty during rebasing commit_becomes_empty_handling:uint8; + + // The last_attempted_step field indicates which step in the rebase plan was last executed. The step may have ended + // with a data conflict that the user has to manually resolve, or it may have been cherry-picked cleanly. This field + // is not valid unless the rebasing_started field is set to true. + last_attempted_step:float; + + // The rebasing_started field indicates if execution of the rebase plan has been started or not. Once execution of the + // plan has been started, the last_attempted_step field holds a reference to the most recent plan step attempted. + rebasing_started:bool; } // KEEP THIS IN SYNC WITH fileidentifiers.go diff --git a/go/store/datas/dataset.go b/go/store/datas/dataset.go index 5f09a085597..4867c2c37ec 100644 --- a/go/store/datas/dataset.go +++ b/go/store/datas/dataset.go @@ -167,6 +167,8 @@ type RebaseState struct { branch string commitBecomesEmptyHandling uint8 emptyCommitHandling uint8 + lastAttemptedStep float32 + rebasingStarted bool } func (rs *RebaseState) PreRebaseWorkingAddr() hash.Hash { @@ -188,6 +190,14 @@ func (rs *RebaseState) OntoCommit(ctx context.Context, vr types.ValueReader) (*C return nil, nil } +func (rs *RebaseState) LastAttemptedStep(_ context.Context) float32 { + return rs.lastAttemptedStep +} + +func (rs *RebaseState) RebasingStarted(_ context.Context) bool { + return rs.rebasingStarted +} + func (rs *RebaseState) CommitBecomesEmptyHandling(_ context.Context) uint8 { return rs.commitBecomesEmptyHandling } @@ -446,6 +456,8 @@ func (h serialWorkingSetHead) HeadWorkingSet() (*WorkingSetHead, error) { string(rebaseState.BranchBytes()), rebaseState.CommitBecomesEmptyHandling(), rebaseState.EmptyCommitHandling(), + rebaseState.LastAttemptedStep(), + rebaseState.RebasingStarted(), ) } diff --git a/go/store/datas/workingset.go b/go/store/datas/workingset.go index ac4e77ed6b4..eb578ad8d2a 100755 --- a/go/store/datas/workingset.go +++ b/go/store/datas/workingset.go @@ -194,6 +194,8 @@ func workingset_flatbuffer(working hash.Hash, staged *hash.Hash, mergeState *Mer serial.RebaseStateAddOntoCommitAddr(builder, ontoAddrOffset) serial.RebaseStateAddCommitBecomesEmptyHandling(builder, rebaseState.commitBecomesEmptyHandling) serial.RebaseStateAddEmptyCommitHandling(builder, rebaseState.emptyCommitHandling) + serial.RebaseStateAddLastAttemptedStep(builder, rebaseState.lastAttemptedStep) + serial.RebaseStateAddRebasingStarted(builder, rebaseState.rebasingStarted) rebaseStateOffset = serial.RebaseStateEnd(builder) } @@ -262,13 +264,15 @@ func NewMergeState( } } -func NewRebaseState(preRebaseWorkingRoot hash.Hash, commitAddr hash.Hash, branch string, commitBecomesEmptyHandling uint8, emptyCommitHandling uint8) *RebaseState { +func NewRebaseState(preRebaseWorkingRoot hash.Hash, commitAddr hash.Hash, branch string, commitBecomesEmptyHandling uint8, emptyCommitHandling uint8, lastAttemptedStep float32, rebasingStarted bool) *RebaseState { return &RebaseState{ preRebaseWorkingAddr: &preRebaseWorkingRoot, ontoCommitAddr: &commitAddr, branch: branch, commitBecomesEmptyHandling: commitBecomesEmptyHandling, emptyCommitHandling: emptyCommitHandling, + lastAttemptedStep: lastAttemptedStep, + rebasingStarted: rebasingStarted, } } From 6eb7325b6b4600b39f9b48d80e34029058d8347c Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Tue, 27 Aug 2024 11:15:05 -0700 Subject: [PATCH 2/5] Fixing up the rebase CLI command to support resolving data conflicts --- go/cmd/dolt/commands/rebase.go | 132 ++++++++++++------ .../doltcore/sqle/dprocedures/dolt_rebase.go | 84 +++++++++-- .../sqle/enginetest/dolt_queries_rebase.go | 2 +- integration-tests/bats/rebase.bats | 83 ++++++++++- 4 files changed, 234 insertions(+), 67 deletions(-) diff --git a/go/cmd/dolt/commands/rebase.go b/go/cmd/dolt/commands/rebase.go index cc56d1137af..2c10c14b2da 100644 --- a/go/cmd/dolt/commands/rebase.go +++ b/go/cmd/dolt/commands/rebase.go @@ -31,6 +31,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/env" "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" "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/libraries/utils/config" "github.com/dolthub/dolt/go/libraries/utils/editor" @@ -95,6 +96,12 @@ func (cmd RebaseCmd) Exec(ctx context.Context, commandStr string, args []string, defer closeFunc() } + // Set @@dolt_allow_commit_conflicts in case there are data conflicts that need to be resolved by the caller. + // Without this, the conflicts can't be committed to the branch working set, and the caller can't access them. + if _, err = GetRowsForSql(queryist, sqlCtx, "set @@dolt_allow_commit_conflicts=1;"); err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + branchName, err := getActiveBranchName(sqlCtx, queryist) if err != nil { return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) @@ -118,65 +125,84 @@ func (cmd RebaseCmd) Exec(ctx context.Context, commandStr string, args []string, return HandleVErrAndExitCode(errhand.VerboseErrorFromError(errors.New("error: "+rows[0][1].(string))), usage) } + // If the rebase was successful, or if it was aborted, print out the message and + // ensure the branch being rebased is checked out in the CLI message := rows[0][1].(string) - if strings.Contains(message, dprocedures.SuccessfulRebaseMessage) { - cli.Println(dprocedures.SuccessfulRebaseMessage + branchName) - } else if strings.Contains(message, dprocedures.RebaseAbortedMessage) { - cli.Println(dprocedures.RebaseAbortedMessage) - } else { - rebasePlan, err := getRebasePlan(cliCtx, sqlCtx, queryist, apr.Arg(0), branchName) + if strings.Contains(message, dprocedures.SuccessfulRebaseMessage) || + strings.Contains(message, dprocedures.RebaseAbortedMessage) { + cli.Println(message) + if err = syncCliBranchToSqlSessionBranch(sqlCtx, dEnv); err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + return 0 + } + + rebasePlan, err := getRebasePlan(cliCtx, sqlCtx, queryist, apr.Arg(0), branchName) + if err != nil { + // attempt to abort the rebase + _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + + // if all uncommented lines are deleted in the editor, abort the rebase + if rebasePlan == nil || rebasePlan.Steps == nil || len(rebasePlan.Steps) == 0 { + rows, err := GetRowsForSql(queryist, sqlCtx, "CALL DOLT_REBASE('--abort');") + if err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + status, err := getInt64ColAsInt64(rows[0][0]) if err != nil { - // attempt to abort the rebase - _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) } + if status == 1 { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(errors.New("error: "+rows[0][1].(string))), usage) + } - // if all uncommented lines are deleted in the editor, abort the rebase - if rebasePlan == nil || rebasePlan.Steps == nil || len(rebasePlan.Steps) == 0 { - rows, err := GetRowsForSql(queryist, sqlCtx, "CALL DOLT_REBASE('--abort');") - if err != nil { - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) - } - status, err := getInt64ColAsInt64(rows[0][0]) - if err != nil { - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) - } - if status == 1 { - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(errors.New("error: "+rows[0][1].(string))), usage) - } + cli.Println(dprocedures.RebaseAbortedMessage) + return 0 + } - cli.Println(dprocedures.RebaseAbortedMessage) - } else { - err = insertRebasePlanIntoDoltRebaseTable(rebasePlan, sqlCtx, queryist) - if err != nil { - // attempt to abort the rebase - _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) - } + err = insertRebasePlanIntoDoltRebaseTable(rebasePlan, sqlCtx, queryist) + if err != nil { + // attempt to abort the rebase + _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } - rows, err := GetRowsForSql(queryist, sqlCtx, "CALL DOLT_REBASE('--continue');") - if err != nil { - // attempt to abort the rebase - _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) - } - status, err := getInt64ColAsInt64(rows[0][0]) - if err != nil { - // attempt to abort the rebase - _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + rows, err = GetRowsForSql(queryist, sqlCtx, "CALL DOLT_REBASE('--continue');") + if err != nil { + // If the error is a data conflict, don't abort the rebase, but let the caller resolve the conflicts + if dprocedures.ErrRebaseDataConflict.Is(err) || strings.Contains(err.Error(), dprocedures.ErrRebaseDataConflict.Message[:40]) { + if checkoutErr := syncCliBranchToSqlSessionBranch(sqlCtx, dEnv); checkoutErr != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(checkoutErr), usage) } - if status == 1 { - // attempt to abort the rebase - _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") - return HandleVErrAndExitCode(errhand.VerboseErrorFromError(errors.New("error: "+rows[0][1].(string))), usage) + } else { + _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") + if checkoutErr := syncCliBranchToSqlSessionBranch(sqlCtx, dEnv); checkoutErr != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(checkoutErr), usage) } + } + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } - cli.Println(dprocedures.SuccessfulRebaseMessage + branchName) + status, err = getInt64ColAsInt64(rows[0][0]) + if err != nil { + _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") + if err = syncCliBranchToSqlSessionBranch(sqlCtx, dEnv); err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) } + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + if status == 1 { + _, _, _, _ = queryist.Query(sqlCtx, "CALL DOLT_REBASE('--abort');") + if err = syncCliBranchToSqlSessionBranch(sqlCtx, dEnv); err != nil { + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage) + } + return HandleVErrAndExitCode(errhand.VerboseErrorFromError(errors.New("error: "+rows[0][1].(string))), usage) } - return HandleVErrAndExitCode(nil, usage) + cli.Println(rows[0][1].(string)) + return 0 } // getRebasePlan opens an editor for users to edit the rebase plan and returns the parsed rebase plan from the editor. @@ -313,3 +339,17 @@ func insertRebasePlanIntoDoltRebaseTable(plan *rebase.RebasePlan, sqlCtx *sql.Co return nil } + +// syncCliBranchToSqlSessionBranch sets the current branch for the CLI (in repo_state.json) to the active branch +// for the current session. This is needed during rebasing, since any conflicts need to be resolved while the +// session is on the rebase working branch (e.g. dolt_rebase_t1) and after the rebase finishes, the session needs +// to be back on the branch being rebased (e.g. t1). +func syncCliBranchToSqlSessionBranch(ctx *sql.Context, dEnv *env.DoltEnv) error { + doltSession := dsess.DSessFromSess(ctx.Session) + currentBranch, err := doltSession.GetBranch() + if err != nil { + return err + } + + return saveHeadBranch(dEnv.FS, currentBranch) +} diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index b791e266380..34ac05ddd79 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -88,12 +88,13 @@ var ErrRebaseUncommittedChanges = goerrors.NewKind("cannot start a rebase with u var ErrRebaseUnresolvedConflicts = goerrors.NewKind( "conflicts detected in tables %s; resolve conflicts before continuing the rebase") -// ErrRebaseDataConflictWithAutocommit is used when data conflicts are detected in a rebase, but @@autocommit has not -// been disabled, so it's not possible to resolve the conflicts since they would get rolled back automatically. -var ErrRebaseDataConflictWithAutocommit = goerrors.NewKind( - "data conflicts from rebase, but @@autocommit has not been disabled. " + - "@@autocommit must be disabled to resolve conflicts. The rebase has been aborted. " + - "Set @@autocommit to 0 and try the rebase again to resolve the conflicts.") +// ErrRebaseDataConflictsCantBeResolved is used when data conflicts are detected in a rebase, but @@autocommit has not +// been disabled or @@dolt_allow_commit_conflicts has not been enabled, so it's not possible to resolve the conflicts +// since they would get rolled back automatically. +var ErrRebaseDataConflictsCantBeResolved = goerrors.NewKind( + "data conflicts from rebase, but session settings do not allow preserving conflicts, so they cannot be resolved. " + + "The rebase has been aborted. Set @@autocommit to 0 or set @@dolt_allow_commit_conflicts to 1 and " + + "try the rebase again to resolve the conflicts.") // ErrRebaseDataConflict is used when a data conflict is detected while rebasing a commit. var ErrRebaseDataConflict = goerrors.NewKind("data conflict detected while rebasing commit %s (%s). \n\n" + @@ -790,17 +791,32 @@ func handleRebaseCherryPick(ctx *sql.Context, planStep *rebase.RebasePlanStep, o var schemaConflict merge.SchemaConflict isSchemaConflict := errors.As(err, &schemaConflict) + doltSession := dsess.DSessFromSess(ctx.Session) if (mergeResult != nil && mergeResult.HasMergeArtifacts()) && !isSchemaConflict { - // Conflicts can't be resolved if @@autocommit is still enabled, so warn and abort the rebase - autocommitEnabled, err := isAutocommitEnabled(ctx) + if err := validateConflictsCanBeResolved(ctx, planStep); err != nil { + return err + } + + // If @@dolt_allow_commit_conflicts is enabled, then we need to make a SQL commit here, which + // will save the conflicts and make them visible to other sessions. This is necessary for the CLI's + // rebase command support. However, it's also valid to use @@autocommit and resolve the data + // conflicts within the same session, so in that case, we do NOT make a SQL commit. + allowCommitConflictsEnabled, err := isAllowCommitConflictsEnabled(ctx) if err != nil { return err } - if autocommitEnabled { - if abortErr := abortRebase(ctx); abortErr != nil { - return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) + if allowCommitConflictsEnabled { + if doltSession.GetTransaction() == nil { + _, err := doltSession.StartTransaction(ctx, sql.ReadWrite) + if err != nil { + return err + } + } + + err = doltSession.CommitTransaction(ctx, doltSession.GetTransaction()) + if err != nil { + return err } - return ErrRebaseDataConflictWithAutocommit.New() } // Otherwise, let the caller know about the conflict and how to resolve @@ -874,16 +890,54 @@ func currentBranch(ctx *sql.Context) (string, error) { return headRef.GetPath(), nil } +// validateConflictsCanBeResolved checks to see if conflicts can be recorded in the current session, by looking to see +// if @@autocommit is disabled, or if @@dolt_allow_commit_conflicts has been enabled. If conflicts cannot be recorded +// then the current rebase is aborted, and an error is returned describing how to change the session settings and +// restart the rebase. +func validateConflictsCanBeResolved(ctx *sql.Context, planStep *rebase.RebasePlanStep) error { + autocommitEnabled, err := isAutocommitEnabled(ctx) + if err != nil { + return err + } + + allowCommitConflictsEnabled, err := isAllowCommitConflictsEnabled(ctx) + if err != nil { + return err + } + + // If @@autocommit is turned off, or if @@dolt_allow_commit_conflicts is enabled, then the user will + // be able to resolve conflicts in the session. + if !autocommitEnabled || allowCommitConflictsEnabled { + return nil + } + + // Otherwise, abort the rebase and return an error message, since conflicts can't be worked with. + if abortErr := abortRebase(ctx); abortErr != nil { + return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) + } + return ErrRebaseDataConflictsCantBeResolved.New() +} + // isAutocommitEnabled returns true if @@autocommit is enabled in the current session. func isAutocommitEnabled(ctx *sql.Context) (bool, error) { - autocommitVal, err := ctx.GetSessionVariable(ctx, "autocommit") + return lookupBoolSysVar(ctx, "autocommit") +} + +// isAllowCommitConflictsEnabled returns true if @@dolt_allow_commit_conflicts is enabled in the current session. +func isAllowCommitConflictsEnabled(ctx *sql.Context) (bool, error) { + return lookupBoolSysVar(ctx, "dolt_allow_commit_conflicts") +} + +// lookupBoolSysVar returns the value of the boolean system variable named |systemVarName| for the current session. +func lookupBoolSysVar(ctx *sql.Context, systemVarName string) (bool, error) { + value, err := ctx.GetSessionVariable(ctx, systemVarName) if err != nil { return false, err } - autocommitBoolVal, _, err := types.Boolean.Convert(autocommitVal) + boolValue, _, err := types.Boolean.Convert(value) if err != nil { return false, err } - return autocommitBoolVal == int8(1) || autocommitBoolVal == true, nil + return boolValue == int8(1) || boolValue == true, nil } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go index 12be7fd9a1a..b4c5a9c4c73 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go @@ -362,7 +362,7 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ }, { Query: "call dolt_rebase('--continue');", - ExpectedErr: dprocedures.ErrRebaseDataConflictWithAutocommit, + ExpectedErr: dprocedures.ErrRebaseDataConflictsCantBeResolved, }, }, }, diff --git a/integration-tests/bats/rebase.bats b/integration-tests/bats/rebase.bats index e0b35a1803f..c66019d6ad7 100755 --- a/integration-tests/bats/rebase.bats +++ b/integration-tests/bats/rebase.bats @@ -336,7 +336,7 @@ setupCustomEditorScript() { ! [[ "$output" =~ "b1 merge commit" ]] || false } -@test "rebase: rebase with data conflicts aborts" { +@test "rebase: rebase with data conflict" { setupCustomEditorScript dolt checkout b1 @@ -345,15 +345,88 @@ setupCustomEditorScript() { run dolt rebase -i main [ "$status" -eq 1 ] - [[ "$output" =~ "merge conflict detected while rebasing commit" ]] || false - [[ "$output" =~ "the rebase has been automatically aborted" ]] || false + [[ "$output" =~ "data conflict detected while rebasing commit" ]] || false + [[ "$output" =~ "b1 commit 2" ]] || false + + # Assert that we are on the rebase working branch (not the branch being rebased) + run dolt sql -q "select active_branch();" + [ "$status" -eq 0 ] + [[ "$output" =~ " dolt_rebase_b1 " ]] || false + + # Review and resolve the data conflict + run dolt conflicts cat . + [ "$status" -eq 0 ] + [[ "$output" =~ "+ | ours | 1 | 1 | NULL | NULL | NULL | NULL |" ]] || false + [[ "$output" =~ "+ | theirs | 1 | 2 | NULL | NULL | NULL | NULL |" ]] || false + dolt conflicts resolve --theirs t1 run dolt rebase --continue + [ "$status" -eq 0 ] + [[ "$output" =~ "Successfully rebased and updated refs/heads/b1" ]] || false + + # Assert that we are back on the branch being rebased + run dolt branch + [ "$status" -eq 0 ] + [[ "$output" =~ "b1" ]] || false + ! [[ "$output" =~ "dolt_rebase_b1" ]] || false +} + +@test "rebase: rebase with multiple data conflicts" { + setupCustomEditorScript + dolt sql -q "INSERT INTO t1 VALUES (2,200);" + dolt commit -am "main commit 3" + + dolt checkout b1 + dolt sql -q "INSERT INTO t1 VALUES (1,2);" + dolt commit -am "b1 commit 2" + dolt sql -q "INSERT INTO t1 VALUES (2,3);" + dolt commit -am "b1 commit 3" + + # Start the rebase + run dolt rebase -i main [ "$status" -eq 1 ] - [[ "$output" =~ "no rebase in progress" ]] || false + [[ "$output" =~ "data conflict detected while rebasing commit" ]] || false + [[ "$output" =~ "b1 commit 2" ]] || false + + # Assert that we are on the rebase working branch now (not the branch being rebased) + run dolt sql -q "select active_branch();" + [ "$status" -eq 0 ] + [[ "$output" =~ " dolt_rebase_b1 " ]] || false + + # Review and resolve the first data conflict + run dolt conflicts cat . + [ "$status" -eq 0 ] + [[ "$output" =~ "+ | ours | 1 | 1 | NULL | NULL | NULL | NULL |" ]] || false + [[ "$output" =~ "+ | theirs | 1 | 2 | NULL | NULL | NULL | NULL |" ]] || false + dolt conflicts resolve --theirs t1 + + # Continue the rebase and hit the second data conflict + run dolt rebase --continue + [ "$status" -eq 1 ] + [[ "$output" =~ "data conflict detected while rebasing commit" ]] || false + [[ "$output" =~ "b1 commit 3" ]] || false + + # Assert that we are still on the rebase working branch + run dolt sql -q "select active_branch();" + [ "$status" -eq 0 ] + [[ "$output" =~ " dolt_rebase_b1 " ]] || false + + # Review and resolve the second data conflict + run dolt conflicts cat . + [ "$status" -eq 0 ] + [[ "$output" =~ "+ | ours | 2 | 200 | NULL | NULL | NULL | NULL |" ]] || false + [[ "$output" =~ "+ | theirs | 2 | 3 | NULL | NULL | NULL | NULL |" ]] || false + dolt conflicts resolve --ours t1 + + # Finish the rebase + run dolt rebase --continue + [ "$status" -eq 0 ] + [[ "$output" =~ "Successfully rebased and updated refs/heads/b1" ]] || false + # Assert that we are back on the branch that was rebased, and that the rebase working branch is gone run dolt branch [ "$status" -eq 0 ] + [[ "$output" =~ "b1" ]] || false ! [[ "$output" =~ "dolt_rebase_b1" ]] || false } @@ -366,7 +439,7 @@ setupCustomEditorScript() { run dolt rebase -i main [ "$status" -eq 1 ] - [[ "$output" =~ "merge conflict detected while rebasing commit" ]] || false + [[ "$output" =~ "schema conflict detected while rebasing commit" ]] || false [[ "$output" =~ "the rebase has been automatically aborted" ]] || false run dolt rebase --continue From 5a7c8489be462a82c96d2f1f8eff73f9579c2172 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 28 Aug 2024 10:12:56 -0700 Subject: [PATCH 3/5] PR Feedback --- .../doltcore/sqle/dprocedures/dolt_rebase.go | 12 ++++++++---- integration-tests/bats/rebase.bats | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index 34ac05ddd79..cbc8203f6ca 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -512,12 +512,15 @@ func isWorkingSetClean(ctx *sql.Context) (bool, error) { return true, nil } +// recordCurrentStep updates working set metadata to record the current rebase plan step number as well +// as the rebase started flag indicating that execution of the rebase plan has been started. This +// information is all stored in the RebaseState of the WorkingSet. func recordCurrentStep(ctx *sql.Context, step rebase.RebasePlanStep) error { doltSession := dsess.DSessFromSess(ctx.Session) if doltSession.GetTransaction() == nil { _, err := doltSession.StartTransaction(ctx, sql.ReadWrite) if err != nil { - panic(err) + return err } } @@ -539,7 +542,7 @@ func recordCurrentStep(ctx *sql.Context, step rebase.RebasePlanStep) error { if doltSession.GetTransaction() != nil { err = doltSession.CommitTransaction(ctx, doltSession.GetTransaction()) if err != nil { - panic(err) + return err } } @@ -685,7 +688,7 @@ func commitManualChangesForStep(ctx *sql.Context, step rebase.RebasePlanStep) er doltSession := dsess.DSessFromSess(ctx.Session) workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) if err != nil { - panic(err) + return err } options, err := createCherryPickOptionsForRebaseStep(ctx, &step, workingSet.RebaseState().CommitBecomesEmptyHandling(), @@ -758,7 +761,8 @@ func createCherryPickOptionsForRebaseStep(ctx *sql.Context, planStep *rebase.Reb switch planStep.Action { case rebase.RebaseActionDrop, rebase.RebaseActionPick: - // Nothing to do + // Nothing to do – the drop action doesn't result in a cherry pick and the pick action + // doesn't require any special options (i.e. no amend, no custom commit message). case rebase.RebaseActionReword: options.CommitMessage = planStep.CommitMsg diff --git a/integration-tests/bats/rebase.bats b/integration-tests/bats/rebase.bats index c66019d6ad7..6033e24e7a2 100755 --- a/integration-tests/bats/rebase.bats +++ b/integration-tests/bats/rebase.bats @@ -367,7 +367,7 @@ setupCustomEditorScript() { # Assert that we are back on the branch being rebased run dolt branch [ "$status" -eq 0 ] - [[ "$output" =~ "b1" ]] || false + [[ "$output" =~ "* b1" ]] || false ! [[ "$output" =~ "dolt_rebase_b1" ]] || false } From 0372eb34bec7719d547c6f7cd93a0e1eedd46a4b Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 28 Aug 2024 10:23:00 -0700 Subject: [PATCH 4/5] PR Feedback --- .../doltcore/sqle/dprocedures/dolt_rebase.go | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index cbc8203f6ca..0a3ae19e6e6 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -792,11 +792,19 @@ func createCherryPickOptionsForRebaseStep(ctx *sql.Context, planStep *rebase.Reb func handleRebaseCherryPick(ctx *sql.Context, planStep *rebase.RebasePlanStep, options cherry_pick.CherryPickOptions) error { _, mergeResult, err := cherry_pick.CherryPick(ctx, planStep.CommitHash, options) + // TODO: rebase doesn't support schema conflict resolution yet. Ideally, when a schema conflict + // is detected, the rebase would be paused and the user would resolve the conflict just + // like any other conflict, and then call dolt_rebase --continue to keep going. var schemaConflict merge.SchemaConflict - isSchemaConflict := errors.As(err, &schemaConflict) + if errors.As(err, &schemaConflict) { + if abortErr := abortRebase(ctx); abortErr != nil { + return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) + } + return ErrRebaseSchemaConflict.New(planStep.CommitHash) + } doltSession := dsess.DSessFromSess(ctx.Session) - if (mergeResult != nil && mergeResult.HasMergeArtifacts()) && !isSchemaConflict { + if mergeResult != nil && mergeResult.HasMergeArtifacts() { if err := validateConflictsCanBeResolved(ctx, planStep); err != nil { return err } @@ -827,16 +835,6 @@ func handleRebaseCherryPick(ctx *sql.Context, planStep *rebase.RebasePlanStep, o return ErrRebaseDataConflict.New(planStep.CommitHash, planStep.CommitMsg) } - // TODO: rebase doesn't support schema conflict resolution yet. Ideally, when a conflict is - // detected, the rebase would be paused and the user would resolve the conflict just - // like any other conflict, and then call dolt_rebase --continue to keep going. - if isSchemaConflict { - if abortErr := abortRebase(ctx); abortErr != nil { - return ErrRebaseConflictWithAbortError.New(planStep.CommitHash, abortErr) - } - return ErrRebaseSchemaConflict.New(planStep.CommitHash) - } - return err } From 8a49cd8f807c5e35ceb127358446488c54e4f7ad Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 28 Aug 2024 16:16:21 -0700 Subject: [PATCH 5/5] PR Feedback: Changing to require that all tables are staged when a rebase is continued after manually resolving a conflict --- .../doltcore/sqle/dprocedures/dolt_rebase.go | 92 +++++++++++++++---- .../sqle/enginetest/dolt_queries_rebase.go | 48 +++++++++- integration-tests/bats/rebase.bats | 10 +- 3 files changed, 127 insertions(+), 23 deletions(-) diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go index 0a3ae19e6e6..1aa38f4ee6b 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go @@ -83,6 +83,11 @@ var DoltRebaseSystemTableSchema = []*sql.Column{ // ignored) changes in the working set. var ErrRebaseUncommittedChanges = goerrors.NewKind("cannot start a rebase with uncommitted changes") +// ErrRebaseUnstagedChanges is used when a rebase is continued, but there are unstaged changes +// in the working set. +var ErrRebaseUnstagedChanges = goerrors.NewKind("cannot continue a rebase with unstaged changes. " + + "Use dolt_add() to stage tables and then continue the rebase") + // ErrRebaseUnresolvedConflicts is used when a rebase is continued, but there are // unresolved conflicts still present. var ErrRebaseUnresolvedConflicts = goerrors.NewKind( @@ -492,24 +497,53 @@ func loadRebasePlan(ctx *sql.Context) (*rebase.RebasePlan, error) { return rebasePlan, nil } -// isWorkingSetClean returns true if the working set for the current session doesn't contain any staged or -// working changes, other than any changes to ignored tables. -func isWorkingSetClean(ctx *sql.Context) (bool, error) { +// filterIgnoredTables iterates over the specified |unstagedTableDeltas|, filters out any TableDeltas for +// ignored tables, and returns the filtered list. +func filterIgnoredTables(ctx *sql.Context, unstagedTableDeltas []diff.TableDelta) ([]diff.TableDelta, error) { doltSession := dsess.DSessFromSess(ctx.Session) roots, ok := doltSession.GetRoots(ctx, ctx.GetCurrentDatabase()) if !ok { - return false, fmt.Errorf("unable to get roots for current session") + return nil, fmt.Errorf("unable to get roots for current session") } - wsOnlyHasIgnoredTables, err := diff.WorkingSetContainsOnlyIgnoredTables(ctx, roots) + filteredTableDeltas := make([]diff.TableDelta, 0, len(unstagedTableDeltas)) + ignorePatterns, err := doltdb.GetIgnoredTablePatterns(ctx, roots) if err != nil { - return false, err + return nil, err } - if !wsOnlyHasIgnoredTables { - return false, nil + + for _, tableDelta := range unstagedTableDeltas { + isIgnored, err := ignorePatterns.IsTableNameIgnored(tableDelta.ToName) + if err != nil { + return nil, err + } + if isIgnored != doltdb.Ignore { + filteredTableDeltas = append(filteredTableDeltas, tableDelta) + } + } + + return filteredTableDeltas, nil +} + +// workingSetStatus checks the workspace status and returns whether there are any staged changes and unstaged changes. +func workingSetStatus(ctx *sql.Context) (stagedChanges, unstagedChanges bool, err error) { + doltSession := dsess.DSessFromSess(ctx.Session) + roots, ok := doltSession.GetRoots(ctx, ctx.GetCurrentDatabase()) + if !ok { + return false, false, fmt.Errorf("unable to get roots for current session") + } + + stagedTables, unstagedTables, err := diff.GetStagedUnstagedTableDeltas(ctx, roots) + if err != nil { + return false, false, err + } + + unstagedTables, err = filterIgnoredTables(ctx, unstagedTables) + if err != nil { + return false, false, err } - return true, nil + return len(stagedTables) > 0, len(unstagedTables) > 0, nil } // recordCurrentStep updates working set metadata to record the current rebase plan step number as well @@ -560,12 +594,24 @@ func continueRebase(ctx *sql.Context) (string, error) { return "", err } + // After we've checked for conflicts in the working set, commit the SQL transaction to ensure + // our session is in sync with the latest changes on the branch + doltSession := dsess.DSessFromSess(ctx.Session) + if doltSession.GetTransaction() == nil { + _, err := doltSession.StartTransaction(ctx, sql.ReadWrite) + if err != nil { + return "", err + } + } + if err := doltSession.CommitTransaction(ctx, doltSession.GetTransaction()); err != nil { + return "", err + } + rebasePlan, err := loadRebasePlan(ctx) if err != nil { return "", err } - doltSession := dsess.DSessFromSess(ctx.Session) for _, step := range rebasePlan.Steps { workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) if err != nil { @@ -576,12 +622,14 @@ func continueRebase(ctx *sql.Context) (string, error) { lastAttemptedStep := workingSet.RebaseState().LastAttemptedStep() rebasingStarted := workingSet.RebaseState().RebasingStarted() - changesCommited, err := isWorkingSetClean(ctx) + hasStagedChanges, hasUnstagedChanges, err := workingSetStatus(ctx) if err != nil { return "", err } - if !rebasingStarted && !changesCommited { + // If the rebase is just starting but there are any uncommitted changes in the working set, + // tell the user they need to commit them before we can start executing the rebase plan. + if !rebasingStarted && (hasStagedChanges || hasUnstagedChanges) { return "", ErrRebaseUncommittedChanges.New() } @@ -590,10 +638,16 @@ func continueRebase(ctx *sql.Context) (string, error) { continue } - if rebasingStarted && rebaseStepOrder == lastAttemptedStep && !changesCommited { - // If we've already executed this step, but the working set has changes, then we need - // to make the commit for the manual changes made for this step - if err = commitManualChangesForStep(ctx, step); err != nil { + // If the rebase is continued, but not all working set changes are staged, then tell the user + // they need to explicitly stage the tables before the rebase can be continued. + if hasUnstagedChanges { + return "", ErrRebaseUnstagedChanges.New() + } + + // If we've already executed this step, but the working set has staged changes, + // then we need to make the commit for the manual changes made for this step. + if rebasingStarted && rebaseStepOrder == lastAttemptedStep && hasStagedChanges { + if err = commitManuallyStagedChangesForStep(ctx, step); err != nil { return "", err } continue @@ -684,7 +738,10 @@ func continueRebase(ctx *sql.Context) (string, error) { }, doltSession.Provider(), nil) } -func commitManualChangesForStep(ctx *sql.Context, step rebase.RebasePlanStep) error { +// commitManuallyStagedChangesForStep handles committing staged changes after a conflict has been manually +// resolved by the caller before rebasing has been continued. This involves building the correct commit +// message based on the details of the rebase plan |step| and then creating the commit. +func commitManuallyStagedChangesForStep(ctx *sql.Context, step rebase.RebasePlanStep) error { doltSession := dsess.DSessFromSess(ctx.Session) workingSet, err := doltSession.WorkingSet(ctx, ctx.GetCurrentDatabase()) if err != nil { @@ -710,7 +767,6 @@ func commitManualChangesForStep(ctx *sql.Context, step rebase.RebasePlanStep) er if !ok { return fmt.Errorf("unable to get roots for current session") } - roots.Staged = roots.Working pendingCommit, err := doltSession.NewPendingCommit(ctx, ctx.GetCurrentDatabase(), roots, *commitProps) if err != nil { return err diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go index b4c5a9c4c73..bc5b7f0a6e0 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_rebase.go @@ -868,6 +868,15 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "SELECT * FROM t;", Expected: []sql.Row{{0, "zero"}, {1, "uno"}}, }, + { + // If we don't stage the table, then rebase will give us an error about having unstaged changes + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUnstagedChanges, + }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", ExpectedErr: dprocedures.ErrRebaseDataConflict, @@ -999,10 +1008,6 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "select base_pk, base_c1, our_pk, our_c1, our_diff_type, their_pk, their_c1, their_diff_type from dolt_conflicts_t;", Expected: []sql.Row{{1, "uno", 1, "one", "modified", 1, "ein", "modified"}}, }, - { - Query: "select active_branch();", - Expected: []sql.Row{{"dolt_rebase_branch1"}}, - }, { Query: "select message from dolt_log;", Expected: []sql.Row{ @@ -1023,6 +1028,15 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "SELECT * FROM t;", Expected: []sql.Row{{0, "zero"}, {1, "ein"}}, }, + { + // If we don't stage the table, then rebase will give us an error about having unstaged changes + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUnstagedChanges, + }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", ExpectedErr: dprocedures.ErrRebaseDataConflict, @@ -1171,6 +1185,15 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "SELECT * FROM t;", Expected: []sql.Row{{0, "zero"}}, }, + { + // If we don't stage the table, then rebase will give us an error about having unstaged changes + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUnstagedChanges, + }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", ExpectedErr: dprocedures.ErrRebaseDataConflict, @@ -1203,6 +1226,10 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", Expected: []sql.Row{{0}}, }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, @@ -1314,6 +1341,15 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "SELECT * FROM t;", Expected: []sql.Row{{0, "zero"}}, }, + { + // If we don't stage the table, then rebase will give us an error about having unstaged changes + Query: "call dolt_rebase('--continue');", + ExpectedErr: dprocedures.ErrRebaseUnstagedChanges, + }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", ExpectedErr: dprocedures.ErrRebaseDataConflict, @@ -1342,6 +1378,10 @@ var DoltRebaseScriptTests = []queries.ScriptTest{ Query: "CALL DOLT_CONFLICTS_RESOLVE('--theirs', 't');", Expected: []sql.Row{{0}}, }, + { + Query: "call dolt_add('t');", + Expected: []sql.Row{{0}}, + }, { Query: "call dolt_rebase('--continue');", Expected: []sql.Row{{0, "Successfully rebased and updated refs/heads/branch1"}}, diff --git a/integration-tests/bats/rebase.bats b/integration-tests/bats/rebase.bats index 6033e24e7a2..e43cf5d616f 100755 --- a/integration-tests/bats/rebase.bats +++ b/integration-tests/bats/rebase.bats @@ -359,6 +359,7 @@ setupCustomEditorScript() { [[ "$output" =~ "+ | ours | 1 | 1 | NULL | NULL | NULL | NULL |" ]] || false [[ "$output" =~ "+ | theirs | 1 | 2 | NULL | NULL | NULL | NULL |" ]] || false dolt conflicts resolve --theirs t1 + dolt add t1 run dolt rebase --continue [ "$status" -eq 0 ] @@ -400,7 +401,13 @@ setupCustomEditorScript() { [[ "$output" =~ "+ | theirs | 1 | 2 | NULL | NULL | NULL | NULL |" ]] || false dolt conflicts resolve --theirs t1 - # Continue the rebase and hit the second data conflict + # Without staging the changed tables, trying to continue the rebase results in an error + run dolt rebase --continue + [ "$status" -eq 1 ] + [[ "$output" =~ "cannot continue a rebase with unstaged changes" ]] || false + + # Stage the tables and then continue the rebase and hit the second data conflict + dolt add t1 run dolt rebase --continue [ "$status" -eq 1 ] [[ "$output" =~ "data conflict detected while rebasing commit" ]] || false @@ -419,6 +426,7 @@ setupCustomEditorScript() { dolt conflicts resolve --ours t1 # Finish the rebase + dolt add t1 run dolt rebase --continue [ "$status" -eq 0 ] [[ "$output" =~ "Successfully rebased and updated refs/heads/b1" ]] || false