noff doesn't surface constraint violations (#2233)

* noff doesn't surface constraint violations

* only no-ff merge if we can fast-forward HEAD safely

* daylon's comments

* same error skip for sql

go/libraries/doltcore/merge/action.go

@@ -105,31 +105,19 @@ func NewMergeSpec(ctx context.Context, rsr env.RepoStateReader, ddb *doltdb.Dolt
 	}, true, nil
 }
 
+// TODO forcing a commit with a constrain violation should warn users that subsequest
+//      FF merges will not surface constraint violations on their own; constraint verify --all
+//      is required to reify violations.
 func MergeCommitSpec(ctx context.Context, dEnv *env.DoltEnv, spec *MergeSpec) (map[string]*MergeStats, error) {
-	if ok, err := spec.HeadC.CanFastForwardTo(ctx, spec.MergeC); ok {
-		ancRoot, err := spec.HeadC.GetRootValue()
-		if err != nil {
-			return nil, err
-		}
-		mergedRoot, err := spec.MergeC.GetRootValue()
-		if err != nil {
-			return nil, err
-		}
-		if cvPossible, err := MayHaveConstraintViolations(ctx, ancRoot, mergedRoot); err != nil {
-			return nil, err
-		} else if cvPossible {
-			return ExecuteMerge(ctx, dEnv, spec)
-		}
+	if ok, err := spec.HeadC.CanFastForwardTo(ctx, spec.MergeC); err != nil && !errors.Is(err, doltdb.ErrUpToDate) {
+		return nil, err
+	} else if ok {
 		if spec.Noff {
 			return ExecNoFFMerge(ctx, dEnv, spec)
-		} else {
-			return nil, ExecuteFFMerge(ctx, dEnv, spec)
 		}
-	} else if err == doltdb.ErrUpToDate || err == doltdb.ErrIsAhead {
-		return nil, err
-	} else {
-		return ExecuteMerge(ctx, dEnv, spec)
+		return nil, ExecuteFFMerge(ctx, dEnv, spec)
 	}
+	return ExecuteMerge(ctx, dEnv, spec)
 }
 
 func ExecNoFFMerge(ctx context.Context, dEnv *env.DoltEnv, spec *MergeSpec) (map[string]*MergeStats, error) {

go/libraries/doltcore/sqle/dfunctions/dolt_merge.go

@@ -119,6 +119,7 @@ func (d DoltMergeFunc) Eval(ctx *sql.Context, row sql.Row) (interface{}, error)
 // mergeIntoWorkingSet encapsulates server merge logic, switching between fast-forward, no fast-forward, merge commit,
 // and merging into working set. Returns a new WorkingSet and whether there were merge conflicts. This currently
 // persists merge commits in the database, but expects the caller to update the working set.
+// TODO FF merging commit with constraint violations requires `constraint verify`
 func mergeIntoWorkingSet(ctx *sql.Context, sess *dsess.Session, roots doltdb.Roots, ws *doltdb.WorkingSet, dbName string, spec *merge.MergeSpec) (*doltdb.WorkingSet, int, error) {
 	if conflicts, err := roots.Working.HasConflicts(ctx); err != nil {
 		return ws, noConflicts, err
@@ -141,51 +142,34 @@ func mergeIntoWorkingSet(ctx *sql.Context, sess *dsess.Session, roots doltdb.Roo
 		return ws, noConflicts, err
 	}
 
-	canFF, err := spec.HeadC.CanFastForwardTo(ctx, spec.MergeC)
-	if err != nil {
-		return ws, noConflicts, err
+	dbData, ok := sess.GetDbData(ctx, dbName)
+	if !ok {
+		return ws, noConflicts, fmt.Errorf("failed to get dbData")
 	}
 
-	if canFF {
-		headRoot, err := spec.HeadC.GetRootValue()
-		if err != nil {
-			return ws, noConflicts, err
-		}
-		mergeRoot, err := spec.MergeC.GetRootValue()
-		if err != nil {
-			return ws, noConflicts, err
-		}
-		if cvPossible, err := merge.MayHaveConstraintViolations(ctx, headRoot, mergeRoot); err != nil {
-			return ws, noConflicts, err
-		} else if !cvPossible {
-			dbData, ok := sess.GetDbData(ctx, dbName)
-			if !ok {
-				return ws, noConflicts, fmt.Errorf("could not load database %s", dbName)
-			}
-			if spec.Noff {
-				ws, err = executeNoFFMerge(ctx, sess, spec, dbName, ws, dbData)
-				if err == doltdb.ErrUnresolvedConflicts {
-					// if there are unresolved conflicts, write the resulting working set back to the session and return an
-					// error message
-					wsErr := sess.SetWorkingSet(ctx, dbName, ws, nil)
-					if wsErr != nil {
-						return ws, hasConflicts, wsErr
-					}
-
-					ctx.Warn(DoltConflictWarningCode, err.Error())
-
-					// Return 0 indicating there are conflicts
-					return ws, hasConflicts, nil
+	if canFF, err := spec.HeadC.CanFastForwardTo(ctx, spec.MergeC); err != nil && !errors.Is(err, doltdb.ErrUpToDate) {
+		return ws, noConflicts, err
+	} else if canFF {
+		if spec.Noff {
+			ws, err = executeNoFFMerge(ctx, sess, spec, dbName, ws, dbData)
+			if err == doltdb.ErrUnresolvedConflicts {
+				// if there are unresolved conflicts, write the resulting working set back to the session and return an
+				// error message
+				wsErr := sess.SetWorkingSet(ctx, dbName, ws, nil)
+				if wsErr != nil {
+					return ws, hasConflicts, wsErr
 				}
-			} else {
-				ws, err = executeFFMerge(ctx, spec.Squash, ws, dbData, spec.MergeC)
-			}
 
-			if err != nil {
-				return ws, noConflicts, err
+				ctx.Warn(DoltConflictWarningCode, err.Error())
+
+				// Return 0 indicating there are conflicts
+				return ws, hasConflicts, nil
 			}
 			return ws, noConflicts, err
 		}
+
+		ws, err = executeFFMerge(ctx, spec.Squash, ws, dbData, spec.MergeC)
+		return ws, noConflicts, err
 	}
 
 	dbState, ok, err := sess.LookupDbState(ctx, dbName)
@@ -196,7 +180,7 @@ func mergeIntoWorkingSet(ctx *sql.Context, sess *dsess.Session, roots doltdb.Roo
 	}
 
 	ws, err = executeMerge(ctx, spec.Squash, spec.HeadC, spec.MergeC, ws, dbState.EditSession.Opts)
-	if err == doltdb.ErrUnresolvedConflicts {
+	if err == doltdb.ErrUnresolvedConflicts || err == doltdb.ErrUnresolvedConstraintViolations {
 		// if there are unresolved conflicts, write the resulting working set back to the session and return an
 		// error message
 		wsErr := sess.SetWorkingSet(ctx, dbName, ws, nil)
@@ -210,6 +194,7 @@ func mergeIntoWorkingSet(ctx *sql.Context, sess *dsess.Session, roots doltdb.Roo
 	} else if err != nil {
 		return ws, noConflicts, err
 	}
+
 	return ws, noConflicts, nil
 }
 

integration-tests/bats/constraint-violations.bats

@@ -2694,6 +2694,10 @@ SQL
     dolt checkout main
     dolt merge other
 
+    # FF merge no longer checks constraints; forced commits require constraint reification
+    run dolt constraints verify --all
+    [ "$status" -eq "1" ]
+
     run dolt sql -q "SELECT * FROM dolt_constraint_violations" -r=csv
     [ "$status" -eq "0" ]
     [[ "$output" =~ "table,num_violations" ]] || false
@@ -2720,11 +2724,14 @@ SQL
     [[ "$output" =~ "2,2" ]] || false
     [[ "${#lines[@]}" = "3" ]] || false
 
-    dolt merge --abort
     dolt reset --hard
     dolt checkout main2
     dolt merge other2
 
+    # FF merge no longer checks constraints; forced commits require constraint reification
+    run dolt constraints verify --all
+    [ "$status" -eq "1" ]
+
     run dolt sql -q "SELECT * FROM dolt_constraint_violations" -r=csv
     [ "$status" -eq "0" ]
     [[ "$output" =~ "table,num_violations" ]] || false