go/libraries/doltcore/sqle: dsess/transactions,engintest: PR feedback.

go/libraries/doltcore/sqle/dsess/session.go

@@ -570,6 +570,8 @@ func (d *DoltSession) NewPendingCommit(ctx *sql.Context, dbName string, roots do
 			mergeParentCommits = append(mergeParentCommits, parentCommit)
 		}
 
+		// 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.
 		root, err := actions.ResetSoftToRef(ctx, sessionState.dbData, "HEAD~1")
 		if err != nil {
 			return nil, err

go/libraries/doltcore/sqle/dsess/transactions.go

@@ -156,6 +156,7 @@ func doltCommit(ctx *sql.Context,
 		return nil, nil, err
 	}
 
+	// We check if the branch HEAD has changed since our transaction started.
 	if curHead != nil {
 		curRootVal, err := curHead.ResolveRootValue(ctx)
 		if err != nil {
@@ -171,8 +172,13 @@ func doltCommit(ctx *sql.Context,
 		}
 
 		if curRootValHash != headRootValHash {
+			// If the branch head changed since our transaction started, then we merge
+			// the existing branch head (curRootVal) into our staged root value. We
+			// treat the HEAD of the branch when our transaction started as the common
+			// ancestor (TODO: This will not be true in the case of destructive branch
+			// updates). The merged root value becomes our new Staged root value which
+			// is the value which we are trying to commit.
 			start := time.Now()
-			mo := merge.MergeOpts{IsCherryPick: false}
 			pending.Roots.Staged, _, err = merge.MergeRoots(
 				ctx,
 				pending.Roots.Staged,
@@ -181,7 +187,7 @@ func doltCommit(ctx *sql.Context,
 				curHead,
 				tx.startState,
 				tx.mergeEditOpts,
-				mo)
+				merge.MergeOpts{})
 			if err != nil {
 				return nil, nil, err
 			}
@@ -303,7 +309,6 @@ func (tx *DoltTransaction) mergeRoots(
 	existingWorkingRoot *doltdb.WorkingSet,
 	workingSet *doltdb.WorkingSet,
 ) (*doltdb.WorkingSet, error) {
-	mo := merge.MergeOpts{IsCherryPick: false}
 	mergedRoot, _, err := merge.MergeRoots(
 		ctx,
 		existingWorkingRoot.WorkingRoot(),
@@ -311,7 +316,8 @@ func (tx *DoltTransaction) mergeRoots(
 		tx.startState.WorkingRoot(),
 		workingSet,
 		tx.startState,
-		tx.mergeEditOpts, mo)
+		tx.mergeEditOpts,
+		merge.MergeOpts{})
 	if err != nil {
 		return nil, err
 	}

go/libraries/doltcore/sqle/enginetest/dolt_transaction_commit_test.go

@@ -292,9 +292,8 @@ func TestDoltTransactionCommitTwoClients(t *testing.T) {
 				Query:    "/* client c */ SELECT * FROM x AS OF 'HEAD' ORDER BY y;",
 				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}},
 			},
-			// After we commit both of these transactions, our working set should not have any pending changes.
-			// In the past, we have merged the working set but failed to land the merged root value in the
-			// commit itself.
+			// After we commit both transactions, our working set should still have the change which
+			// was never dolt_committed.
 			{
 				Query:    "/* client c */ SELECT COUNT(*) FROM DOLT_DIFF('HEAD', 'WORKING', 'x');",
 				Expected: []sql.Row{{1}},

go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go

@@ -755,6 +755,109 @@ var DoltTransactionTests = []queries.TransactionTest{
 			},
 		},
 	},
+	{
+		Name: "call dolt_commit commits staged stuff, merges with working set and branch head",
+		SetUpScript: []string{
+			"create table t1 (id int primary key, val int)",
+			"create table t2 (id int primary key, val int)",
+			"insert into t1 values (1, 1), (2, 2)",
+			"insert into t2 values (1, 1), (2, 2)",
+		},
+		Assertions: []queries.ScriptTestAssertion{
+			{
+				Query:    "/* client a */ set autocommit = off",
+				Expected: []sql.Row{{}},
+			},
+			{
+				Query:    "/* client a */ call dolt_add('t1')",
+				Expected: []sql.Row{{0}},
+			},
+			{
+				Query:    "/* client a */ call dolt_commit('-m', 'initial commit of t1')",
+				SkipResultsCheck: true,
+			},
+			{
+				Query:    "/* client b */ set autocommit = off",
+				Expected: []sql.Row{{}},
+			},
+			{
+				Query:    "/* client a */ start transaction",
+				Expected: []sql.Row{},
+			},
+			{
+				Query:    "/* client b */ start transaction",
+				Expected: []sql.Row{},
+			},
+			{
+				Query:    "/* client a */ insert into t1 values (3, 3)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client b */ insert into t1 values (4, 4)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client a */ insert into t2 values (3, 3)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client b */ insert into t2 values (4, 4)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client a */ call dolt_add('t1')",
+				Expected: []sql.Row{{0}},
+			},
+			{
+				Query:    "/* client b */ call dolt_add('t1')",
+				Expected: []sql.Row{{0}},
+			},
+			{
+				Query:    "/* client a */ insert into t1 values (5, 5)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client b */ insert into t1 values (6, 6)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client c */ insert into t2 values (6, 6)",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "/* client a */ call dolt_commit('-m', 'add 3 to t1')",
+				SkipResultsCheck: true,
+			},
+			{
+				Query:    "/* client a */ select * from t2 order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}, {6, 6}},
+			},
+			{
+				Query:    "/* client a */ select * from t1 order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}, {5, 5}},
+			},
+			{
+				Query:    "/* client a */ select * from t1 as of 'HEAD' order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}},
+			},
+			{
+				Query:    "/* client b */ call dolt_commit('-m', 'add 4 to t1')",
+				SkipResultsCheck: true,
+			},
+			{
+				Query:    "/* client b */ select * from t2 order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}, {4, 4}, {6, 6}},
+			},
+			{
+				Query:    "/* client b */ select * from t1 order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}, {4, 4}, {5, 5}, {6, 6}},
+			},
+			{
+				Query:    "/* client b */ select * from t1 as of 'HEAD' order by id asc",
+				Expected: []sql.Row{{1, 1}, {2, 2}, {3, 3}, {4, 4}},
+			},
+		},
+	},
 }
 
 var DoltConflictHandlingTests = []queries.TransactionTest{