Merge pull request #4568 from cli/bin-ext-dispatch

dispatch binary extensions directly

pkg/cmd/extension/extension.go

@@ -48,3 +48,7 @@ func (e *Extension) UpdateAvailable() bool {
 	}
 	return true
 }
+
+func (e *Extension) IsBinary() bool {
+	return e.kind == BinaryKind
+}

pkg/cmd/extension/manager.go

@@ -69,9 +69,11 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri
 	forwardArgs := args[1:]
 
 	exts, _ := m.list(false)
+	var ext Extension
 	for _, e := range exts {
 		if e.Name() == extName {
-			exe = e.Path()
+			ext = e
+			exe = ext.Path()
 			break
 		}
 	}
@@ -81,7 +83,9 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri
 
 	var externalCmd *exec.Cmd
 
-	if runtime.GOOS == "windows" {
+	if ext.IsBinary() || runtime.GOOS != "windows" {
+		externalCmd = m.newCommand(exe, forwardArgs...)
+	} else if runtime.GOOS == "windows" {
 		// Dispatch all extension calls through the `sh` interpreter to support executable files with a
 		// shebang line on Windows.
 		shExe, err := m.findSh()
@@ -93,8 +97,6 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri
 		}
 		forwardArgs = append([]string{"-c", `command "$@"`, "--", exe}, forwardArgs...)
 		externalCmd = m.newCommand(shExe, forwardArgs...)
-	} else {
-		externalCmd = m.newCommand(exe, forwardArgs...)
 	}
 	externalCmd.Stdin = stdin
 	externalCmd.Stdout = stdout
@@ -268,7 +270,17 @@ func (m *Manager) getLatestVersion(ext Extension) (string, error) {
 	if ext.isLocal {
 		return "", fmt.Errorf("unable to get latest version for local extensions")
 	}
-	if ext.kind == GitKind {
+	if ext.IsBinary() {
+		repo, err := ghrepo.FromFullName(ext.url)
+		if err != nil {
+			return "", err
+		}
+		r, err := fetchLatestRelease(m.client, repo)
+		if err != nil {
+			return "", err
+		}
+		return r.Tag, nil
+	} else {
 		gitExe, err := m.lookPath("git")
 		if err != nil {
 			return "", err
@@ -282,16 +294,6 @@ func (m *Manager) getLatestVersion(ext Extension) (string, error) {
 		}
 		remoteSha := bytes.SplitN(lsRemote, []byte("\t"), 2)[0]
 		return string(remoteSha), nil
-	} else {
-		repo, err := ghrepo.FromFullName(ext.url)
-		if err != nil {
-			return "", err
-		}
-		r, err := fetchLatestRelease(m.client, repo)
-		if err != nil {
-			return "", err
-		}
-		return r.Tag, nil
 	}
 }
 
@@ -477,7 +479,7 @@ func (m *Manager) upgradeExtension(ext Extension, force bool) error {
 		return upToDateError
 	}
 	var err error
-	if ext.kind == BinaryKind {
+	if ext.IsBinary() {
 		err = m.upgradeBinExtension(ext)
 	} else {
 		err = m.upgradeGitExtension(ext, force)

pkg/cmd/extension/manager_test.go

@@ -139,6 +139,30 @@ func TestManager_Dispatch(t *testing.T) {
 	assert.Equal(t, "", stderr.String())
 }
 
+func TestManager_Dispatch_binary(t *testing.T) {
+	tempDir := t.TempDir()
+	extPath := filepath.Join(tempDir, "extensions", "gh-hello")
+	exePath := filepath.Join(extPath, "gh-hello")
+	bm := binManifest{
+		Owner: "owner",
+		Name:  "gh-hello",
+		Host:  "github.com",
+		Tag:   "v1.0.0",
+	}
+	assert.NoError(t, stubBinaryExtension(extPath, bm))
+
+	m := newTestManager(tempDir, nil, nil)
+
+	stdout := &bytes.Buffer{}
+	stderr := &bytes.Buffer{}
+	found, err := m.Dispatch([]string{"hello", "one", "two"}, nil, stdout, stderr)
+	assert.NoError(t, err)
+	assert.True(t, found)
+
+	assert.Equal(t, fmt.Sprintf("[%s one two]\n", exePath), stdout.String())
+	assert.Equal(t, "", stderr.String())
+}
+
 func TestManager_Remove(t *testing.T) {
 	tempDir := t.TempDir()
 	assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello")))

pkg/extensions/extension.go

@@ -13,6 +13,7 @@ type Extension interface {
 	URL() string
 	IsLocal() bool
 	UpdateAvailable() bool
+	IsBinary() bool
 }
 
 //go:generate moq -rm -out manager_mock.go . ExtensionManager

pkg/extensions/extension_mock.go

@@ -17,6 +17,9 @@ var _ Extension = &ExtensionMock{}
 //
 // 		// make and configure a mocked Extension
 // 		mockedExtension := &ExtensionMock{
+// 			IsBinaryFunc: func() bool {
+// 				panic("mock out the IsBinary method")
+// 			},
 // 			IsLocalFunc: func() bool {
 // 				panic("mock out the IsLocal method")
 // 			},
@@ -39,6 +42,9 @@ var _ Extension = &ExtensionMock{}
 //
 // 	}
 type ExtensionMock struct {
+	// IsBinaryFunc mocks the IsBinary method.
+	IsBinaryFunc func() bool
+
 	// IsLocalFunc mocks the IsLocal method.
 	IsLocalFunc func() bool
 
@@ -56,6 +62,9 @@ type ExtensionMock struct {
 
 	// calls tracks calls to the methods.
 	calls struct {
+		// IsBinary holds details about calls to the IsBinary method.
+		IsBinary []struct {
+		}
 		// IsLocal holds details about calls to the IsLocal method.
 		IsLocal []struct {
 		}
@@ -72,6 +81,7 @@ type ExtensionMock struct {
 		UpdateAvailable []struct {
 		}
 	}
+	lockIsBinary        sync.RWMutex
 	lockIsLocal         sync.RWMutex
 	lockName            sync.RWMutex
 	lockPath            sync.RWMutex
@@ -79,6 +89,32 @@ type ExtensionMock struct {
 	lockUpdateAvailable sync.RWMutex
 }
 
+// IsBinary calls IsBinaryFunc.
+func (mock *ExtensionMock) IsBinary() bool {
+	if mock.IsBinaryFunc == nil {
+		panic("ExtensionMock.IsBinaryFunc: method is nil but Extension.IsBinary was just called")
+	}
+	callInfo := struct {
+	}{}
+	mock.lockIsBinary.Lock()
+	mock.calls.IsBinary = append(mock.calls.IsBinary, callInfo)
+	mock.lockIsBinary.Unlock()
+	return mock.IsBinaryFunc()
+}
+
+// IsBinaryCalls gets all the calls that were made to IsBinary.
+// Check the length with:
+//     len(mockedExtension.IsBinaryCalls())
+func (mock *ExtensionMock) IsBinaryCalls() []struct {
+} {
+	var calls []struct {
+	}
+	mock.lockIsBinary.RLock()
+	calls = mock.calls.IsBinary
+	mock.lockIsBinary.RUnlock()
+	return calls
+}
+
 // IsLocal calls IsLocalFunc.
 func (mock *ExtensionMock) IsLocal() bool {
 	if mock.IsLocalFunc == nil {