From 9ab6eb2117d41154c0318f930ece25463d11e943 Mon Sep 17 00:00:00 2001
From: Chris Kim <chriskim06@gmail.com>
Date: Thu, 23 Apr 2020 11:02:12 -0700
Subject: [PATCH] Code review changes

---
 cmd/krew/cmd/namingutils_test.go   | 29 ++++++++++++++++++++++-------
 cmd/krew/cmd/uninstall.go          |  3 +--
 cmd/krew/cmd/upgrade.go            |  3 +--
 integration_test/uninstall_test.go | 19 ++++++++++++++++---
 integration_test/upgrade_test.go   |  7 +++++++
 5 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/cmd/krew/cmd/namingutils_test.go b/cmd/krew/cmd/namingutils_test.go
index 3333b325..f883373a 100644
--- a/cmd/krew/cmd/namingutils_test.go
+++ b/cmd/krew/cmd/namingutils_test.go
@@ -114,13 +114,28 @@ func Test_canonicalName(t *testing.T) {
 }
 
 func Test_isCanonicalName(t *testing.T) {
-	if isCanonicalName("foo") {
-		t.Error("expected foo to not be considered a canonical name")
-	}
-	if isCanonicalName("../index/foo") {
-		t.Error("expected ../index/foo to not be considered a canonical name")
+	tests := []struct {
+		arg      string
+		expected bool
+	}{
+		{"foo", false},
+		{"../index/foo", false},
+		{"index/foo", true},
+		{"", false},
+		{"0-0", false},
+		{"a/", false},
+		{"/b", false},
+		{"a-a/b-b", true},
+		{"a//b", false},
+		{"a / b", false},
+		{"a /b", false},
+		{"a/ b", false},
 	}
-	if !isCanonicalName("index/foo") {
-		t.Error("expected index/foo to be considered a canonical name")
+	for _, tt := range tests {
+		t.Run(tt.arg, func(t *testing.T) {
+			if isCanonicalName(tt.arg) != tt.expected {
+				t.Errorf("expected isCanonicalName(%q) to be %t", tt.arg, tt.expected)
+			}
+		})
 	}
 }
diff --git a/cmd/krew/cmd/uninstall.go b/cmd/krew/cmd/uninstall.go
index 480b41b7..fa9f1534 100644
--- a/cmd/krew/cmd/uninstall.go
+++ b/cmd/krew/cmd/uninstall.go
@@ -41,8 +41,7 @@ Remarks:
 		for _, name := range args {
 			if isCanonicalName(name) {
 				return errors.New("uninstall command does not support INDEX/PLUGIN syntax; just specify PLUGIN")
-			}
-			if !validation.IsSafePluginName(name) {
+			} else if !validation.IsSafePluginName(name) {
 				return unsafePluginNameErr(name)
 			}
 			klog.V(4).Infof("Going to uninstall plugin %s\n", name)
diff --git a/cmd/krew/cmd/upgrade.go b/cmd/krew/cmd/upgrade.go
index 938cfdb5..cacf642c 100644
--- a/cmd/krew/cmd/upgrade.go
+++ b/cmd/krew/cmd/upgrade.go
@@ -63,8 +63,7 @@ kubectl krew upgrade foo bar"`,
 				for _, arg := range args {
 					if isCanonicalName(arg) {
 						return errors.New("upgrade command does not support INDEX/PLUGIN syntax; just specify PLUGIN")
-					}
-					if !validation.IsSafePluginName(arg) {
+					} else if !validation.IsSafePluginName(arg) {
 						return unsafePluginNameErr(arg)
 					}
 					r, err := receipt.Load(paths.PluginInstallReceiptPath(arg))
diff --git a/integration_test/uninstall_test.go b/integration_test/uninstall_test.go
index 47fd2c94..04625c22 100644
--- a/integration_test/uninstall_test.go
+++ b/integration_test/uninstall_test.go
@@ -57,13 +57,26 @@ func TestKrewUninstallPluginsFromCustomIndex(t *testing.T) {
 	test.Krew("index", "add", "foo", test.TempDir().Path("index/"+constants.DefaultIndexName)).RunOrFail()
 	test.Krew("install", "foo/"+validPlugin).RunOrFail()
 
-	if _, err := test.Krew("uninstall", "foo/"+validPlugin).Run(); err == nil {
-		t.Error("expected error when uninstalling by canonical name")
-	}
 	test.Krew("uninstall", validPlugin).RunOrFail()
 	test.AssertExecutableNotInPATH("kubectl-" + validPlugin)
 }
 
+func TestKrewUninstall_CannotUseIndexSyntax(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithEnv(constants.EnableMultiIndexSwitch, 1).WithIndex()
+	out, err := test.Krew("uninstall", "foo/"+validPlugin).Run()
+	if err == nil {
+		t.Error("expected error when uninstalling by canonical name")
+	}
+	if !strings.Contains(string(out), "INDEX/PLUGIN") {
+		t.Error("expected warning about using canonical name to be in output")
+	}
+}
+
 func TestKrewRemove_AliasSupported(t *testing.T) {
 	skipShort(t)
 
diff --git a/integration_test/upgrade_test.go b/integration_test/upgrade_test.go
index 494bef22..4e54bdb0 100644
--- a/integration_test/upgrade_test.go
+++ b/integration_test/upgrade_test.go
@@ -73,6 +73,13 @@ func TestKrewUpgradePluginsFromCustomIndex(t *testing.T) {
 	if !strings.Contains(out, "Upgrading plugin: foo/"+validPlugin) {
 		t.Errorf("expected plugin foo/%s to be upgraded", validPlugin)
 	}
+}
+
+func TestKrewUpgrade_CannotUseIndexSyntax(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
 
 	b, err := test.Krew("upgrade", "foo/"+validPlugin).Run()
 	if err == nil {