Don't prompt for password if explicitly set to blank string. Fixes #73

This commit fixes edge cases where the behavior of Skeema's password option
did not exactly match the behavior of the MySQL command-line client. In
particular, this caused problems with Travis CI's default ~/.my.cnf file.

These command-line invocations now correctly use no password, rather than
prompting from a TTY:
* --password=
* --password=''
* --password=""

Ditto for these option file lines:
* password=
* password=''
* password=""

In other words, Skeema only prompts for a password on STDIN if no equals sign
is found immediately after the password option.

Gopkg.lock

@@ -154,8 +154,8 @@
 [[projects]]
   name = "github.com/skeema/mybase"
   packages = ["."]
-  revision = "8116f0ad0e3bdd0889061d636547492d0744e016"
-  version = "v1.0.5"
+  revision = "488acfe7d1bc0916d1f58b1cc549e74b0eb056fd"
+  version = "v1.0.6"
 
 [[projects]]
   name = "github.com/skeema/tengo"

skeema.go

@@ -16,7 +16,7 @@ schema to the filesystem, and apply online schema changes by modifying files.`
 
 // Globals overridden by GoReleaser's ldflags
 var (
-	version = "1.2.2-dev"
+	version = "1.2.3-dev"
 	commit  = "unknown"
 	date    = "unknown"
 )

util/config.go

@@ -31,7 +31,7 @@ func AddGlobalOptions(cmd *mybase.Command) {
 
 	// Visible global options
 	cmd.AddOption(mybase.StringOption("user", 'u', "root", "Username to connect to database host"))
-	cmd.AddOption(mybase.StringOption("password", 'p', "<no password>", "Password for database user; supply with no value to prompt").ValueOptional())
+	cmd.AddOption(mybase.StringOption("password", 'p', "", "Password for database user; omit value to prompt from TTY (default no password)").ValueOptional())
 	cmd.AddOption(mybase.StringOption("host-wrapper", 'H', "", "External bin to shell out to for host lookup; see manual for template vars"))
 	cmd.AddOption(mybase.StringOption("temp-schema", 't', "_skeema_tmp", "Name of temporary schema for intermediate operations, created and dropped each run unless --reuse-temp-schema"))
 	cmd.AddOption(mybase.StringOption("connect-options", 'o', "", "Comma-separated session options to set upon connecting to each database instance"))
@@ -107,14 +107,14 @@ func ProcessSpecialGlobalOptions(cfg *mybase.Config) error {
 	}
 
 	// Special handling for password option: if not supplied at all, check env
-	// var instead. Or if supplied but with no value (empty string, instead of
-	// its default of "<no password>"), prompt on STDIN like mysql client does.
+	// var instead. Or if supplied but with no equals sign or value, prompt on
+	// STDIN like mysql client does.
 	if !cfg.Supplied("password") {
 		if val := os.Getenv("MYSQL_PWD"); val != "" {
 			cfg.CLI.OptionValues["password"] = val
 			cfg.MarkDirty()
 		}
-	} else if cfg.Get("password") == "" {
+	} else if !cfg.SuppliedWithValue("password") {
 		var err error
 		cfg.CLI.OptionValues["password"], err = PromptPassword()
 		cfg.MarkDirty()

util/config_test.go

@@ -20,8 +20,8 @@ func TestAddGlobalConfigFiles(t *testing.T) {
 	// Expectation: global config files not existing isn't fatal
 	cfg := mybase.ParseFakeCLI(t, cmdSuite, "skeema diff")
 	AddGlobalConfigFiles(cfg)
-	if actualPassword := cfg.Get("password"); actualPassword != "<no password>" {
-		t.Errorf("Expected password to be unchanged from default; instead found %s", actualPassword)
+	if cfg.Supplied("password") || cfg.Changed("password") {
+		t.Errorf("Expected password to be unsupplied and unchanged from default; instead found %q", cfg.GetRaw("password"))
 	}
 
 	os.MkdirAll("fake-etc", 0777)
@@ -65,8 +65,8 @@ func TestAddGlobalConfigFiles(t *testing.T) {
 	if actualUser := cfg.Get("user"); actualUser != "two" {
 		t.Errorf("Expected user in fake-home/.my.cnf to take precedence; instead found %s", actualUser)
 	}
-	if actualPassword := cfg.Get("password"); actualPassword != "<no password>" {
-		t.Errorf("Expected password to be unchanged from default; instead found %s", actualPassword)
+	if cfg.Supplied("password") || cfg.Changed("password") {
+		t.Errorf("Expected password to be unsupplied and unchanged from default; instead found %q", cfg.GetRaw("password"))
 	}
 }
 
@@ -127,7 +127,8 @@ func TestPasswordOption(t *testing.T) {
 		t.Errorf("Expected password to be howdyplanet, instead found %s", cfg.Get("password"))
 	}
 
-	// ProcessSpecialGlobalOptions should error if STDIN isn't TTY
+	// ProcessSpecialGlobalOptions should error with valueless password if STDIN
+	// isn't a TTY. Test bare "password" (no =) on both CLI and config file.
 	oldStdin := os.Stdin
 	defer func() {
 		os.Stdin = oldStdin
@@ -140,6 +141,29 @@ func TestPasswordOption(t *testing.T) {
 	if err := ProcessSpecialGlobalOptions(cfg); err == nil {
 		t.Error("Expected ProcessSpecialGlobalOptions to return an error for non-TTY STDIN, but it did not")
 	}
+	cfg = mybase.ParseFakeCLI(t, cmdSuite, "skeema diff --password", fakeFileSource)
+	if err := ProcessSpecialGlobalOptions(cfg); err == nil {
+		t.Error("Expected ProcessSpecialGlobalOptions to return an error for non-TTY STDIN, but it did not")
+	}
+	fakeFileSource["password"] = ""
+	cfg = mybase.ParseFakeCLI(t, cmdSuite, "skeema diff", fakeFileSource)
+	if err := ProcessSpecialGlobalOptions(cfg); err == nil {
+		t.Error("Expected ProcessSpecialGlobalOptions to return an error for non-TTY STDIN, but it did not")
+	}
+
+	// Setting password to an empty string explicitly should not trigger TTY prompt
+	// (note: STDIN intentionally still points to a file here, from test above)
+	cfg = mybase.ParseFakeCLI(t, cmdSuite, "skeema diff --password=")
+	if err := ProcessSpecialGlobalOptions(cfg); err != nil {
+		t.Errorf("Unexpected error from ProcessSpecialGlobalOptions: %v", err)
+	}
+	if cfg.Changed("password") {
+		t.Error("Password unexpectedly considered changed from default")
+	}
+	cfg = mybase.ParseFakeCLI(t, cmdSuite, "skeema diff --password=''")
+	if err := ProcessSpecialGlobalOptions(cfg); err != nil {
+		t.Errorf("Unexpected error from ProcessSpecialGlobalOptions: %v", err)
+	}
 }
 
 func TestSplitConnectOptions(t *testing.T) {

vendor/github.com/skeema/mybase/cli.go

@@ -47,6 +47,12 @@ func (cli *CommandLine) parseLongArg(arg string, args *[]string, longOptionIndex
 			// Boolean without value is treated as true
 			value = "1"
 		}
+	} else if value == "" && opt.Type == OptionTypeString {
+		// Convert empty strings into quote-wrapped empty strings, so that callers
+		// may differentiate between bare "--foo" vs "--foo=" if desired, by using
+		// Config.GetRaw(). Meanwhile Config.Get and most other getters strip
+		// surrounding quotes, so this does not break anything.
+		value = "''"
 	}
 
 	cli.OptionValues[opt.Name] = value

vendor/github.com/skeema/mybase/command.go

@@ -271,7 +271,7 @@ func helpHandler(cfg *Config) error {
 	}
 	var forCommandName string
 	if len(cfg.CLI.ArgValues) > 0 {
-		forCommandName = cfg.CLI.ArgValues[0]
+		forCommandName = unquote(cfg.CLI.ArgValues[0])
 	}
 	if len(forCommand.SubCommands) > 0 && forCommandName != "" {
 		var ok bool

vendor/github.com/skeema/mybase/config.go

@@ -156,7 +156,7 @@ func (cfg *Config) MarkDirty() {
 }
 
 // Changed returns true if the specified option name has been set, and its
-// set value differs from the option's default value.
+// set value (after unquoting) differs from the option's default value.
 func (cfg *Config) Changed(name string) bool {
 	if !cfg.Supplied(name) {
 		return false
@@ -164,7 +164,7 @@ func (cfg *Config) Changed(name string) bool {
 	opt := cfg.FindOption(name)
 	// Note that opt cannot be nil here, so no need to check. If the name didn't
 	// correspond to an existing option, the previous call to Supplied panics.
-	return (cfg.unifiedValues[name] != opt.Default)
+	return (unquote(cfg.unifiedValues[name]) != opt.Default)
 }
 
 // Supplied returns true if the specified option name has been set by some
@@ -186,6 +186,24 @@ func (cfg *Config) Supplied(name string) bool {
 	}
 }
 
+// SuppliedWithValue returns true if the specified option name has been set by
+// some configuration source AND had a value specified, even if that value was
+// a blank string. For example, this returns true even for "--foo=''" or
+// "--foo=" on a command line, or "foo=''" or "foo=" in an option file. Returns
+// false for bare "--foo" on CLI or bare "foo" in an option file.
+// This method is only usable on OptionTypeString options with !RequireValue.
+// Panics if the supplied option name does not meet those requirements.
+func (cfg *Config) SuppliedWithValue(name string) bool {
+	opt := cfg.FindOption(name)
+	if opt.Type != OptionTypeString || opt.RequireValue {
+		panic(fmt.Errorf("Assertion failed: SuppliedWithValue called on wrong kind of option %s", name))
+	}
+	if !cfg.Supplied(name) {
+		return false
+	}
+	return cfg.GetRaw(name) != ""
+}
+
 // OnCLI returns true if the specified option name was set on the command-line,
 // or false otherwise. If the option does not exist, panics to indicate
 // programmer error.

vendor/github.com/skeema/mybase/file.go

@@ -198,6 +198,12 @@ func (f *File) Parse(cfg *Config) error {
 					// For booleans, option without value indicates option is being enabled
 					parsedLine.value = "1"
 				}
+			} else if parsedLine.value == "" && opt.Type == OptionTypeString {
+				// Convert empty strings into quote-wrapped empty strings, so that callers
+				// may differentiate between bare "foo" vs "foo=" if desired, by using
+				// Config.GetRaw(). Meanwhile Config.Get and most other getters strip
+				// surrounding quotes, so this does not break anything.
+				parsedLine.value = "''"
 			}
 			section.Values[parsedLine.key] = parsedLine.value
 		}