diff --git a/go/cmd/dolt/commands/admin/createchunk/createcommit.go b/go/cmd/dolt/commands/admin/createchunk/createcommit.go index ddf63087f19..9c5e107b016 100644 --- a/go/cmd/dolt/commands/admin/createchunk/createcommit.go +++ b/go/cmd/dolt/commands/admin/createchunk/createcommit.go @@ -125,11 +125,11 @@ func (cmd CreateCommitCmd) Exec(ctx context.Context, commandStr string, args []s return 1 } - // This isn't technically an amend, but the Amend field controls whether the commit must be a child of the ref's current commit (if any) + // The Force field controls whether the commit must be a child of the ref's current commit (if any). commitOpts := datas.CommitOptions{ Parents: parentCommits, Meta: commitMeta, - Amend: force, + Force: force, } rootVal, err := db.ValueReadWriter().ReadValue(ctx, commitRootHash) diff --git a/go/cmd/dolt/commands/schcmds/copy-tags.go b/go/cmd/dolt/commands/schcmds/copy-tags.go index 7eb4778330a..69ee5b9e597 100644 --- a/go/cmd/dolt/commands/schcmds/copy-tags.go +++ b/go/cmd/dolt/commands/schcmds/copy-tags.go @@ -32,6 +32,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess" "github.com/dolthub/dolt/go/libraries/utils/argparser" "github.com/dolthub/dolt/go/store/datas" + "github.com/dolthub/dolt/go/store/hash" ) var copyTagsDocs = cli.CommandDocumentationContent{ @@ -277,7 +278,7 @@ func doltCommitUpdatedTags(ctx *sql.Context, tableResolver doltdb.TableResolver, return err } - pendingCommit, err := actions.GetCommitStaged(ctx, tableResolver, roots, workingSet, nil, doltDB, commitStagedProps) + pendingCommit, err := actions.GetCommitStaged(ctx, tableResolver, roots, workingSet, nil, hash.Hash{}, doltDB, commitStagedProps) if err != nil { return err } diff --git a/go/libraries/doltcore/doltdb/commit.go b/go/libraries/doltcore/doltdb/commit.go index c14671ef828..f76d1adb95c 100644 --- a/go/libraries/doltcore/doltdb/commit.go +++ b/go/libraries/doltcore/doltdb/commit.go @@ -294,14 +294,14 @@ type PendingCommit struct { // commit, once written. // |headRef| is the ref of the HEAD the commit will update // |mergeParentCommits| are any merge parents for this commit -// |amend| is a flag which indicates that additional parents should not be added to the provided |mergeParentCommits|. +// |amendedCommit|, when not empty, is the address of an existing commit that this commit amends and replaces. // |cm| is the metadata for the commit // The current branch head will be automatically filled in as the first parent at commit time. func (ddb *DoltDB) NewPendingCommit( ctx context.Context, roots Roots, mergeParentCommits []*Commit, - amend bool, + amendedCommit hash.Hash, cm *datas.CommitMeta, ) (*PendingCommit, error) { newstaged, val, err := ddb.writeRootValue(ctx, roots.Staged) @@ -315,7 +315,7 @@ func (ddb *DoltDB) NewPendingCommit( parents = append(parents, pc.dCommit.Addr()) } - commitOpts := datas.CommitOptions{Parents: parents, Meta: cm, Amend: amend} + commitOpts := datas.CommitOptions{Parents: parents, Meta: cm, AmendedCommit: amendedCommit} return &PendingCommit{ Roots: roots, Val: val, diff --git a/go/libraries/doltcore/doltdb/workingset.go b/go/libraries/doltcore/doltdb/workingset.go index a7be54ae62d..3563bc194ef 100644 --- a/go/libraries/doltcore/doltdb/workingset.go +++ b/go/libraries/doltcore/doltdb/workingset.go @@ -200,6 +200,19 @@ func (m MergeState) IsRevert() bool { return m.isRevert } +// OperationName returns the user facing name of the operation this merge state belongs to, one of "merge", +// "cherry-pick", or "revert". +func (m MergeState) OperationName() string { + switch { + case m.isCherryPick: + return "cherry-pick" + case m.isRevert: + return "revert" + default: + return "merge" + } +} + func (m MergeState) PreMergeWorkingRoot() RootValue { return m.preMergeWorking } diff --git a/go/libraries/doltcore/dtestutils/testcommands/multienv.go b/go/libraries/doltcore/dtestutils/testcommands/multienv.go index ea66aa50254..8ed2c53de0e 100644 --- a/go/libraries/doltcore/dtestutils/testcommands/multienv.go +++ b/go/libraries/doltcore/dtestutils/testcommands/multienv.go @@ -36,6 +36,7 @@ import ( "github.com/dolthub/dolt/go/libraries/utils/filesys" "github.com/dolthub/dolt/go/store/datas" "github.com/dolthub/dolt/go/store/datas/pull" + "github.com/dolthub/dolt/go/store/hash" "github.com/dolthub/dolt/go/store/types" ) @@ -276,7 +277,7 @@ func (mr *MultiRepoTestSetup) CommitWithWorkingSet(dbName string) *doltdb.Commit Author: ident, Committer: ident, } - pendingCommit, err := actions.GetCommitStaged(ctx, doltdb.SimpleTableResolver{}, roots, ws, mergeParentCommits, dEnv.DbData(ctx).Ddb, commitStagedProps) + pendingCommit, err := actions.GetCommitStaged(ctx, doltdb.SimpleTableResolver{}, roots, ws, mergeParentCommits, hash.Hash{}, dEnv.DbData(ctx).Ddb, commitStagedProps) if err != nil { panic("pending commit error: " + err.Error()) } diff --git a/go/libraries/doltcore/env/actions/commit.go b/go/libraries/doltcore/env/actions/commit.go index ae7929f9029..03487cdd9ed 100644 --- a/go/libraries/doltcore/env/actions/commit.go +++ b/go/libraries/doltcore/env/actions/commit.go @@ -26,6 +26,7 @@ import ( "github.com/dolthub/dolt/go/libraries/doltcore/diff" "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" "github.com/dolthub/dolt/go/store/datas" + "github.com/dolthub/dolt/go/store/hash" ) const CommitVerificationFailedPrefix = "commit verification failed:" @@ -73,13 +74,15 @@ func getCommitRunTestGroups() []string { return nil } -// GetCommitStaged returns a new pending commit with the roots and commit properties given. +// GetCommitStaged returns a new pending commit with the roots and commit properties given. When props.Amend is +// set, |amendedCommit| must be the address of the commit being amended. func GetCommitStaged( ctx *sql.Context, tableResolver doltdb.TableResolver, roots doltdb.Roots, ws *doltdb.WorkingSet, mergeParents []*doltdb.Commit, + amendedCommit hash.Hash, db *doltdb.DoltDB, props CommitStagedProps, ) (*doltdb.PendingCommit, error) { @@ -187,7 +190,7 @@ func GetCommitStaged( return nil, err } - return db.NewPendingCommit(ctx, roots, mergeParents, props.Amend, commitMeta) + return db.NewPendingCommit(ctx, roots, mergeParents, amendedCommit, commitMeta) } // runCommitVerification runs the commit verification tests for the given test groups. diff --git a/go/libraries/doltcore/sqle/dprocedures/dolt_commit.go b/go/libraries/doltcore/sqle/dprocedures/dolt_commit.go index 00d064481f3..cfc9e83db74 100644 --- a/go/libraries/doltcore/sqle/dprocedures/dolt_commit.go +++ b/go/libraries/doltcore/sqle/dprocedures/dolt_commit.go @@ -111,6 +111,15 @@ func doDoltCommit(ctx *sql.Context, args []string) (string, bool, error) { msg, msgOk := apr.GetValue(cli.MessageArg) amend := apr.Contains(cli.AmendFlag) + if amend { + ws, err := dSess.WorkingSet(ctx, dbName) + if err != nil { + return "", false, err + } + if ws.MergeActive() { + return "", false, fmt.Errorf("you are in the middle of a %s -- cannot amend", ws.MergeState().OperationName()) + } + } if !msgOk { if amend { commit, err := dSess.GetHeadCommit(ctx, dbName) diff --git a/go/libraries/doltcore/sqle/dsess/session.go b/go/libraries/doltcore/sqle/dsess/session.go index 05dcb58f012..0d356e9b139 100644 --- a/go/libraries/doltcore/sqle/dsess/session.go +++ b/go/libraries/doltcore/sqle/dsess/session.go @@ -773,11 +773,20 @@ func (d *DoltSession) newPendingCommit(ctx *sql.Context, dbName string, branchSt } } + var amendedCommit hash.Hash + if props.Amend { + var err error + amendedCommit, err = headCommit.HashOf() + if err != nil { + return nil, err + } + } + tableResolver, err := GetTableResolver(ctx, dbName) if err != nil { return nil, err } - pendingCommit, err := actions.GetCommitStaged(ctx, tableResolver, roots, branchState.WorkingSet(), mergeParentCommits, branchState.dbData.Ddb, props) + pendingCommit, err := actions.GetCommitStaged(ctx, tableResolver, roots, branchState.WorkingSet(), mergeParentCommits, amendedCommit, branchState.dbData.Ddb, props) if err != nil { // Special case for nothing staged, which is not an error if _, ok := err.(actions.NothingStaged); !ok { diff --git a/go/libraries/doltcore/sqle/dsess/transactions.go b/go/libraries/doltcore/sqle/dsess/transactions.go index 6bbd667bd3c..7b6c2be7275 100644 --- a/go/libraries/doltcore/sqle/dsess/transactions.go +++ b/go/libraries/doltcore/sqle/dsess/transactions.go @@ -283,6 +283,11 @@ func doltCommit(ctx *sql.Context, var rsc doltdb.ReplicationStatusController newCommit, err := doltDb.CommitWithWorkingSet(ctx, headRef, workingSet.Ref(), &pending, workingSet, currHash, tx.WorkingSetMeta(name, email), &rsc) WaitForReplicationController(ctx, rsc) + // The check in doCommit can go stale before the ref update, so the storage layer compares the head against + // AmendedCommit once more, atomically with the update. A failure there surfaces the same way. + if err != nil && !pending.CommitOptions.AmendedCommit.IsEmpty() && errors.Is(err, datas.ErrMergeNeeded) { + return nil, nil, tx.rollbackAndErr(ctx, retryTransactionError(err.Error())) + } return workingSet, newCommit, err } @@ -451,6 +456,12 @@ func (tx *DoltTransaction) doCommit( return nil, nil, err } + // Checked before the working set merge so that a stale amend reports the moved head rather + // than a data conflict. + if err := tx.validateAmendedHead(ctx, startPoint.db, workingSet, commit); err != nil { + return nil, nil, err + } + if newWorkingSet || workingAndStagedEqual(existingWs, startState) { // ff merge err = tx.validateWorkingSetForCommit(ctx, workingSet, isFfMerge) @@ -558,6 +569,51 @@ func (tx *DoltTransaction) rollback(ctx *sql.Context) error { return nil } +// rollbackAndErr rolls the transaction back and returns |err|, or the rollback error when rolling back fails. +func (tx *DoltTransaction) rollbackAndErr(ctx *sql.Context, err error) error { + if rollbackErr := tx.rollback(ctx); rollbackErr != nil { + return rollbackErr + } + return err +} + +// retryTransactionError returns the client facing serialization failure for a transaction that conflicts with a +// committed transaction. A non empty |detail| is placed before the retry advice. +func retryTransactionError(detail string) error { + if detail == "" { + return sql.ErrLockDeadlock.New(ErrRetryTransaction.Error()) + } + return sql.ErrLockDeadlock.New(fmt.Sprintf("%s: %s", detail, ErrRetryTransaction.Error())) +} + +// validateAmendedHead returns a retryable error when |pending| amends a commit that is no longer the head of the +// branch that |workingSet| belongs to. The transaction is rolled back before the error is returned. A nil +// |pending| or an ordinary commit passes without any check. +func (tx *DoltTransaction) validateAmendedHead(ctx *sql.Context, doltDb *doltdb.DoltDB, workingSet *doltdb.WorkingSet, pending *doltdb.PendingCommit) error { + if pending == nil { + return nil + } + amended := pending.CommitOptions.AmendedCommit + if amended.IsEmpty() { + return nil + } + + headRef, err := workingSet.Ref().ToHeadRef() + if err != nil { + return err + } + curHeadAddr, err := doltDb.GetHashForRefStr(ctx, headRef.String()) + if err != nil { + return err + } + if *curHeadAddr != amended { + return tx.rollbackAndErr(ctx, retryTransactionError(fmt.Sprintf( + "cannot amend head of branch '%s': is at %s but expected %s", + headRef.GetPath(), *curHeadAddr, amended))) + } + return nil +} + type ffMerge bool const ( @@ -624,12 +680,7 @@ func (tx *DoltTransaction) validateWorkingSetForCommit(ctx *sql.Context, working // Conflicts are never acceptable when they resulted from a merge with the existing working set -- it's equivalent // to hitting a write lock (which we didn't take). Always roll back and return an error in this case. if !isFf { - rollbackErr := tx.rollback(ctx) - if rollbackErr != nil { - return rollbackErr - } - - return sql.ErrLockDeadlock.New(ErrRetryTransaction.Error()) + return tx.rollbackAndErr(ctx, retryTransactionError("")) } // If there were conflicts before merge with the persisted working set, whether we allow it to be committed is a @@ -769,12 +820,7 @@ func (tx *DoltTransaction) validateWorkingSetForCommit(ctx *sql.Context, working } } - rollbackErr := tx.rollback(ctx) - if rollbackErr != nil { - return rollbackErr - } - - return fmt.Errorf("%s%s%s", ErrUnresolvedConstraintViolationsCommit, ConstraintViolationsListPrefix, messageBuilder.String()) + return tx.rollbackAndErr(ctx, fmt.Errorf("%s%s%s", ErrUnresolvedConstraintViolationsCommit, ConstraintViolationsListPrefix, messageBuilder.String())) } } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go index 70b30d754e3..e5e454ee179 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries.go @@ -8262,6 +8262,54 @@ var DoltCommitTests = []queries.ScriptTest{ }, }, }, + { + // See https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---amend + Name: "CALL DOLT_COMMIT with --amend fails while a merge or cherry-pick is in progress", + SetUpScript: []string{ + "drop table if exists invalidFK", + "set @@dolt_allow_commit_conflicts = 1", + "create table amend_merge_t (pk int primary key, c int)", + "insert into amend_merge_t values (1, 1)", + "call dolt_commit('-Am', 'base')", + "call dolt_checkout('-b', 'other')", + "update amend_merge_t set c = 2 where pk = 1", + "call dolt_commit('-am', 'other change')", + "call dolt_checkout('main')", + "update amend_merge_t set c = 3 where pk = 1", + "call dolt_commit('-am', 'main change')", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "call dolt_merge('other')", + Expected: []sql.Row{{"", 0, 1, "conflicts found"}}, + }, + { + Query: "call dolt_commit('-a', '--amend', '-m', 'amend during merge')", + ExpectedErrStr: "you are in the middle of a merge -- cannot amend", + }, + { + Query: "select count(*) from dolt_conflicts_amend_merge_t", + // The merge state and its conflicts are untouched by the rejected amend + Expected: []sql.Row{{1}}, + }, + { + Query: "call dolt_merge('--abort')", + Expected: []sql.Row{{"", 0, 0, "merge aborted"}}, + }, + { + Query: "call dolt_cherry_pick(dolt_hashof('other'))", + Expected: []sql.Row{{"", 1, 0, 0}}, + }, + { + Query: "call dolt_commit('-a', '--amend', '-m', 'amend during cherry-pick')", + ExpectedErrStr: "you are in the middle of a cherry-pick -- cannot amend", + }, + { + Query: "call dolt_cherry_pick('--abort')", + Expected: []sql.Row{{"", 0, 0, 0}}, + }, + }, + }, } var DoltIndexPrefixScripts = []queries.ScriptTest{ diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go index fd4e49c28b1..e4a2301a010 100755 --- a/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go @@ -2108,6 +2108,244 @@ var DoltStoredProcedureTransactionTests = []queries.TransactionTest{ }, }, }, + // See https://github.com/dolthub/dolt/issues/9072 + { + Name: "amend commit in transaction fails when another client merges into the branch", + SetUpScript: []string{ + "create table test (pk int primary key, val varchar(30))", + "call dolt_add('.')", + "insert into test values (1, 'initial'), (2, 'second')", + "call dolt_commit('-a', '-m', 'initial commit')", + "call dolt_checkout('-b', 'side')", + "insert into test values (100, 'side work')", + "call dolt_commit('-a', '-m', 'side work')", + "call dolt_checkout('main')", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "/* client a */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ update test set val = 'stale change' where pk = 1", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client b */ call dolt_merge('--no-ff', 'side')", + Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}}, + }, + { + Query: "/* client a */ select hashof('HEAD') = hashof('main')", + // The transaction still sees its snapshot head even though the branch ref has moved + Expected: []sql.Row{{false}}, + }, + { + Query: "/* client a */ call dolt_commit('-a', '--amend', '-m', 'stale amend')", + ExpectedErr: sql.ErrLockDeadlock, + }, + { + Query: "/* client a */ rollback", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ select message from dolt_log limit 1", + // The merge commit survives as the branch head + Expected: []sql.Row{{"Merge branch 'side' into main"}}, + }, + { + Query: "/* client a */ select count(*) from dolt_log where message = 'stale amend'", + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client a */ select val from test where pk = 1", + Expected: []sql.Row{{"initial"}}, + }, + { + Query: "/* client a */ select val from test where pk = 100", + Expected: []sql.Row{{"side work"}}, + }, + }, + }, + { + Name: "amend commit in transaction fails when another client moves the branch head", + SetUpScript: []string{ + "create table test (pk int primary key, val varchar(30))", + "call dolt_add('.')", + "insert into test values (1, 'initial'), (2, 'second')", + "call dolt_commit('-a', '-m', 'initial commit')", + }, + Assertions: []queries.ScriptTestAssertion{ + // Another client moves the head with a plain commit + { + Query: "/* client a */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ update test set val = 'stale change' where pk = 1", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client b */ insert into test values (3, 'concurrent row')", + Expected: []sql.Row{{types.NewOkResult(1)}}, + }, + { + Query: "/* client b */ call dolt_commit('-a', '-m', 'concurrent commit')", + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "/* client a */ select hashof('HEAD') = hashof('main')", + // The transaction still sees its snapshot head even though the branch ref has moved + Expected: []sql.Row{{false}}, + }, + { + Query: "/* client a */ call dolt_commit('-a', '--amend', '-m', 'stale amend')", + ExpectedErr: sql.ErrLockDeadlock, + }, + // Another client moves the head with a commit that leaves the root data identical + { + Query: "/* client a */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ update test set val = 'stale change' where pk = 1", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client b */ call dolt_commit('--allow-empty', '-m', 'empty head move')", + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "/* client a */ call dolt_commit('-a', '--amend', '-m', 'stale amend')", + ExpectedErr: sql.ErrLockDeadlock, + }, + // Another client amends the head first, touching only rows this transaction never changed + { + Query: "/* client a */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ update test set val = 'stale change' where pk = 1", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client b */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client b */ update test set val = 'concurrent amend' where pk = 2", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client b */ call dolt_commit('-a', '--amend', '-m', 'concurrent amend')", + // This amend is legal because the branch head has not moved since client b began + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "/* client a */ call dolt_commit('-a', '--amend', '-m', 'stale amend')", + ExpectedErr: sql.ErrLockDeadlock, + }, + { + Query: "/* client a */ rollback", + Expected: []sql.Row{}, + }, + // Every rejected amend left the branch history untouched + { + Query: "/* client a */ select message from dolt_log limit 1", + Expected: []sql.Row{{"concurrent amend"}}, + }, + { + Query: "/* client a */ select count(*) from dolt_log where message = 'stale amend'", + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client a */ select count(*) from dolt_log where message = 'empty head move'", + // Client b's legal amend replaced the empty commit + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client a */ select val from test where pk = 1", + Expected: []sql.Row{{"initial"}}, + }, + { + Query: "/* client a */ select val from test where pk = 2", + Expected: []sql.Row{{"concurrent amend"}}, + }, + }, + }, + { + Name: "amend commit tolerates ignored table changes from both clients", + SetUpScript: []string{ + "create table test (pk int primary key, val varchar(30))", + "insert into dolt_ignore values ('ignored_*', 1)", + "create table ignored_t (pk int primary key)", + "insert into test values (1, 'initial')", + "call dolt_commit('-Am', 'initial commit')", + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "/* client a */ start transaction", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ update test set val = 'amend me' where pk = 1", + Expected: []sql.Row{{queries.NewUpdateResult(1, 1)}}, + }, + { + Query: "/* client a */ insert into ignored_t values (2)", + Expected: []sql.Row{{types.NewOkResult(1)}}, + }, + { + Query: "/* client a */ create table ignored_a (pk int primary key)", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "/* client b */ insert into ignored_t values (1)", + Expected: []sql.Row{{types.NewOkResult(1)}}, + }, + { + Query: "/* client b */ create table ignored_b (pk int primary key)", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "/* client a */ select hashof('HEAD') = hashof('main')", + // Client b's ignored table writes created no commit, so the branch head has not moved + Expected: []sql.Row{{true}}, + }, + { + Query: "/* client a */ call dolt_commit('-a', '--amend', '-m', 'amended')", + Expected: []sql.Row{{doltCommit}}, + }, + { + Query: "/* client a */ select message from dolt_log limit 1", + Expected: []sql.Row{{"amended"}}, + }, + { + Query: "/* client a */ select pk from ignored_t order by pk", + // Both clients' uncommitted ignored table rows survive the amend + Expected: []sql.Row{{1}, {2}}, + }, + { + Query: "/* client a */ select val from test as of hashof('HEAD')", + // The amended commit contains the staged data, not only the new message + Expected: []sql.Row{{"amend me"}}, + }, + { + Query: "/* client a */ select table_name from dolt_status_ignored where ignored = 1 order by table_name", + // Both clients' new ignored tables and the shared one remain classified as ignored + Expected: []sql.Row{{"ignored_a"}, {"ignored_b"}, {"ignored_t"}}, + }, + { + Query: "/* client a */ select count(*) from dolt_status", + // The ignored table leaves nothing to commit + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client a */ show tables as of hashof('HEAD')", + // The ignored table is not part of the amended commit + Expected: []sql.Row{{"test"}}, + }, + }, + }, } var DoltConstraintViolationTransactionTests = []queries.TransactionTest{ diff --git a/go/store/datas/commit_options.go b/go/store/datas/commit_options.go index 85852950a33..3b03d90bcea 100644 --- a/go/store/datas/commit_options.go +++ b/go/store/datas/commit_options.go @@ -41,10 +41,13 @@ type CommitOptions struct { // creating. If it is empty, the existing dataset head will be the only // parent. Parents []hash.Hash - // Amend flag indicates that the commit being build it to amend an existing commit. Generally we add the branch HEAD - // as a parent, in addition to the parent set provided here. When we amend, we want to strictly use the commits - // provided in |Parents|, and no others. - Amend bool + // AmendedCommit, when not empty, is the address of an existing commit that the commit being built amends and + // replaces. The branch HEAD is then not added as a parent and only the commits provided in |Parents| are + // used. The commit is rejected with [ErrMergeNeeded] unless this address is still the dataset head. + AmendedCommit hash.Hash + // Force skips the check that the dataset head is among the new commit's parents. It is intended for + // administrative tooling and must not be combined with AmendedCommit. + Force bool // Signer, when non-nil, is called to sign the commit payload. DBName, HeadHash, and StagedHash // must also be set when Signer is provided. Signer CommitSigner diff --git a/go/store/datas/database_common.go b/go/store/datas/database_common.go index b71b51950b0..bfe27f99ebb 100644 --- a/go/store/datas/database_common.go +++ b/go/store/datas/database_common.go @@ -499,17 +499,28 @@ func (db *database) doFastForward(ctx context.Context, ds Dataset, newHeadAddr h return err } +// BuildNewCommit creates a new commit for the dataset with the value and options given. An ordinary commit must +// have the current dataset head among its parents. An amend commit must name the current dataset head in +// [CommitOptions.AmendedCommit]. A force commit skips head validation. func (db *database) BuildNewCommit(ctx context.Context, ds Dataset, v types.Value, opts CommitOptions) (*Commit, error) { - if !opts.Amend { - headAddr, ok := ds.MaybeHeadAddr() - if ok { - if len(opts.Parents) == 0 { - opts.Parents = []hash.Hash{headAddr} - } else { - if !hasParentHash(opts, headAddr) { - return nil, ErrMergeNeeded - } - } + if opts.Force && !opts.AmendedCommit.IsEmpty() { + return nil, errors.New("datas: the Force and AmendedCommit commit options are mutually exclusive") + } + headAddr, hasHead := ds.MaybeHeadAddr() + if !opts.AmendedCommit.IsEmpty() { + if !hasHead { + return nil, fmt.Errorf("cannot amend head of dataset '%s': dataset has no head: %w", + ds.ID(), ErrMergeNeeded) + } + if headAddr != opts.AmendedCommit { + return nil, fmt.Errorf("cannot amend head of dataset '%s': is at %s but expected %s: %w", + ds.ID(), headAddr, opts.AmendedCommit, ErrMergeNeeded) + } + } else if hasHead && !opts.Force { + if len(opts.Parents) == 0 { + opts.Parents = []hash.Hash{headAddr} + } else if !hasParentHash(opts, headAddr) { + return nil, ErrMergeNeeded } } @@ -733,7 +744,7 @@ func (db *database) CommitWithWorkingSet( // Prepend the current head hash to the list of parents if one was provided. This is only necessary if parents were // provided because we fill it in automatically in buildNewCommit otherwise. - if len(opts.Parents) > 0 && !opts.Amend { + if len(opts.Parents) > 0 && opts.AmendedCommit.IsEmpty() && !opts.Force { headHash, ok := commitDS.MaybeHeadAddr() if ok { if !hasParentHash(opts, headHash) { diff --git a/go/store/datas/database_test.go b/go/store/datas/database_test.go index 1d4a4ff1f54..81866c93957 100644 --- a/go/store/datas/database_test.go +++ b/go/store/datas/database_test.go @@ -254,6 +254,73 @@ func (suite *DatabaseSuite) TestDatabaseDuplicateCommit() { suite.IsType(ErrMergeNeeded, err) } +// TestBuildNewCommitHeadChecks verifies the head relationship that BuildNewCommit enforces for ordinary, amend, +// and force commits. +func (suite *DatabaseSuite) TestBuildNewCommitHeadChecks() { + // See https://github.com/dolthub/dolt/issues/9072 + ctx := context.Background() + ds, err := suite.db.GetDataset(ctx, "testdataset") + suite.NoError(err) + + // dataset: |a| + ds, err = CommitValue(ctx, suite.db, ds, types.String("a")) + suite.NoError(err) + commitA := mustHeadAddr(ds) + meta := newOpts(suite.db, commitA).Meta + + suite.Run("amend with no head fails", func() { + empty, err := suite.db.GetDataset(ctx, "emptydataset") + suite.NoError(err) + _, err = suite.db.BuildNewCommit(ctx, empty, types.String("x"), CommitOptions{ + Meta: meta, AmendedCommit: commitA, + }) + suite.ErrorIs(err, ErrMergeNeeded) + suite.ErrorContains(err, "dataset has no head") + }) + + suite.Run("amend of the current head succeeds", func() { + commit, err := suite.db.BuildNewCommit(ctx, ds, types.String("a2"), CommitOptions{ + Meta: meta, AmendedCommit: commitA, + }) + suite.NoError(err) + suite.NotNil(commit) + }) + + // dataset: |a| <- |b| + ds, err = CommitValue(ctx, suite.db, ds, types.String("b")) + suite.NoError(err) + commitB := mustHeadAddr(ds) + + suite.Run("amend of a stale head fails", func() { + // The amend was created against commit a but the head has since moved to commit b. + _, err := suite.db.BuildNewCommit(ctx, ds, types.String("x"), CommitOptions{ + Meta: meta, AmendedCommit: commitA, + }) + suite.ErrorIs(err, ErrMergeNeeded) + suite.ErrorContains(err, "is at "+commitB.String()+" but expected "+commitA.String()) + }) + + suite.Run("ordinary commit without the head as a parent fails", func() { + _, err := suite.db.BuildNewCommit(ctx, ds, types.String("x"), newOpts(suite.db, commitA)) + suite.ErrorIs(err, ErrMergeNeeded) + }) + + suite.Run("force skips head validation", func() { + opts := newOpts(suite.db, commitA) + opts.Force = true + commit, err := suite.db.BuildNewCommit(ctx, ds, types.String("x"), opts) + suite.NoError(err) + suite.NotNil(commit) + }) + + suite.Run("force and amend together fail", func() { + _, err := suite.db.BuildNewCommit(ctx, ds, types.String("x"), CommitOptions{ + Meta: meta, Force: true, AmendedCommit: commitB, + }) + suite.ErrorContains(err, "mutually exclusive") + }) +} + func (suite *DatabaseSuite) TestDatabaseDelete() { datasetID1, datasetID2 := "ds1", "ds2" ds1, err := suite.db.GetDataset(context.Background(), datasetID1)