Merge pull request #6086 from dolthub/fulghum/skip-empty-commit

Add support for `--skip-empty` to `dolt commit` and `dolt_commit()`

go/cmd/dolt/cli/arg_parser_helpers.go

@@ -77,6 +77,7 @@ func ParseAuthor(authorStr string) (string, string, error) {
 
 const (
 	AllowEmptyFlag   = "allow-empty"
+	SkipEmptyFlag    = "skip-empty"
 	DateParam        = "date"
 	MessageArg       = "message"
 	AuthorParam      = "author"
@@ -140,7 +141,8 @@ var branchForceFlagDesc = "Reset {{.LessThan}}branchname{{.GreaterThan}} to {{.L
 func CreateCommitArgParser() *argparser.ArgParser {
 	ap := argparser.NewArgParserWithMaxArgs("commit", 0)
 	ap.SupportsString(MessageArg, "m", "msg", "Use the given {{.LessThan}}msg{{.GreaterThan}} as the commit message.")
-	ap.SupportsFlag(AllowEmptyFlag, "", "Allow recording a commit that has the exact same data as its sole parent. This is usually a mistake, so it is disabled by default. This option bypasses that safety.")
+	ap.SupportsFlag(AllowEmptyFlag, "", "Allow recording a commit that has the exact same data as its sole parent. This is usually a mistake, so it is disabled by default. This option bypasses that safety. Cannot be used with --skip-empty.")
+	ap.SupportsFlag(SkipEmptyFlag, "", "Only create a commit if there are staged changes. If no changes are staged, the call to commit is a no-op. Cannot be used with --allow-empty.")
 	ap.SupportsString(DateParam, "", "date", "Specify the date used in the commit. If not specified the current system time is used.")
 	ap.SupportsFlag(ForceFlag, "f", "Ignores any foreign key warnings and proceeds with the commit.")
 	ap.SupportsString(AuthorParam, "", "author", "Specify an explicit author using the standard A U Thor {{.LessThan}}author@example.com{{.GreaterThan}} format.")
@@ -403,3 +405,13 @@ func VerifyNoAwsParams(apr *argparser.ArgParseResults) error {
 
 	return nil
 }
+
+// VerifyCommitArgs validates the arguments in |apr| for `dolt commit` and returns an error
+// if any validation problems were encountered.
+func VerifyCommitArgs(apr *argparser.ArgParseResults) error {
+	if apr.Contains(AllowEmptyFlag) && apr.Contains(SkipEmptyFlag) {
+		return fmt.Errorf("error: cannot use both --allow-empty and --skip-empty")
+	}
+
+	return nil
+}

go/cmd/dolt/commands/commit.go

@@ -77,41 +77,53 @@ func (cmd CommitCmd) ArgParser() *argparser.ArgParser {
 
 // Exec executes the command
 func (cmd CommitCmd) Exec(ctx context.Context, commandStr string, args []string, dEnv *env.DoltEnv, cliCtx cli.CliContext) int {
-	res := performCommit(ctx, commandStr, args, dEnv)
+	res, skipped := performCommit(ctx, commandStr, args, dEnv)
 	if res == 1 {
 		return res
 	}
 
+	if skipped {
+		iohelp.WriteLine(cli.CliOut, "Skipping empty commit")
+		return res
+	}
+
 	// if the commit was successful, print it out using the log command
 	return LogCmd{}.Exec(ctx, "log", []string{"-n=1"}, dEnv, nil)
 }
 
-func performCommit(ctx context.Context, commandStr string, args []string, dEnv *env.DoltEnv) int {
+// performCommit creates a new Dolt commit using the specified |commandStr| and |args| for the specified Dolt environment
+// |dEnv|. The response is an integer status code indicating success or failure, as well as a boolean that indicates
+// if the commit was skipped (e.g. because --skip-empty was specified as an argument).
+func performCommit(ctx context.Context, commandStr string, args []string, dEnv *env.DoltEnv) (int, bool) {
 	ap := cli.CreateCommitArgParser()
 	help, usage := cli.HelpAndUsagePrinters(cli.CommandDocsForCommandString(commandStr, commitDocs, ap))
 	apr := cli.ParseArgsOrDie(ap, args, help)
 
+	if err := cli.VerifyCommitArgs(apr); err != nil {
+		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), help), false
+	}
+
 	allFlag := apr.Contains(cli.AllFlag)
 	upperCaseAllFlag := apr.Contains(cli.UpperCaseAllFlag)
 
 	if dEnv.IsLocked() {
-		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(env.ErrActiveServerLock.New(dEnv.LockFile())), help)
+		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(env.ErrActiveServerLock.New(dEnv.LockFile())), help), false
 	}
 
 	roots, err := dEnv.Roots(ctx)
 	if err != nil {
-		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working root").AddCause(err).Build(), usage)
+		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working root").AddCause(err).Build(), usage), false
 	}
 
 	if upperCaseAllFlag {
 		roots, err = actions.StageAllTables(ctx, roots, true)
 		if err != nil {
-			return handleCommitErr(ctx, dEnv, err, help)
+			return handleCommitErr(ctx, dEnv, err, help), false
 		}
 	} else if allFlag {
 		roots, err = actions.StageModifiedAndDeletedTables(ctx, roots)
 		if err != nil {
-			return handleCommitErr(ctx, dEnv, err, help)
+			return handleCommitErr(ctx, dEnv, err, help), false
 		}
 	}
 
@@ -125,13 +137,13 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 	} else {
 		// This command creates a commit, so we need user identity
 		if !cli.CheckUserNameAndEmail(dEnv) {
-			return 1
+			return 1, false
 		}
 		name, email, err = env.GetNameAndEmail(dEnv.Config)
 	}
 
 	if err != nil {
-		return handleCommitErr(ctx, dEnv, err, usage)
+		return handleCommitErr(ctx, dEnv, err, usage), false
 	}
 
 	msg, msgOk := apr.GetValue(cli.MessageArg)
@@ -140,13 +152,13 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 		if apr.Contains(cli.AmendFlag) {
 			commitMeta, cmErr := headCommit.GetCommitMeta(ctx)
 			if cmErr != nil {
-				return handleCommitErr(ctx, dEnv, cmErr, usage)
+				return handleCommitErr(ctx, dEnv, cmErr, usage), false
 			}
 			amendStr = commitMeta.Description
 		}
 		msg, err = getCommitMessageFromEditor(ctx, dEnv, "", amendStr, false)
 		if err != nil {
-			return handleCommitErr(ctx, dEnv, err, usage)
+			return handleCommitErr(ctx, dEnv, err, usage), false
 		}
 	}
 
