Merge pull request #10810 from dolthub/elian/10807

Fix remotesapi push fail with "target has uncommitted changes" after any successful push

Author: elianddb

go/libraries/doltcore/doltdb/doltdb.go

@@ -885,10 +885,12 @@ func (ddb *DoltDB) Commit(ctx context.Context, valHash hash.Hash, dref ref.DoltR
 	return ddb.CommitWithParentSpecs(ctx, valHash, dref, nil, cm)
 }
 
-// FastForwardWithWorkspaceCheck will perform a fast forward update of the branch given to the commit given, but only
-// if the working set is in sync with the head of the branch given. This is used in the course of pushing to a remote.
-// If the target doesn't currently have the working set ref, then no working set change will be made.
-func (ddb *DoltDB) FastForwardWithWorkspaceCheck(ctx context.Context, branch ref.DoltRef, commit *Commit) error {
+// FastForwardWithWorkspaceCheck performs a fast-forward of |branch| to |commit| only if the remote
+// working set is in sync with its HEAD, then updates the working set to match the new HEAD.
+// This is used when pushing to a remote. If the remote does not maintain a working set, no working
+// set update is performed.
+// When |allowDirtyWorking| is true, the working set may have unstaged changes, such as ignored tables.
+func (ddb *DoltDB) FastForwardWithWorkspaceCheck(ctx context.Context, branch ref.DoltRef, commit *Commit, allowDirtyWorking bool) error {
 	ds, err := ddb.db.GetDataset(ctx, branch.String())
 	if err != nil {
 		return err
@@ -909,7 +911,7 @@ func (ddb *DoltDB) FastForwardWithWorkspaceCheck(ctx context.Context, branch ref
 		ws = wsRef.String()
 	}
 
-	_, err = ddb.db.FastForward(ctx, ds, addr, ws)
+	_, err = ddb.db.FastForward(ctx, ds, addr, ws, allowDirtyWorking)
 
 	return err
 }
@@ -931,7 +933,7 @@ func (ddb *DoltDB) FastForwardToHash(ctx context.Context, branch ref.DoltRef, ha
 		return err
 	}
 
-	_, err = ddb.db.FastForward(ctx, ds, hash, "")
+	_, err = ddb.db.FastForward(ctx, ds, hash, "", false)
 
 	return err
 }

go/libraries/doltcore/doltdb/hooksdatabase.go

@@ -164,8 +164,8 @@ func (db hooksDatabase) SetHead(ctx context.Context, ds datas.Dataset, newHeadAd
 	return ds, err
 }
 
-func (db hooksDatabase) FastForward(ctx context.Context, ds datas.Dataset, newHeadAddr hash.Hash, workingSetPath string) (datas.Dataset, error) {
-	ds, err := db.Database.FastForward(ctx, ds, newHeadAddr, workingSetPath)
+func (db hooksDatabase) FastForward(ctx context.Context, ds datas.Dataset, newHeadAddr hash.Hash, workingSetPath string, allowDirtyWorking bool) (datas.Dataset, error) {
+	ds, err := db.Database.FastForward(ctx, ds, newHeadAddr, workingSetPath, allowDirtyWorking)
 	if err == nil {
 		db.ExecuteCommitHooks(ctx, ds, false, false)
 	}

go/libraries/doltcore/env/actions/remotes.go

@@ -86,18 +86,13 @@ func Push(ctx context.Context, tempTableDir string, mode ref.UpdateMode, destRef
 		}
 		err = srcDB.SetHeadToCommit(ctx, remoteRef, commit)
 	case ref.FastForwardOnly:
-		// ResolveBranchRoots fails for remotes that don't maintain a working set (e.g. file://),
-		// in which case FastForwardWithWorkspaceCheck handles it correctly without the ignored-table check.
+		// Ignored tables create a staged/working mismatch that blocks fast-forward pushes, so we detect and permit that case.
 		onlyIgnored := false
 		roots, err := destDB.ResolveBranchRoots(ctx, destRef)
 		if err == nil {
 			onlyIgnored, _ = diff.WorkingSetContainsOnlyIgnoredTables(ctx, roots)
 		}
-		if onlyIgnored {
-			err = destDB.FastForward(ctx, destRef, commit)
-		} else {
-			err = destDB.FastForwardWithWorkspaceCheck(ctx, destRef, commit)
-		}
+		err = destDB.FastForwardWithWorkspaceCheck(ctx, destRef, commit, onlyIgnored)
 		if err != nil {
 			return err
 		}

go/store/datas/database.go

@@ -149,14 +149,12 @@ type Database interface {
 	// is not provided, no working set update will be performed.
 	SetHead(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, workingSetPath string) (Dataset, error)
 
-	// FastForward takes a types.Ref to a Commit object and makes it the new
-	// Head of ds iff it is a descendant of the current Head. Intended to be
-	// used e.g. after a call to Pull(). If the update cannot be performed,
-	// e.g., because another process moved the current Head out from under
-	// you, err will be non-nil.
-	// The workingSetPath is the path to the working set that should be used for updating the working set. If one
-	// is not provided, no working set update will be performed.
-	FastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, workingSetPath string) (Dataset, error)
+	// FastForward sets the head of |ds| to the commit at |newHeadAddr| if it is a descendant of
+	// the current head. If the update cannot be performed, for example because another writer has
+	// moved the head concurrently, err will be non-nil.
+	// If |workingSetPath| is non-empty, the working set at that path is updated to match the new head.
+	// When |allowDirtyWorking| is true, the working set may have unstaged changes, such as ignored tables.
+	FastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, workingSetPath string, allowDirtyWorking bool) (Dataset, error)
 
 	// Stats may return some kind of struct that reports statistics about the
 	// ChunkStore that backs this Database instance. The type is

go/store/datas/database_common.go

@@ -337,13 +337,13 @@ func (db *database) doSetHead(ctx context.Context, ds Dataset, addr hash.Hash, w
 	})
 }
 
