Fail amend commits when branch head has moved

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)

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
 	}

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,

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
 }

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())
 	}

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.

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)

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 {

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()))
 		}
 	}
 

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{