@@ -156,7 +168,7 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 		t, err = cli.ParseDate(commitTimeStr)
 
 		if err != nil {
-			return HandleVErrAndExitCode(errhand.BuildDError("error: invalid date").AddCause(err).Build(), usage)
+			return HandleVErrAndExitCode(errhand.BuildDError("error: invalid date").AddCause(err).Build(), usage), false
 		}
 	}
 
@@ -172,23 +184,23 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 
 		newRoots, err := actions.ResetSoftToRef(ctx, dEnv.DbData(), "HEAD~1")
 		if err != nil {
-			return handleResetError(err, usage)
+			return handleResetError(err, usage), false
 		}
 
 		err = dEnv.UpdateStagedRoot(ctx, newRoots.Staged)
 		if err != nil {
-			return handleResetError(err, usage)
+			return handleResetError(err, usage), false
 		}
 	}
 
 	ws, err := dEnv.WorkingSet(ctx)
 	if err != nil {
-		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working set").AddCause(err).Build(), usage)
+		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working set").AddCause(err).Build(), usage), false
 	}
 
 	prevHash, err := ws.HashOf()
 	if err != nil {
-		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working set").AddCause(err).Build(), usage)
+		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't get working set").AddCause(err).Build(), usage), false
 	}
 
 	var mergeParentCommits []*doltdb.Commit
@@ -202,6 +214,7 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 		Message:    msg,
 		Date:       t,
 		AllowEmpty: apr.Contains(cli.AllowEmptyFlag) || apr.Contains(cli.AmendFlag),
+		SkipEmpty:  apr.Contains(cli.SkipEmptyFlag),
 		Force:      apr.Contains(cli.ForceFlag),
 		Name:       name,
 		Email:      email,
@@ -210,20 +223,26 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 		if apr.Contains(cli.AmendFlag) {
 			newRoots, errRes := actions.ResetSoftToRef(ctx, dEnv.DbData(), headHash.String())
 			if errRes != nil {
-				return handleResetError(errRes, usage)
+				return handleResetError(errRes, usage), false
 			}
 
 			err = dEnv.UpdateStagedRoot(ctx, newRoots.Staged)
 			if err != nil {
-				return handleResetError(err, usage)
+				return handleResetError(err, usage), false
 			}
 		}
