diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index f6fbcabd5..c3cde03cf 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -147,8 +147,7 @@ func Test_NewCmdLogin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, stdin, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdoutTTY(true) diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index dbdae26af..c8dbebbe5 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -91,8 +91,7 @@ func Test_NewCmdRefresh(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, _, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdinTTY(tt.tty) io.SetStdoutTTY(tt.tty) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 08d93c2be..110dfb4e9 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "os" - "path/filepath" "time" "github.com/cli/cli/v2/api" @@ -19,17 +18,10 @@ import ( ) func New(appVersion string) *cmdutil.Factory { - var exe string f := &cmdutil.Factory{ - Config: configFunc(), // No factory dependencies - Branch: branchFunc(), // No factory dependencies - Executable: func() string { - if exe != "" { - return exe - } - exe = executable("gh") - return exe - }, + Config: configFunc(), // No factory dependencies + Branch: branchFunc(), // No factory dependencies + ExecutableName: "gh", } f.IOStreams = ioStreams(f) // Depends on Config @@ -121,52 +113,6 @@ func browserLauncher(f *cmdutil.Factory) string { return os.Getenv("BROWSER") } -// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. -// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in -// PATH, return the absolute location to the program. -// -// The idea is that the result of this function is callable in the future and refers to the same -// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software -// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. -// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of -// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew -// location. -// -// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute -// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git -// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh -// auth login`, running `brew update` will print out authentication errors as git is unable to locate -// Homebrew-installed `gh`. -func executable(fallbackName string) string { - exe, err := os.Executable() - if err != nil { - return fallbackName - } - - base := filepath.Base(exe) - path := os.Getenv("PATH") - for _, dir := range filepath.SplitList(path) { - p, err := filepath.Abs(filepath.Join(dir, base)) - if err != nil { - continue - } - f, err := os.Stat(p) - if err != nil { - continue - } - - if p == exe { - return p - } else if f.Mode()&os.ModeSymlink != 0 { - if t, err := os.Readlink(p); err == nil && t == exe { - return p - } - } - } - - return exe -} - func configFunc() func() (config.Config, error) { var cachedConfig config.Config var configError error diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 83d4a638f..73cee489a 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -2,6 +2,9 @@ package cmdutil import ( "net/http" + "os" + "path/filepath" + "strings" "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/config" @@ -25,7 +28,59 @@ type Factory struct { Branch func() (string, error) ExtensionManager extensions.ExtensionManager - - // Executable is the path to the currently invoked gh binary - Executable func() string + ExecutableName string +} + +// Executable is the path to the currently invoked binary +func (f *Factory) Executable() string { + if !strings.ContainsRune(f.ExecutableName, os.PathSeparator) { + f.ExecutableName = executable(f.ExecutableName) + } + return f.ExecutableName +} + +// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. +// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in +// PATH, return the absolute location to the program. +// +// The idea is that the result of this function is callable in the future and refers to the same +// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software +// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. +// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of +// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew +// location. +// +// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute +// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git +// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh +// auth login`, running `brew update` will print out authentication errors as git is unable to locate +// Homebrew-installed `gh`. +func executable(fallbackName string) string { + exe, err := os.Executable() + if err != nil { + return fallbackName + } + + base := filepath.Base(exe) + path := os.Getenv("PATH") + for _, dir := range filepath.SplitList(path) { + p, err := filepath.Abs(filepath.Join(dir, base)) + if err != nil { + continue + } + f, err := os.Stat(p) + if err != nil { + continue + } + + if p == exe { + return p + } else if f.Mode()&os.ModeSymlink != 0 { + if t, err := os.Readlink(p); err == nil && t == exe { + return p + } + } + } + + return exe }