Merge pull request #9505 from dolthub/nathan/diffsystem

Dolt diff --system flag

go/cmd/dolt/commands/diff.go

@@ -179,6 +179,7 @@ func (cmd DiffCmd) ArgParser() *argparser.ArgParser {
 	ap.SupportsString(DiffMode, "", "diff mode", "Determines how to display modified rows with tabular output. Valid values are row, line, in-place, context. Defaults to context.")
 	ap.SupportsFlag(ReverseFlag, "R", "Reverses the direction of the diff.")
 	ap.SupportsFlag(NameOnlyFlag, "", "Only shows table names.")
+	ap.SupportsFlag(cli.SystemFlag, "", "Show system tables in addition to user tables")
 	return ap
 }
 
@@ -199,8 +200,7 @@ func (cmd DiffCmd) Exec(ctx context.Context, commandStr string, args []string, _
 		return HandleVErrAndExitCode(verr, usage)
 	}
 
-	queryist, oldSqlCtx, closeFunc, err := cliCtx.QueryEngine(ctx)
-	sqlCtx := doltdb.ContextWithDoltCICreateBypassKey(oldSqlCtx)
+	queryist, sqlCtx, closeFunc, err := cliCtx.QueryEngine(ctx)
 	if err != nil {
 		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
 	}
@@ -208,13 +208,62 @@ func (cmd DiffCmd) Exec(ctx context.Context, commandStr string, args []string, _
 		defer closeFunc()
 	}
 
+	updateSystemVar, err := setSystemVar(queryist, sqlCtx, apr.Contains(cli.SystemFlag))
+	if err != nil {
+		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
+	}
+
 	dArgs, err := parseDiffArgs(queryist, sqlCtx, apr)
 	if err != nil {
 		return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
 	}
 
 	verr = diffUserTables(queryist, sqlCtx, dArgs)
-	return HandleVErrAndExitCode(verr, usage)
+	if verr != nil {
+		return HandleVErrAndExitCode(verr, usage)
+	}
+
+	if updateSystemVar != nil {
+		err = updateSystemVar()
+	}
+
+	return HandleVErrAndExitCode(errhand.VerboseErrorFromError(err), usage)
+}
+
+// setSystemVar sets the @@dolt_show_system_tables variable if necessary, and returns the value it must be
+// set to after the commands completion, if necessary.
+func setSystemVar(queryist cli.Queryist, sqlCtx *sql.Context, showSystem bool) (func() error, error) {
+	_, rowIter, _, err := queryist.Query(sqlCtx, "SHOW VARIABLES WHERE VARIABLE_NAME='dolt_show_system_tables'")
+	if err != nil {
+		return nil, err
+	}
+
+	row, err := sql.RowIterToRows(sqlCtx, rowIter)
+	if err != nil {
+		return nil, err
+	}
+	prevVal, err := GetInt8ColAsBool(row[0][1])
+	if err != nil {
+		return nil, err
+	}
+
+	newVal := false
+	var update func() error
+
+	if showSystem {
+		newVal = true
+	}
+	if newVal != prevVal {
+		query := fmt.Sprintf("SET @@dolt_show_system_tables = %t", newVal)
+		_, _, _, err = queryist.Query(sqlCtx, query)
+		update = func() error {
+			query := fmt.Sprintf("SET @@dolt_show_system_tables = %t", prevVal)
+			_, _, _, err := queryist.Query(sqlCtx, query)
+			return err
+		}
+	}
+
+	return update, err
 }
 
 func (cmd DiffCmd) validateArgs(apr *argparser.ArgParseResults) errhand.VerboseError {

go/cmd/dolt/commands/utils.go

@@ -363,6 +363,20 @@ func GetTinyIntColAsBool(col interface{}) (bool, error) {
 	}
 }
 
+// GetInt8ColAsBool returns the value of an int8 column as a bool
+// This is necessary because Queryist may return an int8 column as a bool (when using SQLEngine)
+// or as a string (when using ConnectionQueryist).
+func GetInt8ColAsBool(col interface{}) (bool, error) {
+	switch v := col.(type) {
+	case int8:
+		return v != 0, nil
+	case string:
+		return v != "0", nil
+	default:
+		return false, fmt.Errorf("unexpected type %T, was expecting int8", v)
+	}
+}
+
 // getInt64ColAsInt64 returns the value of an int64 column as a string
 // This is necessary because Queryist may return an int64 column as an int64 (when using SQLEngine)
 // or as a string (when using ConnectionQueryist).

go/libraries/doltcore/doltdb/system_table.go

@@ -82,7 +82,7 @@ func DoltCICanBypass(ctx context.Context) bool {
 // HasDoltPrefix returns a boolean whether or not the provided string is prefixed with the DoltNamespace. Users should
 // not be able to create tables in this reserved namespace.
 func HasDoltPrefix(s string) bool {
-	return strings.HasPrefix(strings.ToLower(s), DoltNamespace) && !HasDoltCIPrefix(s)
+	return strings.HasPrefix(strings.ToLower(s), DoltNamespace)
 }
 
 // HasDoltCIPrefix returns a boolean whether or not the provided string is prefixed with the DoltCINamespace. Users should

go/libraries/doltcore/sqle/database.go

@@ -976,7 +976,7 @@ func (db Database) GetTableNamesAsOf(ctx *sql.Context, time interface{}) ([]stri
 		return nil, err
 	}
 
-	return filterDoltInternalTables(ctx, tblNames, db.schemaName), nil
+	return filterDoltInternalTables(tblNames, db.schemaName, showSystemTables), nil
 }
 
 // getTable returns the user table with the given baseName from the root given
