Repository: cli/cli
Pull request #7493 (2023-05-26 14:53:26)

Speed up listing extensions by lazy-loading extension information when needed
Fetching extension information such as its GitHub URL, current version, and `pinned` property isn't strictly necessary for just listing extensions, so this restructures extensions code to lazy-load this information on `ext.URL()`, `ext.CurrentVersion()`, `ext.LatestVersion()`, and `ext.IsPinned()`. This generally is a shift in responsibility for these items from the extension manager to the extension itself.

This will speed up gh invocation and shell completion, since extensions are automatically listed when constructing the gh command tree. Followup to https://github.com/cli/cli/pull/7457

samcoe: @mislav Thanks for getting started on this work and caring deeply about the performance of `gh`. My understanding of the performance issue is that when listing extensions we shell out to `git` to retrieve information for non-binary extensions, these `git` invocations are slow when compared to reading the manifest files for binary extensions. Is that accurate? 

Assuming it is the `git` invocations that we are really trying to reduce here I have two alternative approaches that I think we could consider here.

1. When mounting extensions on `gh` initialization we only rely on the extension name. Right now we use the name and the URL if it is available. Removing the URL would remove the `git` invocation. We would need to add in a new function to the extension manager to list just the extension names but that would be trivial. This is the simplest idea I could come up with, but slightly sacrifices the UX.

2. We start writing manifest files for non-binary extensions. When installing/updating an extension we will create manifest files just like we do for binary extensions. Then when listing extensions we can read the manifest files instead of invoking `git`. I like this approach as it actually brings the two types of extensions more inline with each other. The code for writing manifest files is already in place so I don't see this is a huge lift. The only thing I would suggest is that when listing extensions we try to write a manifest file if one does not exist so that users won't need to do an install/update of an extension to see get the performance improvement.

Thoughts? 
mislav: > 1. Removing the URL would remove the `git` invocation. We would need to add in a new function to the extension manager to list just the extension names but that would be trivial.

I'd prefer this approach. However, I don't see why we should add yet another `list` functionality when we already have two (list w/ version metadata vs. list without version metadata). I'd rather reduce our current `list` function to only list extensions by name, and then when the caller requests more information per extension, those are looked up on request.

> 2\. We start writing manifest files for non-binary extensions.

I appreciate this idea and I don't think it would be hard to do, but honestly I don't see why we should start writing manifest for git extensions when we can already look up information about them locally. We should just avoid looking up this information altogether unless the information is necessary.

The way I envision the lazy-loading approach, we wouldn't even load the binary extension manifest in plain `list` mode.
samcoe: > I'd prefer this approach. However, I don't see why we should add yet another list functionality when we already have two (list w/ version metadata vs. list without version metadata). I'd rather reduce our current list function to only list extensions by name, and then when the caller requests more information per extension, those are looked up on request.

That's a fair point, the listing functionality with the two functions is already a bit confusing. One option, that may be less work than making this lazy-loaded would be to have the list function take in a list of fields that the caller is requesting being loaded, similar to a GraphQL request. Just an idea.

> I appreciate this idea and I don't think it would be hard to do, but honestly I don't see why we should start writing manifest for git extensions when we can already look up information about them locally. We should just avoid looking up this information altogether unless the information is necessary.

Also a good point. We had previously discussed allowing manifest files for extensions to open up some new features for them so I thought this approach would get us on the right track to move that direction. The lazy-loading fix you are creating here does not impede that potential future direction though, so no concerns from me there. 
mislav: @samcoe Could you see to finishing the lazy-loading aspect? Basically, the parts that I've commented out in this branch should run on request. Or feel free to go with your own approach if that's simpler. Thanks!