-func (db *database) FastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, wsPath string) (Dataset, error) {
+func (db *database) FastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, wsPath string, allowDirtyWorking bool) (Dataset, error) {
 	return db.doHeadUpdate(ctx, ds, func(ds Dataset) error {
-		return db.doFastForward(ctx, ds, newHeadAddr, wsPath)
+		return db.doFastForward(ctx, ds, newHeadAddr, wsPath, allowDirtyWorking)
 	})
 }
 
-func (db *database) doFastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, workingSetPath string) error {
+func (db *database) doFastForward(ctx context.Context, ds Dataset, newHeadAddr hash.Hash, workingSetPath string, allowDirtyWorking bool) error {
 	newHead, err := db.readHead(ctx, newHeadAddr)
 	if err != nil {
 		return err
@@ -434,7 +434,7 @@ func (db *database) doFastForward(ctx context.Context, ds Dataset, newHeadAddr h
 
 					stagedHash := hash.New(msg.StagedRootAddrBytes())
 					workingSetHash := hash.New(msg.WorkingRootAddrBytes())
-					if stagedHash != workingSetHash {
+					if !allowDirtyWorking && stagedHash != workingSetHash {
 						return prolly.AddressMap{}, ErrDirtyWorkspace
 					}
 

go/store/datas/database_test.go

@@ -442,7 +442,7 @@ func (suite *DatabaseSuite) TestFastForward() {
 	cCommitAddr := mustHeadAddr(ds) // To use in FastForward() below.
 
 	// FastForward should disallow this, as |a| is not a descendant of |c|
-	_, err = suite.db.FastForward(context.Background(), ds, aCommitAddr, "")
+	_, err = suite.db.FastForward(context.Background(), ds, aCommitAddr, "", false)
 	suite.Error(err)
 
 	// Move Head back to something earlier in the lineage, so we can test FastForward
@@ -451,7 +451,7 @@ func (suite *DatabaseSuite) TestFastForward() {
 	suite.True(mustHeadValue(ds).Equals(a))
 
 	// This should succeed, because while |a| is not a direct parent of |c|, it is an ancestor.
-	ds, err = suite.db.FastForward(context.Background(), ds, cCommitAddr, "")
+	ds, err = suite.db.FastForward(context.Background(), ds, cCommitAddr, "", false)
 	suite.Require().NoError(err)
 	suite.True(mustHeadValue(ds).Equals(c))
 }

go/store/datas/pull/puller_test.go

@@ -412,7 +412,7 @@ func testPuller(t *testing.T, makeDB datasFactory) {
 
 			sinkDS, err := sinkdb.GetDataset(ctx, "ds")
 			require.NoError(t, err)
-			sinkDS, err = sinkdb.FastForward(ctx, sinkDS, rootAddr, "")
+			sinkDS, err = sinkdb.FastForward(ctx, sinkDS, rootAddr, "", false)
 			require.NoError(t, err)
 
 			require.NoError(t, err)

integration-tests/bats/sql-server-remotesrv.bats

@@ -839,3 +839,46 @@ GRANT CLONE_ADMIN ON *.* TO clone_admin_user@'localhost';
     [ "$status" -eq 0 ]
     [[ "$output" =~ "add rows via remotesapi push" ]] || false
 }
+
+# https://github.com/dolthub/dolt/issues/10807
+@test "sql-server-remotesrv: consecutive pushes succeed with ignored tables, but fail when remote has real uncommitted changes" {
+    mkdir remote
+    cd remote
+    dolt init
+    dolt sql -q 'create table names (name varchar(10) primary key);'
+    dolt sql -q 'insert into names (name) values ("abe"), ("betsy"), ("calvin");'
+    dolt sql -q "INSERT INTO dolt_ignore VALUES ('tmp_*', true);"
+    dolt sql -q 'create table tmp_scratch (id int primary key);'  # ignored: in working but not staged
+    dolt add names dolt_ignore
+    dolt commit -m 'initial names.'
+
+    APIPORT=$( definePORT )
+    dolt sql -q "CREATE USER root@'%' identified by 'rootpass'; GRANT ALL ON *.* to root@'%';"
+    export DOLT_REMOTE_PASSWORD="rootpass"
+    export SQL_USER="root"
+    start_sql_server_with_args --remotesapi-port $APIPORT
+
+    cd ../
+    dolt clone http://localhost:$APIPORT/remote cloned_db -u $SQL_USER
+    cd cloned_db
+
+    dolt sql -q 'insert into names values ("dave");'
+    dolt commit -am 'add dave'
+    run dolt push origin --user $SQL_USER main:main
+    [[ "$status" -eq 0 ]] || false
+
+    dolt sql -q 'insert into names values ("eve");'
+    dolt commit -am 'add eve'
+    run dolt push origin --user $SQL_USER main:main
+    [[ "$status" -eq 0 ]] || false
+    [[ ! "$output" =~ "target has uncommitted changes" ]] || false
+
+    # Add a real (non-ignored) uncommitted change to the remote; push must now fail.
+    dolt --port $PORT --host 127.0.0.1 -u $SQL_USER -p rootpass --no-tls --use-db remote sql -q "INSERT INTO names VALUES ('zeek');"
+
+    dolt sql -q 'insert into names values ("frank");'
+    dolt commit -am 'add frank'
+    run dolt push origin --user $SQL_USER main:main
+    [[ "$status" -ne 0 ]] || false
+    [[ "$output" =~ "target has uncommitted changes" ]] || false
+}