@@ -1194,7 +1194,7 @@ func (db Database) GetTableNames(ctx *sql.Context) ([]string, error) {
 	}
 
 	// TODO: Figure out way to remove filterDoltInternalTables
-	return filterDoltInternalTables(ctx, tblNames, db.schemaName), nil
+	return filterDoltInternalTables(tblNames, db.schemaName, showSystemTables), nil
 }
 
 func (db Database) SchemaName() string {
@@ -1245,14 +1245,12 @@ func (db Database) getAllTableNames(ctx *sql.Context, root doltdb.RootValue, inc
 	return result, nil
 }
 
-func filterDoltInternalTables(ctx *sql.Context, tblNames []string, schemaName string) []string {
+func filterDoltInternalTables(tblNames []string, schemaName string, includeWriteable bool) []string {
 	result := []string{}
 
 	for _, tbl := range tblNames {
-		if doltdb.IsDoltCITable(tbl) {
-			if doltdb.DoltCICanBypass(ctx) {
-				result = append(result, tbl)
-			}
+		if includeWriteable && !doltdb.IsReadOnlySystemTable(doltdb.TableName{Name: tbl, Schema: schemaName}) {
+			result = append(result, tbl)
 		} else if !doltdb.IsSystemTable(doltdb.TableName{Name: tbl, Schema: schemaName}) {
 			result = append(result, tbl)
 		}
@@ -1435,7 +1433,7 @@ func (db Database) CreateTable(ctx *sql.Context, tableName string, sch sql.Prima
 		return err
 	}
 
-	if doltdb.IsSystemTable(doltdb.TableName{Name: tableName, Schema: db.schemaName}) && !doltdb.IsFullTextTable(tableName) {
+	if doltdb.IsSystemTable(doltdb.TableName{Name: tableName, Schema: db.schemaName}) && !doltdb.IsFullTextTable(tableName) && !doltdb.HasDoltCIPrefix(tableName) {
 		return ErrReservedTableName.New(tableName)
 	}
 

integration-tests/bats/ci.bats

@@ -117,8 +117,7 @@ get_commit_hash() {
 
     dolt ci init
     last=$(get_commit_hash 1)
-    dolt diff "$first" "$last"
-    run dolt diff "$first" "$last"
+    run dolt diff "$first" "$last" --system
     [ "$status" -eq 0 ]
     [[ ${output} == *"dolt_ci"* ]] || false
 }
@@ -224,7 +223,7 @@ jobs:
 EOF
     dolt ci import ./workflow_1_updated.yaml
     updated=$(get_commit_hash 1)
-    run dolt diff "$original" "$updated"
+    run dolt diff "$original" "$updated" --system
     [ "$status" -eq 0 ]
     [[ ${output} == *"(new)"* ]] || false
     [[ ${output} == *"dolt_ci_workflow_steps"* ]] || false

integration-tests/bats/diff-system.expect

@@ -0,0 +1,30 @@
+#!/usr/bin/expect
+
+set timeout 5
+set env(NO_COLOR) 1
+
+source  "$env(BATS_CWD)/helper/common_expect_functions.tcl"
+
+spawn dolt sql
+
+expect_with_defaults                                                   {dolt-repo-.*>} { send "\\diff HEAD HEAD~1 --system\r"; }
+
+expect_with_defaults                                                   {dolt-repo-.*>} { send "show tables;\r"; }
+
+expect {
+    -re {dolt_log} {
+        puts "\diff did not preserve existing session variable @@dolt_show_system_tables"
+        exit 1
+    }
+}
+
+expect_with_defaults                                                   {dolt-repo-.*>}   { send "SET @@dolt_show_system_tables = 1;\r"; }
+
+expect_with_defaults                                                   {dolt-repo-.*>} { send "\\diff HEAD HEAD~1 \r"; }
+
+expect_with_defaults                                                   {dolt-repo-.*>} { send "show tables;\r"; }
+
+expect_with_defaults_2 {dolt_log}                                      {dolt-repo-.*>}   { send "quit;\r"; }
+
+expect eof
+exit

integration-tests/bats/diff.bats

@@ -2183,4 +2183,27 @@ EOF
     [[ "$output" =~ "$EXPECTED" ]] || false
     # Count the line numbers to make sure there are no schema changes output
     [ "${#lines[@]}" -eq 3 ]
+}
+
+@test "diff: diff only shows system tables with --system flag" {
+    dolt sql -q "insert into dolt_ignore values ('test', 1)"
+    dolt add . 
+    dolt commit -m "added row to dolt_ignore"
+
+    run dolt diff HEAD HEAD~1
+    [ "$status" -eq 0 ]
+    ! [[ "$output" =~ "dolt_ignore" ]] || false
+
+    run dolt diff HEAD HEAD~1 --system
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "dolt_ignore" ]] || false
+}
+
+# bats test_tags=no_lambda
+@test "diff: --system preserves dolt_show_system_tables value in sql-shell" {
+     skiponwindows "Need to install expect and make this script work on windows."
+     dolt commit --allow-empty -m "Commit"
+
+     run $BATS_TEST_DIRNAME/diff-system.expect
+     [ "$status" -eq 0 ]
 }