From 9e7b3a1bb56ef0ee50be5f4d871095a83479658f Mon Sep 17 00:00:00 2001
From: Tyler Butters <dapegral@gmail.com>
Date: Sun, 8 Oct 2017 12:47:12 -0400
Subject: [PATCH] Improve common mistake when using Cobra flagsets

Users get tripped up when accidentally mix & matching
Cobra's `Flags()` and `PersistentFlags()`, where they will
call `Lookup(<flag>)` on the wrong flagset.

Following the README, users would call `Lookup` to fill in
the flag source parameter of Viper's `BindPFlag`, but by
using the wrong flagset, the parameter would simply be nil.
This would then result in a runtime panic whenever trying
to retrieve the flag's value with `Get(<flag>)`.

This work alleviates the mistake by:
+ Performing a nil check in `BindPFlag` and causing
  `BindFlagValue` to return an error if the parameter
  flag is nil, as it would be during the expected mixup.
+ Updating the godoc of `BindPFlag` and the README's example,
  to drill the fact that `Flags` and `PersistentFlags` are
  different flagsets.
+ Adding a test for this pattern of binding nil on accident
  and then retrieving flags. Before these changes, this test
  would panic. Now, the test instead gracefully errors and
  applies no changes to the viper instance.
---
 README.md     | 5 +++++
 viper.go      | 8 +++++++-
 viper_test.go | 9 +++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 848d92d..bcccd67 100644
--- a/README.md
+++ b/README.md
@@ -246,6 +246,11 @@ serverCmd.Flags().Int("port", 1138, "Port to run Application server on")
 viper.BindPFlag("port", serverCmd.Flags().Lookup("port"))
 ```
 
+The above example would also work with
+[Cobra's Persistent Flags](https://github.com/spf13/cobra#persistent-flags),
+but careful not to mix and match Cobra `Flags` and `PersistentFlags` when
+binding with Viper.
+
 You can also bind an existing set of pflags (pflag.FlagSet):
 
 Example:
diff --git a/viper.go b/viper.go
index 963861a..9b980e0 100644
--- a/viper.go
+++ b/viper.go
@@ -799,9 +799,15 @@ func (v *Viper) BindPFlags(flags *pflag.FlagSet) error {
 //	 serverCmd.Flags().Int("port", 1138, "Port to run Application server on")
 //	 Viper.BindPFlag("port", serverCmd.Flags().Lookup("port"))
 //
+// NOTE: Be consistent with either Cobra's Flags() or PersistentFlags(),
+//       do not mix them.
 func BindPFlag(key string, flag *pflag.Flag) error { return v.BindPFlag(key, flag) }
 func (v *Viper) BindPFlag(key string, flag *pflag.Flag) error {
-	return v.BindFlagValue(key, pflagValue{flag})
+	var flagVal FlagValue
+	if flag != nil {
+		flagVal = pflagValue{flag}
+	}
+	return v.BindFlagValue(key, flagVal)
 }
 
 // BindFlagValues binds a full FlagValue set to the configuration, using each flag's long
diff --git a/viper_test.go b/viper_test.go
index 774ca11..e1f850a 100644
--- a/viper_test.go
+++ b/viper_test.go
@@ -597,6 +597,15 @@ func TestBindPFlag(t *testing.T) {
 
 }
 
+func TestBindPFlagToNil(t *testing.T) {
+	v := New()
+	// nil arg often comes as an accident from calling something like:
+	//     `myCobraCmd.Flags().Lookup("missingFlag")`
+	err := v.BindPFlag("host", nil)
+	assert.Equal(t, nil, v.Get("host")) // `Get` here used to panic
+	assert.EqualError(t, err, `flag for "host" is nil`)
+}
+
 func TestBoundCaseSensitivity(t *testing.T) {
 	assert.Equal(t, "brown", Get("eyes"))