Patch:
diff --git a/go.mod b/go.mod
index a0b3781cf..e0e92c352 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/cli/cli/v2
 go 1.19
 
 require (
-	github.com/AlecAivazis/survey/v2 v2.3.6
+	github.com/AlecAivazis/survey/v2 v2.3.7
 	github.com/MakeNowJust/heredoc v1.0.0
 	github.com/briandowns/spinner v1.18.1
 	github.com/cenkalti/backoff/v4 v4.2.1
diff --git a/go.sum b/go.sum
index 1d3b83805..85ba744f4 100644
--- a/go.sum
+++ b/go.sum
@@ -31,8 +31,8 @@ cloud.google.com/go/storage v1.6.0/go.mod h1:N7U0C8pVQ/+NIKOBQyamJIeKQKkZ+mxpohl
 cloud.google.com/go/storage v1.8.0/go.mod h1:Wv1Oy7z6Yz3DshWRJFhqM/UCfaWIRTdp0RXyy7KQOVs=
 cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9ullr3+Kg0=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
-github.com/AlecAivazis/survey/v2 v2.3.6 h1:NvTuVHISgTHEHeBFqt6BHOe4Ny/NwGZr7w+F8S9ziyw=
-github.com/AlecAivazis/survey/v2 v2.3.6/go.mod h1:4AuI9b7RjAR+G7v9+C4YSlX/YL3K3cWNXgWXOhllqvI=
+github.com/AlecAivazis/survey/v2 v2.3.7 h1:6I/u8FvytdGsgonrYsVn2t8t4QiRnh6QSTqkkhIiSjQ=
+github.com/AlecAivazis/survey/v2 v2.3.7/go.mod h1:xUTIdE4KCOIjsBAE1JYsUPoCqYdZ1reCfTwbto0Fduo=
 github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
 github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo=
 github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ4pzQ=
@@ -401,7 +401,6 @@ golang.org/x/sys v0.0.0-20210831042530-f4d43177bf5e/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220422013727-9388b58f7150/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
@@ -410,7 +409,6 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
-golang.org/x/term v0.0.0-20210503060354-a79de5458b56/go.mod h1:tfny5GFUkzUvx4ps4ajbZsCe5lw1metzhBm9T3x7oIY=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/term v0.7.0 h1:BEvjmm5fURWqcfbSKTdpkDXYBrUS1c0m8agp14W48vQ=
 golang.org/x/term v0.7.0/go.mod h1:P32HKFT3hSsZrRxla30E9HqToFYAQPCMs/zFMBUFqPY=
@@ -421,6 +419,7 @@ golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
 golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
+golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/text v0.5.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/text v0.9.0 h1:2sjJmO8cDvYveuX97RDLsxlyUxLl+GHoLxBiRdHllBE=
 golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8=
diff --git a/pkg/cmd/extension/extension.go b/pkg/cmd/extension/extension.go
index 9d1d61c21..188687634 100644
--- a/pkg/cmd/extension/extension.go
+++ b/pkg/cmd/extension/extension.go
@@ -1,8 +1,16 @@
 package extension
 
 import (
+	"bytes"
+	"fmt"
+	"net/http"
+	"os"
 	"path/filepath"
 	"strings"
+	"sync"
+
+	"github.com/cli/cli/v2/internal/ghrepo"
+	"gopkg.in/yaml.v3"
 )
 
 const manifestName = "manifest.yml"
@@ -12,16 +20,22 @@ type ExtensionKind int
 const (
 	GitKind ExtensionKind = iota
 	BinaryKind
+	LocalKind
 )
 
 type Extension struct {
-	path           string
+	path       string
+	kind       ExtensionKind
+	gitClient  gitClient
+	httpClient *http.Client
+
+	mu sync.RWMutex
+
+	// These fields get resolved dynamically:
 	url            string
-	isLocal        bool
-	isPinned       bool
+	isPinned       *bool
 	currentVersion string
 	latestVersion  string
-	kind           ExtensionKind
 }
 
 func (e *Extension) Name() string {
@@ -32,32 +46,157 @@ func (e *Extension) Path() string {
 	return e.path
 }
 
-func (e *Extension) URL() string {
-	return e.url
+func (e *Extension) IsLocal() bool {
+	return e.kind == LocalKind
 }
 
-func (e *Extension) IsLocal() bool {
-	return e.isLocal
+func (e *Extension) IsBinary() bool {
+	return e.kind == BinaryKind
+}
+
+func (e *Extension) URL() string {
+	e.mu.RLock()
+	if e.url != "" {
+		defer e.mu.RUnlock()
+		return e.url
+	}
+	e.mu.RUnlock()
+
+	var url string
+	switch e.kind {
+	case LocalKind:
+	case BinaryKind:
+		if manifest, err := e.loadManifest(); err == nil {
+			repo := ghrepo.NewWithHost(manifest.Owner, manifest.Name, manifest.Host)
+			url = ghrepo.GenerateRepoURL(repo, "")
+		}
+	case GitKind:
+		if remoteURL, err := e.gitClient.Config("remote.origin.url"); err == nil {
+			url = strings.TrimSpace(string(remoteURL))
+		}
+	}
+
+	e.mu.Lock()
+	e.url = url
+	e.mu.Unlock()
+
+	return e.url
 }
 
 func (e *Extension) CurrentVersion() string {
+	e.mu.RLock()
+	if e.currentVersion != "" {
+		defer e.mu.RUnlock()
+		return e.currentVersion
+	}
+	e.mu.RUnlock()
+
+	var currentVersion string
+	switch e.kind {
+	case LocalKind:
+	case BinaryKind:
+		if manifest, err := e.loadManifest(); err == nil {
+			currentVersion = manifest.Tag
+		}
+	case GitKind:
+		if sha, err := e.gitClient.CommandOutput([]string{"rev-parse", "HEAD"}); err == nil {
+			currentVersion = string(bytes.TrimSpace(sha))
+		}
+	}
+
+	e.mu.Lock()
+	e.currentVersion = currentVersion
+	e.mu.Unlock()
+
 	return e.currentVersion
 }
 
+func (e *Extension) LatestVersion() string {
+	e.mu.RLock()
+	if e.latestVersion != "" {
+		defer e.mu.RUnlock()
+		return e.latestVersion
+	}
+	e.mu.RUnlock()
+
+	var latestVersion string
+	switch e.kind {
+	case LocalKind:
+	case BinaryKind:
+		repo, err := ghrepo.FromFullName(e.URL())
+		if err != nil {
+			return ""
+		}
+		release, err := fetchLatestRelease(e.httpClient, repo)
+		if err != nil {
+			return ""
+		}
+		latestVersion = release.Tag
+	case GitKind:
+		if lsRemote, err := e.gitClient.CommandOutput([]string{"ls-remote", "origin", "HEAD"}); err == nil {
+			latestVersion = string(bytes.SplitN(lsRemote, []byte("\t"), 2)[0])
+		}
+	}
+
+	e.mu.Lock()
+	e.latestVersion = latestVersion
+	e.mu.Unlock()
+
+	return e.latestVersion
+}
+
 func (e *Extension) IsPinned() bool {
-	return e.isPinned
+	e.mu.RLock()
+	if e.isPinned != nil {
+		defer e.mu.RUnlock()
+		return *e.isPinned
+	}
+	e.mu.RUnlock()
+
+	var isPinned bool
+	switch e.kind {
+	case LocalKind:
+	case BinaryKind:
+		if manifest, err := e.loadManifest(); err == nil {
+			isPinned = manifest.IsPinned
+		}
+	case GitKind:
+		pinPath := filepath.Join(e.Path(), fmt.Sprintf(".pin-%s", e.CurrentVersion()))
+		if _, err := os.Stat(pinPath); err == nil {
+			isPinned = true
+		} else {
+			isPinned = false
+		}
+	}
+
+	e.mu.Lock()
+	e.isPinned = &isPinned
+	e.mu.Unlock()
+
+	return *e.isPinned
 }
 
 func (e *Extension) UpdateAvailable() bool {
-	if e.isLocal ||
-		e.currentVersion == "" ||
-		e.latestVersion == "" ||
-		e.currentVersion == e.latestVersion {
+	if e.IsLocal() ||
+		e.CurrentVersion() == "" ||
+		e.LatestVersion() == "" ||
+		e.CurrentVersion() == e.LatestVersion() {
 		return false
 	}
 	return true
 }
 
-func (e *Extension) IsBinary() bool {
-	return e.kind == BinaryKind
+func (e *Extension) loadManifest() (binManifest, error) {
+	var bm binManifest
+	dir, _ := filepath.Split(e.Path())
+	manifestPath := filepath.Join(dir, manifestName)
+	manifest, err := os.ReadFile(manifestPath)
+	if err != nil {
+		return bm, fmt.Errorf("could not open %s for reading: %w", manifestPath, err)
+	}
+	err = yaml.Unmarshal(manifest, &bm)
+	if err != nil {
+		return bm, fmt.Errorf("could not parse %s: %w", manifestPath, err)
+	}
+	return bm, nil
 }
diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go
index 7a76e6a47..b19175880 100644
--- a/pkg/cmd/extension/manager.go
+++ b/pkg/cmd/extension/manager.go
@@ -1,12 +1,10 @@
 package extension
 
 import (
-	"bytes"
 	_ "embed"
 	"errors"
 	"fmt"
 	"io"
-	"io/fs"
 	"net/http"
 	"os"
 	"os/exec"
@@ -83,7 +81,7 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri
 	forwardArgs := args[1:]
 
 	exts, _ := m.list(false)
-	var ext Extension
+	var ext *Extension
 	for _, e := range exts {
 		if e.Name() == extName {
 			ext = e
@@ -121,39 +119,53 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri
 func (m *Manager) List() []extensions.Extension {
 	exts, _ := m.list(false)
 	r := make([]extensions.Extension, len(exts))
-	for i, v := range exts {
-		val := v
-		r[i] = &val
+	for i, ext := range exts {
+		r[i] = ext
 	}
 	return r
 }
 
-func (m *Manager) list(includeMetadata bool) ([]Extension, error) {
+func (m *Manager) list(includeMetadata bool) ([]*Extension, error) {
 	dir := m.installDir()
 	entries, err := os.ReadDir(dir)
 	if err != nil {
 		return nil, err
 	}
 
-	var results []Extension
+	results := make([]*Extension, 0, len(entries))
 	for _, f := range entries {
 		if !strings.HasPrefix(f.Name(), "gh-") {
 			continue
 		}
-		var ext Extension
-		var err error
 		if f.IsDir() {
-			ext, err = m.parseExtensionDir(f)
-			if err != nil {
-				return nil, err
+			if _, err := os.Stat(filepath.Join(dir, f.Name(), manifestName)); err == nil {
+				results = append(results, &Extension{
+					path:       filepath.Join(dir, f.Name(), f.Name()),
+					kind:       BinaryKind,
+					httpClient: m.client,
+				})
+			} else {
+				results = append(results, &Extension{
+					path:      filepath.Join(dir, f.Name(), f.Name()),
+					kind:      GitKind,
+					gitClient: m.gitClient.ForRepo(filepath.Join(dir, f.Name())),
+				})
 			}
-			results = append(results, ext)
+		} else if isSymlink(f.Type()) {
+			results = append(results, &Extension{
+				path: filepath.Join(dir, f.Name(), f.Name()),
+				kind: LocalKind,
+			})
 		} else {
-			ext, err = m.parseExtensionFile(f)
+			// the contents of a regular file point to a local extension on disk
+			p, err := readPathFromFile(filepath.Join(dir, f.Name()))
 			if err != nil {
 				return nil, err
 			}
-			results = append(results, ext)
+			results = append(results, &Extension{
+				path: filepath.Join(p, f.Name()),
+				kind: LocalKind,
+			})
 		}
 	}
 
@@ -164,145 +176,16 @@ func (m *Manager) list(includeMetadata bool) ([]Extension, error) {
 	return results, nil
 }
 
-func (m *Manager) parseExtensionFile(fi fs.DirEntry) (Extension, error) {
-	ext := Extension{isLocal: true}
-	id := m.installDir()
-	exePath := filepath.Join(id, fi.Name(), fi.Name())
-	if !isSymlink(fi.Type()) {
-		// if this is a regular file, its contents is the local directory of the extension
-		p, err := readPathFromFile(filepath.Join(id, fi.Name()))
-		if err != nil {
-			return ext, err
-		}
-		exePath = filepath.Join(p, fi.Name())
-	}
-	ext.path = exePath
-	return ext, nil
-}
-
-func (m *Manager) parseExtensionDir(fi fs.DirEntry) (Extension, error) {
-	id := m.installDir()
-	if _, err := os.Stat(filepath.Join(id, fi.Name(), manifestName)); err == nil {
-		return m.parseBinaryExtensionDir(fi)
-	}
-
-	return m.parseGitExtensionDir(fi)
-}
-
-func (m *Manager) parseBinaryExtensionDir(fi fs.DirEntry) (Extension, error) {
-	id := m.installDir()
-	exePath := filepath.Join(id, fi.Name(), fi.Name())
-	ext := Extension{path: exePath, kind: BinaryKind}
-	manifestPath := filepath.Join(id, fi.Name(), manifestName)
-	manifest, err := os.ReadFile(manifestPath)
-	if err != nil {
-		return ext, fmt.Errorf("could not open %s for reading: %w", manifestPath, err)
-	}
-	var bm binManifest
-	err = yaml.Unmarshal(manifest, &bm)
-	if err != nil {
-		return ext, fmt.Errorf("could not parse %s: %w", manifestPath, err)
-	}
-	repo := ghrepo.NewWithHost(bm.Owner, bm.Name, bm.Host)
-	remoteURL := ghrepo.GenerateRepoURL(repo, "")
-	ext.url = remoteURL
-	ext.currentVersion = bm.Tag
-	ext.isPinned = bm.IsPinned
-	return ext, nil
-}
-
-func (m *Manager) parseGitExtensionDir(fi fs.DirEntry) (Extension, error) {
-	id := m.installDir()
-	exePath := filepath.Join(id, fi.Name(), fi.Name())
-	remoteUrl := m.getRemoteUrl(fi.Name())
-	currentVersion := m.getCurrentVersion(fi.Name())
-
-	var isPinned bool
-	pinPath := filepath.Join(id, fi.Name(), fmt.Sprintf(".pin-%s", currentVersion))
-	if _, err := os.Stat(pinPath); err == nil {
-		isPinned = true
-	}
-
-	return Extension{
-		path:           exePath,
-		url:            remoteUrl,
-		isLocal:        false,
-		currentVersion: currentVersion,
-		kind:           GitKind,
-		isPinned:       isPinned,
-	}, nil
-}
-
-// getCurrentVersion determines the current version for non-local git extensions.
-func (m *Manager) getCurrentVersion(extension string) string {
-	dir := filepath.Join(m.installDir(), extension)
-	scopedClient := m.gitClient.ForRepo(dir)
-	localSha, err := scopedClient.CommandOutput([]string{"rev-parse", "HEAD"})
-	if err != nil {
-		return ""
-	}
-	return string(bytes.TrimSpace(localSha))
-}
-
-// getRemoteUrl determines the remote URL for non-local git extensions.
-func (m *Manager) getRemoteUrl(extension string) string {
-	dir := filepath.Join(m.installDir(), extension)
-	scopedClient := m.gitClient.ForRepo(dir)
-	url, err := scopedClient.Config("remote.origin.url")
-	if err != nil {
-		return ""
-	}
-	return strings.TrimSpace(string(url))
-}
-
-func (m *Manager) populateLatestVersions(exts []Extension) {
-	size := len(exts)
-	type result struct {
-		index   int
-		version string
-	}
-	ch := make(chan result, size)
+func (m *Manager) populateLatestVersions(exts []*Extension) {
 	var wg sync.WaitGroup
-	wg.Add(size)
-	for idx, ext := range exts {
-		go func(i int, e Extension) {
+	for _, ext := range exts {
+		wg.Add(1)
+		go func(e *Extension) {
 			defer wg.Done()
-			version, _ := m.getLatestVersion(e)
-			ch <- result{index: i, version: version}
-		}(idx, ext)
+			e.LatestVersion()
+		}(ext)
 	}
 	wg.Wait()
-	close(ch)
-	for r := range ch {
-		ext := &exts[r.index]
-		ext.latestVersion = r.version
-	}
-}
-
-func (m *Manager) getLatestVersion(ext Extension) (string, error) {
-	if ext.isLocal {
-		return "", localExtensionUpgradeError
-	}
-	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 {
-		extDir := filepath.Dir(ext.path)
-		scopedClient := m.gitClient.ForRepo(extDir)
-		lsRemote, err := scopedClient.CommandOutput([]string{"ls-remote", "origin", "HEAD"})
-		if err != nil {
-			return "", err
-		}
-		remoteSha := bytes.SplitN(lsRemote, []byte("\t"), 2)[0]
-		return string(remoteSha), nil
-	}
 }
 
 func (m *Manager) InstallLocal(dir string) error {
@@ -521,22 +404,23 @@ func (m *Manager) Upgrade(name string, force bool) error {
 		if f.Name() != name {
 			continue
 		}
-		var err error
-		// For single extensions manually retrieve latest version since we forgo
-		// doing it during list.
-		f.latestVersion, err = m.getLatestVersion(f)
-		if err != nil {
-			return err
+		if f.IsLocal() {
+			return localExtensionUpgradeError
+		}
+		// For single extensions manually retrieve latest version since we forgo doing it during list.
+		if latestVersion := f.LatestVersion(); latestVersion == "" {
+			return fmt.Errorf("unable to retrieve latest version for extension %q", name)
 		}
-		return m.upgradeExtensions([]Extension{f}, force)
+		return m.upgradeExtensions([]*Extension{f}, force)
 	}
 	return fmt.Errorf("no extension matched %q", name)
 }
 
-func (m *Manager) upgradeExtensions(exts []Extension, force bool) error {
+func (m *Manager) upgradeExtensions(exts []*Extension, force bool) error {
 	var failed bool
 	for _, f := range exts {
 		fmt.Fprintf(m.io.Out, "[%s]: ", f.Name())
+		currentVersion := displayExtensionVersion(f, f.CurrentVersion())
 		err := m.upgradeExtension(f, force)
 		if err != nil {
 			if !errors.Is(err, localExtensionUpgradeError) &&
@@ -547,8 +431,7 @@ func (m *Manager) upgradeExtensions(exts []Extension, force bool) error {
 			fmt.Fprintf(m.io.Out, "%s\n", err)
 			continue
 		}
-		currentVersion := displayExtensionVersion(&f, f.currentVersion)
-		latestVersion := displayExtensionVersion(&f, f.latestVersion)
+		latestVersion := displayExtensionVersion(f, f.LatestVersion())
 		if m.dryRunMode {
 			fmt.Fprintf(m.io.Out, "would have upgraded from %s to %s\n", currentVersion, latestVersion)
 		} else {
@@ -561,8 +444,8 @@ func (m *Manager) upgradeExtensions(exts []Extension, force bool) error {
 	return nil
 }
 
-func (m *Manager) upgradeExtension(ext Extension, force bool) error {
-	if ext.isLocal {
+func (m *Manager) upgradeExtension(ext *Extension, force bool) error {
+	if ext.IsLocal() {
 		return localExtensionUpgradeError
 	}
 	if !force && ext.IsPinned() {
@@ -592,7 +475,7 @@ func (m *Manager) upgradeExtension(ext Extension, force bool) error {
 	return err
 }
 
-func (m *Manager) upgradeGitExtension(ext Extension, force bool) error {
+func (m *Manager) upgradeGitExtension(ext *Extension, force bool) error {
 	if m.dryRunMode {
 		return nil
 	}
@@ -611,10 +494,10 @@ func (m *Manager) upgradeGitExtension(ext Extension, force bool) error {
 	return scopedClient.Pull("", "")
 }
 
-func (m *Manager) upgradeBinExtension(ext Extension) error {
-	repo, err := ghrepo.FromFullName(ext.url)
+func (m *Manager) upgradeBinExtension(ext *Extension) error {
+	repo, err := ghrepo.FromFullName(ext.URL())
 	if err != nil {
-		return fmt.Errorf("failed to parse URL %s: %w", ext.url, err)
+		return fmt.Errorf("failed to parse URL %s: %w", ext.URL(), err)
 	}
 	return m.installBin(repo, "")
 }
diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go
index 52ef6b0a4..dea637619 100644
--- a/pkg/cmd/root/extension.go
+++ b/pkg/cmd/root/extension.go
@@ -3,29 +3,15 @@ package root
 import (
 	"fmt"
 
-	"github.com/cli/cli/v2/git"
-	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/extensions"
 	"github.com/cli/cli/v2/pkg/iostreams"
 	"github.com/spf13/cobra"
 )
 
 func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ext extensions.Extension) *cobra.Command {
-	var short string
-	if ext.IsLocal() {
-		short = fmt.Sprintf("Local extension gh-%s", ext.Name())
-	} else {
-		path := ext.URL()
-		if u, err := git.ParseURL(ext.URL()); err == nil {
-			if r, err := ghrepo.FromURL(u); err == nil {
-				path = ghrepo.FullName(r)
-			}
-		}
-		short = fmt.Sprintf("Extension %s", path)
-	}
 	return &cobra.Command{
 		Use:   ext.Name(),
-		Short: short,
+		Short: fmt.Sprintf("Extension %s", ext.Name()),
 		RunE: func(c *cobra.Command, args []string) error {
 			args = append([]string{ext.Name()}, args...)
 			if _, err := em.Dispatch(args, io.In, io.Out, io.ErrOut); err != nil {
diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go
index 95af0ccea..2583472c0 100644
--- a/pkg/extensions/extension.go
+++ b/pkg/extensions/extension.go
@@ -20,6 +20,7 @@ type Extension interface {
 	Path() string // Path to executable
 	URL() string
 	CurrentVersion() string
+	LatestVersion() string
 	IsPinned() bool
 	UpdateAvailable() bool
 	IsBinary() bool
diff --git a/pkg/extensions/extension_mock.go b/pkg/extensions/extension_mock.go
index a97de8f62..500603661 100644
--- a/pkg/extensions/extension_mock.go
+++ b/pkg/extensions/extension_mock.go
@@ -29,6 +29,9 @@ var _ Extension = &ExtensionMock{}
 //			IsPinnedFunc: func() bool {
 //				panic("mock out the IsPinned method")
 //			},
+//			LatestVersionFunc: func() string {
+//				panic("mock out the LatestVersion method")
+//			},
 //			NameFunc: func() string {
 //				panic("mock out the Name method")
 //			},
@@ -60,6 +63,9 @@ type ExtensionMock struct {
 	// IsPinnedFunc mocks the IsPinned method.
 	IsPinnedFunc func() bool
 
+	// LatestVersionFunc mocks the LatestVersion method.
+	LatestVersionFunc func() string
+
 	// NameFunc mocks the Name method.
 	NameFunc func() string
 
@@ -86,6 +92,9 @@ type ExtensionMock struct {
 		// IsPinned holds details about calls to the IsPinned method.
 		IsPinned []struct {
 		}
+		// LatestVersion holds details about calls to the LatestVersion method.
+		LatestVersion []struct {
+		}
 		// Name holds details about calls to the Name method.
 		Name []struct {
 		}
@@ -103,6 +112,7 @@ type ExtensionMock struct {
 	lockIsBinary        sync.RWMutex
 	lockIsLocal         sync.RWMutex
 	lockIsPinned        sync.RWMutex
+	lockLatestVersion   sync.RWMutex
 	lockName            sync.RWMutex
 	lockPath            sync.RWMutex
 	lockURL             sync.RWMutex
@@ -217,6 +227,33 @@ func (mock *ExtensionMock) IsPinnedCalls() []struct {
 	return calls
 }
 
+// LatestVersion calls LatestVersionFunc.
+func (mock *ExtensionMock) LatestVersion() string {
+	if mock.LatestVersionFunc == nil {
+		panic("ExtensionMock.LatestVersionFunc: method is nil but Extension.LatestVersion was just called")
+	}
+	callInfo := struct {
+	}{}
+	mock.lockLatestVersion.Lock()
+	mock.calls.LatestVersion = append(mock.calls.LatestVersion, callInfo)
+	mock.lockLatestVersion.Unlock()
+	return mock.LatestVersionFunc()
+}
+
+// LatestVersionCalls gets all the calls that were made to LatestVersion.
+// Check the length with:
+//
+//	len(mockedExtension.LatestVersionCalls())
+func (mock *ExtensionMock) LatestVersionCalls() []struct {
+} {
+	var calls []struct {
+	}
+	mock.lockLatestVersion.RLock()
+	calls = mock.calls.LatestVersion
+	mock.lockLatestVersion.RUnlock()
+	return calls
+}
+
 // Name calls NameFunc.
 func (mock *ExtensionMock) Name() string {
 	if mock.NameFunc == nil {


Test patch:
diff --git a/pkg/cmd/extension/extension_test.go b/pkg/cmd/extension/extension_test.go
index 0a50cbfe8..9b56e0bee 100644
--- a/pkg/cmd/extension/extension_test.go
+++ b/pkg/cmd/extension/extension_test.go
@@ -8,7 +8,7 @@ import (
 
 func TestUpdateAvailable_IsLocal(t *testing.T) {
 	e := &Extension{
-		isLocal: true,
+		kind: LocalKind,
 	}
 
 	assert.False(t, e.UpdateAvailable())
@@ -16,7 +16,7 @@ func TestUpdateAvailable_IsLocal(t *testing.T) {
 
 func TestUpdateAvailable_NoCurrentVersion(t *testing.T) {
 	e := &Extension{
-		isLocal: false,
+		kind: LocalKind,
 	}
 
 	assert.False(t, e.UpdateAvailable())
@@ -24,7 +24,7 @@ func TestUpdateAvailable_NoCurrentVersion(t *testing.T) {
 
 func TestUpdateAvailable_NoLatestVersion(t *testing.T) {
 	e := &Extension{
-		isLocal:        false,
+		kind:           BinaryKind,
 		currentVersion: "1.0.0",
 	}
 
@@ -33,7 +33,7 @@ func TestUpdateAvailable_NoLatestVersion(t *testing.T) {
 
 func TestUpdateAvailable_CurrentVersionIsLatestVersion(t *testing.T) {
 	e := &Extension{
-		isLocal:        false,
+		kind:           BinaryKind,
 		currentVersion: "1.0.0",
 		latestVersion:  "1.0.0",
 	}
@@ -43,7 +43,7 @@ func TestUpdateAvailable_CurrentVersionIsLatestVersion(t *testing.T) {
 
 func TestUpdateAvailable(t *testing.T) {
 	e := &Extension{
-		isLocal:        false,
+		kind:           BinaryKind,
 		currentVersion: "1.0.0",
 		latestVersion:  "1.1.0",
 	}
diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go
index b3fe64567..6d9b63124 100644
--- a/pkg/cmd/extension/manager_test.go
+++ b/pkg/cmd/extension/manager_test.go
@@ -80,12 +80,8 @@ func TestManager_List(t *testing.T) {
 	dirOne := filepath.Join(tempDir, "extensions", "gh-hello")
 	dirTwo := filepath.Join(tempDir, "extensions", "gh-two")
 	gc, gcOne, gcTwo := &mockGitClient{}, &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", dirOne).Return(gcOne).Twice()
-	gc.On("ForRepo", dirTwo).Return(gcTwo).Twice()
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcTwo.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
-	gcTwo.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", dirOne).Return(gcOne).Once()
+	gc.On("ForRepo", dirTwo).Return(gcTwo).Once()
 
 	m := newTestManager(tempDir, nil, gc, nil)
 	exts := m.List()
@@ -145,9 +141,7 @@ func TestManager_Dispatch(t *testing.T) {
 	assert.NoError(t, stubExtension(extPath))
 
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extDir).Return(gcOne).Twice()
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extDir).Return(gcOne).Once()
 
 	m := newTestManager(tempDir, nil, gc, nil)
 
@@ -223,9 +217,7 @@ func TestManager_Upgrade_NoMatchingExtension(t *testing.T) {
 	assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extDir).Return(gcOne).Twice()
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extDir).Return(gcOne).Once()
 	m := newTestManager(tempDir, nil, gc, ios)
 	err := m.Upgrade("invalid", false)
 	assert.EqualError(t, err, `no extension matched "invalid"`)
@@ -244,12 +236,8 @@ func TestManager_UpgradeExtensions(t *testing.T) {
 	assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne, gcTwo := &mockGitClient{}, &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", dirOne).Return(gcOne).Times(4)
-	gc.On("ForRepo", dirTwo).Return(gcTwo).Times(4)
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcTwo.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
-	gcTwo.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", dirOne).Return(gcOne).Times(3)
+	gc.On("ForRepo", dirTwo).Return(gcTwo).Times(3)
 	gcOne.On("Remotes").Return(nil, nil).Once()
 	gcTwo.On("Remotes").Return(nil, nil).Once()
 	gcOne.On("Pull", "", "").Return(nil).Once()
@@ -286,12 +274,8 @@ func TestManager_UpgradeExtensions_DryRun(t *testing.T) {
 	assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne, gcTwo := &mockGitClient{}, &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", dirOne).Return(gcOne).Times(3)
-	gc.On("ForRepo", dirTwo).Return(gcTwo).Times(3)
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcTwo.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
-	gcTwo.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", dirOne).Return(gcOne).Twice()
+	gc.On("ForRepo", dirTwo).Return(gcTwo).Twice()
 	gcOne.On("Remotes").Return(nil, nil).Once()
 	gcTwo.On("Remotes").Return(nil, nil).Once()
 	m := newTestManager(tempDir, nil, gc, ios)
@@ -355,9 +339,7 @@ func TestManager_UpgradeExtension_GitExtension(t *testing.T) {
 	assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extensionDir).Return(gcOne).Times(4)
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extensionDir).Return(gcOne).Times(3)
 	gcOne.On("Remotes").Return(nil, nil).Once()
 	gcOne.On("Pull", "", "").Return(nil).Once()
 	m := newTestManager(tempDir, nil, gc, ios)
@@ -381,9 +363,7 @@ func TestManager_UpgradeExtension_GitExtension_DryRun(t *testing.T) {
 	assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extDir).Return(gcOne).Times(3)
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extDir).Return(gcOne).Twice()
 	gcOne.On("Remotes").Return(nil, nil).Once()
 	m := newTestManager(tempDir, nil, gc, ios)
 	m.EnableDryRunMode()
@@ -407,9 +387,7 @@ func TestManager_UpgradeExtension_GitExtension_Force(t *testing.T) {
 	assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote")))
 	ios, _, stdout, stderr := iostreams.Test()
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extensionDir).Return(gcOne).Times(4)
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extensionDir).Return(gcOne).Times(3)
 	gcOne.On("Remotes").Return(nil, nil).Once()
 	gcOne.On("Fetch", "origin", "HEAD").Return(nil).Once()
 	gcOne.On("CommandOutput", []string{"reset", "--hard", "origin/HEAD"}).Return("", nil).Once()
@@ -721,9 +699,7 @@ func TestManager_UpgradeExtension_GitExtension_Pinned(t *testing.T) {
 	ios, _, _, _ := iostreams.Test()
 
 	gc, gcOne := &mockGitClient{}, &mockGitClient{}
-	gc.On("ForRepo", extDir).Return(gcOne).Twice()
-	gcOne.On("Config", "remote.origin.url").Return("", nil).Once()
-	gcOne.On("CommandOutput", []string{"rev-parse", "HEAD"}).Return("", nil).Once()
+	gc.On("ForRepo", extDir).Return(gcOne).Once()
 
 	m := newTestManager(tempDir, nil, gc, ios)
 
@@ -732,7 +708,8 @@ func TestManager_UpgradeExtension_GitExtension_Pinned(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(exts))
 	ext := exts[0]
-	ext.isPinned = true
+	pinnedTrue := true
+	ext.isPinned = &pinnedTrue
 	ext.latestVersion = "new version"
 
 	err = m.upgradeExtension(ext, false)