Repository: cli/cli
Pull request #8211 (2023-10-18 10:20:31)

`gh extension install` not give an error if extension already installed and a clear message
<!--
  Thank you for contributing to GitHub CLI!
  To reference an open issue, please write this in your description: `Fixes #NUMBER`
-->

Fixes https://github.com/cli/cli/issues/4463 and supercedes https://github.com/cli/cli/pull/4536

Delta456: A terminal screenshot of my change:

![image](https://github.com/cli/cli/assets/28479139/bb8c9c3b-ceff-46f1-a2d0-af33abf5909e)


Patch:
diff --git a/go.mod b/go.mod
index 7123e69cc..252405f72 100644
--- a/go.mod
+++ b/go.mod
@@ -25,7 +25,7 @@ require (
 	github.com/joho/godotenv v1.5.1
 	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/mattn/go-colorable v0.1.13
-	github.com/mattn/go-isatty v0.0.19
+	github.com/mattn/go-isatty v0.0.20
 	github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d
 	github.com/microsoft/dev-tunnels v0.0.25
 	github.com/muhammadmuzzammil1998/jsonc v0.0.0-20201229145248-615b0916ca38
diff --git a/go.sum b/go.sum
index 45f962146..00876d819 100644
--- a/go.sum
+++ b/go.sum
@@ -32,7 +32,6 @@ github.com/cli/safeexec v1.0.1 h1:e/C79PbXF4yYTN/wauC4tviMxEV13BwljGj0N9j+N00=
 github.com/cli/safeexec v1.0.1/go.mod h1:Z/D4tTN8Vs5gXYHDCbaM1S/anmEDnJb1iW0+EJ5zx3Q=
 github.com/cli/shurcooL-graphql v0.0.3 h1:CtpPxyGDs136/+ZeyAfUKYmcQBjDlq5aqnrDCW5Ghh8=
 github.com/cli/shurcooL-graphql v0.0.3/go.mod h1:tlrLmw/n5Q/+4qSvosT+9/W5zc8ZMjnJeYBxSdb4nWA=
-github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w=
 github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
 github.com/cpuguy83/go-md2man/v2 v2.0.3 h1:qMCsGGgs+MAzDFyp9LpAe1Lqy/fY/qCovCm0qnXZOBM=
 github.com/cpuguy83/go-md2man/v2 v2.0.3/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
@@ -103,8 +102,8 @@ github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovk
 github.com/mattn/go-isatty v0.0.8/go.mod h1:Iq45c/XA43vh69/j3iqttzPXn0bhXyGjM0Hdxcsrc5s=
 github.com/mattn/go-isatty v0.0.14/go.mod h1:7GGIvUiUoEMVVmxf/4nioHXj79iQHKdU27kJ6hsGG94=
 github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
-github.com/mattn/go-isatty v0.0.19 h1:JITubQf0MOLdlGRuRq+jtsDlekdYPia9ZFsB8h/APPA=
-github.com/mattn/go-isatty v0.0.19/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
+github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=
+github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y=
 github.com/mattn/go-runewidth v0.0.9/go.mod h1:H031xJmbD/WCDINGzjvQ9THkh0rPKHF+m2gUSrubnMI=
 github.com/mattn/go-runewidth v0.0.10/go.mod h1:RAqKPSqVFrSLVXbA8x7dzmKdmGzieGRCM46jaSJTDAk=
 github.com/mattn/go-runewidth v0.0.12/go.mod h1:RAqKPSqVFrSLVXbA8x7dzmKdmGzieGRCM46jaSJTDAk=
diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go
index 6629e599d..4348e2cd3 100644
--- a/pkg/cmd/extension/command.go
+++ b/pkg/cmd/extension/command.go
@@ -22,6 +22,8 @@ import (
 	"github.com/spf13/cobra"
 )
 
+var alreadyInstalledError = errors.New("alreadyInstalledError")
+
 func NewCmdExtension(f *cmdutil.Factory) *cobra.Command {
 	m := f.ExtensionManager
 	io := f.IOStreams
@@ -333,16 +335,22 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command {
 						return err
 					}
 
-					if ext, err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil {
+					cs := io.ColorScheme()
+
+					if ext, err := checkValidExtension(cmd.Root(), m, repo.RepoName(), repo.RepoOwner()); err != nil {
 						// If an existing extension was found and --force was specified, attempt to upgrade.
 						if forceFlag && ext != nil {
 							return upgradeFunc(ext.Name(), forceFlag, false)
 						}
 
+						if errors.Is(err, alreadyInstalledError) {
+							fmt.Fprintf(io.ErrOut, "%s Extension %s is already installed\n", cs.WarningIcon(), ghrepo.FullName(repo))
+							return nil
+						}
+
 						return err
 					}
 
-					cs := io.ColorScheme()
 					if err := m.Install(repo, pinFlag); err != nil {
 						if errors.Is(err, releaseNotFoundErr) {
 							return fmt.Errorf("%s Could not find a release of %s for %s",
@@ -637,7 +645,7 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command {
 	return &extCmd
 }
 
-func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, extName string) (extensions.Extension, error) {
+func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, extName, extOwner string) (extensions.Extension, error) {
 	if !strings.HasPrefix(extName, "gh-") {
 		return nil, errors.New("extension repository name must start with `gh-`")
 	}
@@ -649,6 +657,9 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager,
 
 	for _, ext := range m.List() {
 		if ext.Name() == commandName {
+			if ext.Owner() == extOwner {
+				return ext, alreadyInstalledError
+			}
 			return ext, fmt.Errorf("there is already an installed extension that provides the %q command", commandName)
 		}
 	}
diff --git a/pkg/cmd/extension/extension.go b/pkg/cmd/extension/extension.go
index 188687634..dc7ffa8c3 100644
--- a/pkg/cmd/extension/extension.go
+++ b/pkg/cmd/extension/extension.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/cli/cli/v2/git"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"gopkg.in/yaml.v3"
 )
@@ -36,6 +37,7 @@ type Extension struct {
 	isPinned       *bool
 	currentVersion string
 	latestVersion  string
+	owner          string
 }
 
 func (e *Extension) Name() string {
@@ -176,6 +178,38 @@ func (e *Extension) IsPinned() bool {
 	return *e.isPinned
 }
 
+func (e *Extension) Owner() string {
+	e.mu.RLock()
+	if e.owner != "" {
+		defer e.mu.RUnlock()
+		return e.owner
+	}
+	e.mu.RUnlock()
+
+	var owner string
+	switch e.kind {
+	case LocalKind:
+	case BinaryKind:
+		if manifest, err := e.loadManifest(); err == nil {
+			owner = manifest.Owner
+		}
+	case GitKind:
+		if remoteURL, err := e.gitClient.Config("remote.origin.url"); err == nil {
+			if url, err := git.ParseURL(strings.TrimSpace(string(remoteURL))); err == nil {
+				if repo, err := ghrepo.FromURL(url); err == nil {
+					owner = repo.RepoOwner()
+				}
+			}
+		}
+	}
+
+	e.mu.Lock()
+	e.owner = owner
+	e.mu.Unlock()
+
+	return e.owner
+}
+
 func (e *Extension) UpdateAvailable() bool {
 	if e.IsLocal() ||
 		e.CurrentVersion() == "" ||
diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go
index 2583472c0..0b94032df 100644
--- a/pkg/extensions/extension.go
+++ b/pkg/extensions/extension.go
@@ -25,6 +25,7 @@ type Extension interface {
 	UpdateAvailable() bool
 	IsBinary() bool
 	IsLocal() bool
+	Owner() string
 }
 
 //go:generate moq -rm -out manager_mock.go . ExtensionManager
diff --git a/pkg/extensions/extension_mock.go b/pkg/extensions/extension_mock.go
index 500603661..9b473e20a 100644
--- a/pkg/extensions/extension_mock.go
+++ b/pkg/extensions/extension_mock.go
@@ -35,6 +35,9 @@ var _ Extension = &ExtensionMock{}
 //			NameFunc: func() string {
 //				panic("mock out the Name method")
 //			},
+//			OwnerFunc: func() string {
+//				panic("mock out the Owner method")
+//			},
 //			PathFunc: func() string {
 //				panic("mock out the Path method")
 //			},
@@ -69,6 +72,9 @@ type ExtensionMock struct {
 	// NameFunc mocks the Name method.
 	NameFunc func() string
 
+	// OwnerFunc mocks the Owner method.
+	OwnerFunc func() string
+
 	// PathFunc mocks the Path method.
 	PathFunc func() string
 
@@ -98,6 +104,9 @@ type ExtensionMock struct {
 		// Name holds details about calls to the Name method.
 		Name []struct {
 		}
+		// Owner holds details about calls to the Owner method.
+		Owner []struct {
+		}
 		// Path holds details about calls to the Path method.
 		Path []struct {
 		}
@@ -114,6 +123,7 @@ type ExtensionMock struct {
 	lockIsPinned        sync.RWMutex
 	lockLatestVersion   sync.RWMutex
 	lockName            sync.RWMutex
+	lockOwner           sync.RWMutex
 	lockPath            sync.RWMutex
 	lockURL             sync.RWMutex
 	lockUpdateAvailable sync.RWMutex
@@ -281,6 +291,33 @@ func (mock *ExtensionMock) NameCalls() []struct {
 	return calls
 }
 
+// Owner calls OwnerFunc.
+func (mock *ExtensionMock) Owner() string {
+	if mock.OwnerFunc == nil {
+		panic("ExtensionMock.OwnerFunc: method is nil but Extension.Owner was just called")
+	}
+	callInfo := struct {
+	}{}
+	mock.lockOwner.Lock()
+	mock.calls.Owner = append(mock.calls.Owner, callInfo)
+	mock.lockOwner.Unlock()
+	return mock.OwnerFunc()
+}
+
+// OwnerCalls gets all the calls that were made to Owner.
+// Check the length with:
+//
+//	len(mockedExtension.OwnerCalls())
+func (mock *ExtensionMock) OwnerCalls() []struct {
+} {
+	var calls []struct {
+	}
+	mock.lockOwner.RLock()
+	calls = mock.calls.Owner
+	mock.lockOwner.RUnlock()
+	return calls
+}
+
 // Path calls PathFunc.
 func (mock *ExtensionMock) Path() string {
 	if mock.PathFunc == nil {


Test patch:
diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go
index fd693d7e5..094fa330a 100644
--- a/pkg/cmd/extension/command_test.go
+++ b/pkg/cmd/extension/command_test.go
@@ -239,7 +239,7 @@ func TestNewCmdExtension(t *testing.T) {
 			args: []string{"install", "owner/gh-existing-ext"},
 			managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
 				em.ListFunc = func() []extensions.Extension {
-					e := &Extension{path: "owner2/gh-existing-ext"}
+					e := &Extension{path: "owner2/gh-existing-ext", owner: "owner2"}
 					return []extensions.Extension{e}
 				}
 				return func(t *testing.T) {
@@ -250,6 +250,21 @@ func TestNewCmdExtension(t *testing.T) {
 			wantErr: true,
 			errMsg:  "there is already an installed extension that provides the \"existing-ext\" command",
 		},
+		{
+			name: "install an already installed extension",
+			args: []string{"install", "owner/gh-existing-ext"},
+			managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
+				em.ListFunc = func() []extensions.Extension {
+					e := &Extension{path: "owner/gh-existing-ext", owner: "owner"}
+					return []extensions.Extension{e}
+				}
+				return func(t *testing.T) {
+					calls := em.ListCalls()
+					assert.Equal(t, 1, len(calls))
+				}
+			},
+			wantStderr: "! Extension owner/gh-existing-ext is already installed\n",
+		},
 		{
 			name: "install local extension",
 			args: []string{"install", "."},
@@ -817,7 +832,7 @@ func TestNewCmdExtension(t *testing.T) {
 			managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) {
 				em.ListFunc = func() []extensions.Extension {
 					return []extensions.Extension{
-						&Extension{path: "owner/gh-hello"},
+						&Extension{path: "owner/gh-hello", owner: "owner"},
 					}
 				}
 				em.InstallFunc = func(_ ghrepo.Interface, _ string) error {
@@ -924,19 +939,22 @@ func Test_checkValidExtension(t *testing.T) {
 		ListFunc: func() []extensions.Extension {
 			return []extensions.Extension{
 				&extensions.ExtensionMock{
-					NameFunc: func() string { return "screensaver" },
+					OwnerFunc: func() string { return "monalisa" },
+					NameFunc:  func() string { return "screensaver" },
 				},
 				&extensions.ExtensionMock{
-					NameFunc: func() string { return "triage" },
+					OwnerFunc: func() string { return "monalisa" },
+					NameFunc:  func() string { return "triage" },
 				},
 			}
 		},
 	}
 
 	type args struct {
-		rootCmd *cobra.Command
-		manager extensions.ExtensionManager
-		extName string
+		rootCmd  *cobra.Command
+		manager  extensions.ExtensionManager
+		extName  string
+		extOwner string
 	}
 	tests := []struct {
 		name      string
@@ -946,42 +964,56 @@ func Test_checkValidExtension(t *testing.T) {
 		{
 			name: "valid extension",
 			args: args{
-				rootCmd: rootCmd,
-				manager: m,
-				extName: "gh-hello",
+				rootCmd:  rootCmd,
+				manager:  m,
+				extOwner: "monalisa",
+				extName:  "gh-hello",
 			},
 		},
 		{
 			name: "invalid extension name",
 			args: args{
-				rootCmd: rootCmd,
-				manager: m,
-				extName: "gherkins",
+				rootCmd:  rootCmd,
+				manager:  m,
+				extOwner: "monalisa",
+				extName:  "gherkins",
 			},
 			wantError: "extension repository name must start with `gh-`",
 		},
 		{
 			name: "clashes with built-in command",
 			args: args{
-				rootCmd: rootCmd,
-				manager: m,
-				extName: "gh-auth",
+				rootCmd:  rootCmd,
+				manager:  m,
+				extOwner: "monalisa",
+				extName:  "gh-auth",
 			},
 			wantError: "\"auth\" matches the name of a built-in command or alias",
 		},
 		{
 			name: "clashes with an installed extension",
 			args: args{
-				rootCmd: rootCmd,
-				manager: m,
-				extName: "gh-triage",
+				rootCmd:  rootCmd,
+				manager:  m,
+				extOwner: "cli",
+				extName:  "gh-triage",
 			},
 			wantError: "there is already an installed extension that provides the \"triage\" command",
 		},
+		{
+			name: "clashes with same extension",
+			args: args{
+				rootCmd:  rootCmd,
+				manager:  m,
+				extOwner: "monalisa",
+				extName:  "gh-triage",
+			},
+			wantError: "alreadyInstalledError",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			_, err := checkValidExtension(tt.args.rootCmd, tt.args.manager, tt.args.extName)
+			_, err := checkValidExtension(tt.args.rootCmd, tt.args.manager, tt.args.extName, tt.args.extOwner)
 			if tt.wantError == "" {
 				assert.NoError(t, err)
 			} else {
diff --git a/pkg/cmd/extension/extension_test.go b/pkg/cmd/extension/extension_test.go
index 9b56e0bee..6f0afc3a6 100644
--- a/pkg/cmd/extension/extension_test.go
+++ b/pkg/cmd/extension/extension_test.go
@@ -1,6 +1,7 @@
 package extension
 
 import (
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -50,3 +51,54 @@ func TestUpdateAvailable(t *testing.T) {
 
 	assert.True(t, e.UpdateAvailable())
 }
+
+func TestOwnerLocalExtension(t *testing.T) {
+	tempDir := t.TempDir()
+	extPath := filepath.Join(tempDir, "extensions", "gh-local", "gh-local")
+	assert.NoError(t, stubLocalExtension(tempDir, extPath))
+	e := &Extension{
+		kind: LocalKind,
+		path: extPath,
+	}
+
+	assert.Equal(t, "", e.Owner())
+}
+
+func TestOwnerBinaryExtension(t *testing.T) {
+	tempDir := t.TempDir()
+	extName := "gh-bin-ext"
+	extDir := filepath.Join(tempDir, "extensions", extName)
+	extPath := filepath.Join(extDir, extName)
+	bm := binManifest{
+		Owner: "owner",
+		Name:  "gh-bin-ext",
+		Host:  "example.com",
+		Tag:   "v1.0.1",
+	}
+	assert.NoError(t, stubBinaryExtension(extDir, bm))
+	e := &Extension{
+		kind: BinaryKind,
+		path: extPath,
+	}
+
+	assert.Equal(t, "owner", e.Owner())
+}
+
+func TestOwnerGitExtension(t *testing.T) {
+	gc := &mockGitClient{}
+	gc.On("Config", "remote.origin.url").Return("git@github.com:owner/repo.git", nil).Once()
+	e := &Extension{
+		kind:      GitKind,
+		gitClient: gc,
+	}
+
+	assert.Equal(t, "owner", e.Owner())
+}
+
+func TestOwnerCached(t *testing.T) {
+	e := &Extension{
+		owner: "cli",
+	}
+
+	assert.Equal(t, "cli", e.Owner())
+}