-		return handleCommitErr(ctx, dEnv, err, usage)
+		return handleCommitErr(ctx, dEnv, err, usage), false
+	}
+
+	// If no error was reported and there is no pending commit, then no commit was created, likely
+	// because of --skip-empty, so return a success code, but indicate that no commit was created.
+	if pendingCommit == nil {
+		return 0, true
 	}
 
 	headRef, err := dEnv.RepoStateReader().CWBHeadRef()
 	if err != nil {
-		return handleCommitErr(ctx, dEnv, err, usage)
+		return handleCommitErr(ctx, dEnv, err, usage), false
 	}
 	_, err = dEnv.DoltDB.CommitWithWorkingSet(
 		ctx,
@@ -239,18 +258,18 @@ func performCommit(ctx context.Context, commandStr string, args []string, dEnv *
 		if apr.Contains(cli.AmendFlag) {
 			newRoots, errRes := actions.ResetSoftToRef(ctx, dEnv.DbData(), headHash.String())
 			if errRes != nil {
-				return handleResetError(errRes, usage)
+				return handleResetError(errRes, usage), false
 			}
 
 			err = dEnv.UpdateStagedRoot(ctx, newRoots.Staged)
 			if err != nil {
-				return handleResetError(err, usage)
+				return handleResetError(err, usage), false
 			}
 		}
-		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't commit").AddCause(err).Build(), usage)
+		return HandleVErrAndExitCode(errhand.BuildDError("Couldn't commit").AddCause(err).Build(), usage), false
 	}
 
-	return 0
+	return 0, false
 }
 
 func handleCommitErr(ctx context.Context, dEnv *env.DoltEnv, err error, usage cli.UsagePrinter) int {

go/cmd/dolt/commands/merge.go

@@ -580,8 +580,8 @@ func executeMergeAndCommit(ctx context.Context, dEnv *env.DoltEnv, spec *merge.M
 
 	author := fmt.Sprintf("%s <%s>", spec.Name, spec.Email)
 
-	res := performCommit(ctx, "commit", []string{"-m", msg, "--author", author}, dEnv)
-	if res != 0 {
+	res, skipped := performCommit(ctx, "commit", []string{"-m", msg, "--author", author}, dEnv)
+	if res != 0 || skipped {
 		return nil, fmt.Errorf("dolt commit failed after merging")
 	}
 

go/libraries/doltcore/env/actions/commit.go

@@ -27,6 +27,7 @@ type CommitStagedProps struct {
 	Message    string
 	Date       time.Time
 	AllowEmpty bool
+	SkipEmpty  bool
 	Amend      bool
 	Force      bool
 	Name       string
@@ -62,6 +63,10 @@ func GetCommitStaged(
 
 	isEmpty := len(staged) == 0
 	allowEmpty := ws.MergeActive() || props.AllowEmpty || props.Amend
+
+	if isEmpty && props.SkipEmpty {
+		return nil, nil
+	}
 	if isEmpty && !allowEmpty {
 		return nil, NothingStaged{notStaged}
 	}

go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go

@@ -93,7 +93,8 @@ func doDoltCherryPick(ctx *sql.Context, args []string) (string, error) {
 		return "", fmt.Errorf("dolt add failed")
 	}
 
-	return doDoltCommit(ctx, []string{"-m", commitMsg})
+	commitHash, _, err := doDoltCommit(ctx, []string{"-m", commitMsg})
+	return commitHash, err
 }
 
 // cherryPick checks that the current working set is clean, verifies the cherry-pick commit is not a merge commit

go/libraries/doltcore/sqle/dprocedures/dolt_commit.go

@@ -32,51 +32,65 @@ var hashType = types.MustCreateString(query.Type_TEXT, 32, sql.Collation_ascii_b
 
 // doltCommit is the stored procedure version for the CLI command `dolt commit`.
 func doltCommit(ctx *sql.Context, args ...string) (sql.RowIter, error) {
-	res, err := doDoltCommit(ctx, args)
+	commitHash, skipped, err := doDoltCommit(ctx, args)
 	if err != nil {
 		return nil, err
 	}
-	return rowToIter(res), nil
+	if skipped {
+		return nil, nil
+	}
+	return rowToIter(commitHash), nil
 }
 
 // doltCommitHashOut is the stored procedure version for the CLI function `commit`. The first parameter is the variable
 // to set the hash of.
 func doltCommitHashOut(ctx *sql.Context, outHash *string, args ...string) (sql.RowIter, error) {
-	res, err := doDoltCommit(ctx, args)
+	commitHash, skipped, err := doDoltCommit(ctx, args)
 	if err != nil {
 		return nil, err
 	}
-	*outHash = res
-	return rowToIter(res), nil
+	if skipped {
+		return nil, nil
+	}
+
+	*outHash = commitHash
+	return rowToIter(commitHash), nil
 }
 
-func doDoltCommit(ctx *sql.Context, args []string) (string, error) {
+// doDoltCommit creates a dolt commit using the specified command line |args| provided. The response is the commit hash
+// of the new commit (or the empty string if the commit was skipped), a boolean that indicates if creating the commit
+// was skipped (e.g. due to --skip-empty), and an error describing any error encountered.
+func doDoltCommit(ctx *sql.Context, args []string) (string, bool, error) {
 	if err := branch_control.CheckAccess(ctx, branch_control.Permissions_Write); err != nil {
-		return "", err
+		return "", false, err
 	}
 	// Get the information for the sql context.
 	dbName := ctx.GetCurrentDatabase()
 
 	apr, err := cli.CreateCommitArgParser().Parse(args)
 	if err != nil {
-		return "", err
+		return "", false, err
+	}
+
+	if err := cli.VerifyCommitArgs(apr); err != nil {
+		return "", false, err
 	}
 
 	dSess := dsess.DSessFromSess(ctx.Session)
 	roots, ok := dSess.GetRoots(ctx, dbName)
 	if !ok {
-		return "", fmt.Errorf("Could not load database %s", dbName)
+		return "", false, fmt.Errorf("Could not load database %s", dbName)
 	}
 
 	if apr.Contains(cli.UpperCaseAllFlag) {
 		roots, err = actions.StageAllTables(ctx, roots, true)
 		if err != nil {
-			return "", fmt.Errorf(err.Error())
+			return "", false, fmt.Errorf(err.Error())
 		}
 	} else if apr.Contains(cli.AllFlag) {
 		roots, err = actions.StageModifiedAndDeletedTables(ctx, roots)
 		if err != nil {
-			return "", fmt.Errorf(err.Error())
+			return "", false, fmt.Errorf(err.Error())
 		}
 	}
 
@@ -84,7 +98,7 @@ func doDoltCommit(ctx *sql.Context, args []string) (string, error) {
 	if authorStr, ok := apr.GetValue(cli.AuthorParam); ok {
 		name, email, err = cli.ParseAuthor(authorStr)
 		if err != nil {
-			return "", err
+			return "", false, err
 		}
 	} else {
 		// In SQL mode, use the current SQL user as the commit author, instead of the `dolt config` configured values.
@@ -100,15 +114,15 @@ func doDoltCommit(ctx *sql.Context, args []string) (string, error) {
 		if amend {
 			commit, err := dSess.GetHeadCommit(ctx, dbName)
 			if err != nil {
-				return "", err
+				return "", false, err
 			}
 			commitMeta, err := commit.GetCommitMeta(ctx)
 			if err != nil {
-				return "", err
+				return "", false, err
 			}
 			msg = commitMeta.Description
 		} else {
-			return "", fmt.Errorf("Must provide commit message.")
+			return "", false, fmt.Errorf("Must provide commit message.")
 		}
 	}
 
@@ -118,7 +132,7 @@ func doDoltCommit(ctx *sql.Context, args []string) (string, error) {
 		t, err = cli.ParseDate(commitTimeStr)
 
 		if err != nil {
-			return "", fmt.Errorf(err.Error())
+			return "", false, fmt.Errorf(err.Error())
 		}
 	}
 
@@ -126,31 +140,34 @@ func doDoltCommit(ctx *sql.Context, args []string) (string, error) {
 		Message:    msg,
 		Date:       t,
 		AllowEmpty: apr.Contains(cli.AllowEmptyFlag),
+		SkipEmpty:  apr.Contains(cli.SkipEmptyFlag),
 		Amend:      amend,
 		Force:      apr.Contains(cli.ForceFlag),
 		Name:       name,
 		Email:      email,
 	})
 	if err != nil {
-		return "", err
+		return "", false, err
 	}
 
 	// Nothing to commit, and we didn't pass --allowEmpty
-	if pendingCommit == nil {
-		return "", errors.New("nothing to commit")
+	if pendingCommit == nil && apr.Contains(cli.SkipEmptyFlag) {
+		return "", true, nil
+	} else if pendingCommit == nil {
+		return "", false, errors.New("nothing to commit")
 	}
 
 	newCommit, err := dSess.DoltCommit(ctx, dbName, dSess.GetTransaction(), pendingCommit)
 	if err != nil {
-		return "", err
+		return "", false, err
 	}
 
 	h, err := newCommit.HashOf()
 	if err != nil {
-		return "", err
+		return "", false, err
 	}
 
-	return h.String(), nil
+	return h.String(), false, nil
 }
 
 func getDoltArgs(ctx *sql.Context, row sql.Row, children []sql.Expression) ([]string, error) {

go/libraries/doltcore/sqle/dprocedures/dolt_merge.go

@@ -222,7 +222,7 @@ func performMerge(ctx *sql.Context, sess *dsess.DoltSession, roots doltdb.Roots,
 
 	if !noCommit {
 		author := fmt.Sprintf("%s <%s>", spec.Name, spec.Email)
-		_, err = doDoltCommit(ctx, []string{"-m", msg, "--author", author})
+		_, _, err = doDoltCommit(ctx, []string{"-m", msg, "--author", author})
 		if err != nil {
 			return ws, noConflictsOrViolations, threeWayMerge, fmt.Errorf("dolt_commit failed")
 		}

go/libraries/doltcore/sqle/dprocedures/dolt_revert.go

@@ -128,7 +128,7 @@ func doDoltRevert(ctx *sql.Context, args []string) (int, error) {
 		if err != nil {
 			return 1, err
 		}
-		_, err = doDoltCommit(ctx, commitArgs)
+		_, _, err = doDoltCommit(ctx, commitArgs)
 		if err != nil {
 			return 1, err
 		}

integration-tests/bats/commit.bats

@@ -51,4 +51,25 @@ teardown() {
     run dolt commit
     [ $status -eq 1 ]
     [[ "$output" =~ "Failed to open commit editor" ]] || false
+}
+
+@test "commit: --skip-empty correctly skips committing when no changes are staged" {
+  dolt sql -q "create table t(pk int primary key);"
+  dolt add t
+  original_head=$(get_head_commit)
+
+  # When --allow-empty and --skip-empty are both specified, the user should get an error
+  run dolt commit --allow-empty --skip-empty -m "commit message"
+  [ $status -eq 1 ]
+  [[ "$output" =~ 'error: cannot use both --allow-empty and --skip-empty' ]] || false
+  [ $original_head = $(get_head_commit) ]
+
+  # When changes are staged, --skip-empty has no effect
+  dolt commit --skip-empty -m "commit message"
+  new_head=$(get_head_commit)
+  [ $original_head != $new_head ]
+
+  # When no changes are staged, --skip-empty skips creating the commit
+  dolt commit --skip-empty -m "commit message"
+  [ $new_head = $(get_head_commit) ]
 }

integration-tests/bats/helper/common.bash

@@ -38,6 +38,11 @@ current_dolt_user_email() {
     dolt config --global --get user.email
 }
 
+# get_head_commit returns the commit hash for the current HEAD
+get_head_commit() {
+    dolt log -n 1 | grep -m 1 commit | cut -c 13-44
+}
+
 setup_no_dolt_init() {
     export PATH=$PATH:~/go/bin
     cd $BATS_TMPDIR

integration-tests/bats/sql-commit.bats

@@ -453,6 +453,21 @@ SQL
     [[ "$output" =~ 'error: no value for option `message' ]] || false
 }
 
-get_head_commit() {
-    dolt log -n 1 | grep -m 1 commit | cut -c 13-44
+@test "sql-commit: --skip-empty correctly skips committing when no changes are staged" {
+  original_head=$(get_head_commit)
+
+  # When --allow-empty and --skip-empty are both specified, the user should get an error
+  run dolt sql -q "CALL DOLT_COMMIT('--allow-empty', '--skip-empty', '-m', 'commit message');"
+  [ $status -eq 1 ]
+  [[ "$output" =~ 'error: cannot use both --allow-empty and --skip-empty' ]] || false
+  [ $original_head = $(get_head_commit) ]
+
+  # When changes are staged, --skip-empty has no effect
+  dolt sql -q "CALL DOLT_COMMIT('--skip-empty', '-m', 'commit message');"
+  new_head=$(get_head_commit)
+  [ $original_head != $new_head ]
+
+  # When no changes are staged, --skip-empty skips creating the commit
+  dolt sql -q "CALL DOLT_COMMIT('--skip-empty', '-m', 'commit message');"
+  [ $new_head = $(get_head_commit) ]
 }