From 46922694dcd0754a9852c26468026e69659d1d66 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 13:31:34 +0100 Subject: [PATCH 01/17] Support secure credential pattern --- git/client.go | 82 ++++++- git/client_test.go | 308 +++++++++++++++++++------- git/command.go | 6 + internal/run/stub.go | 2 +- pkg/cmd/issue/develop/develop_test.go | 8 +- pkg/cmd/pr/checkout/checkout.go | 13 +- pkg/cmd/pr/checkout/checkout_test.go | 14 ++ pkg/cmd/pr/create/create_test.go | 6 + pkg/cmd/pr/merge/merge_test.go | 6 + pkg/cmd/repo/clone/clone_test.go | 2 + pkg/cmd/repo/create/create.go | 7 +- pkg/cmd/repo/create/create_test.go | 3 + pkg/cmd/repo/fork/fork_test.go | 8 + pkg/cmd/repo/sync/git.go | 7 +- 14 files changed, 379 insertions(+), 93 deletions(-) diff --git a/git/client.go b/git/client.go index 560eccfdd..ce204c50e 100644 --- a/git/client.go +++ b/git/client.go @@ -16,6 +16,7 @@ import ( "strings" "sync" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/safeexec" ) @@ -94,16 +95,27 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } +// WM-TODO: not sure about this type, but I want to ensure that all call sites are provding the host, +// which is hard if the signature of AuthenticatedCommand is (context.Context, host string, args ...string) +// because this means AuthenticatedCommand(ctx, "fetch") will not be a compile error. +type CredentialPattern struct { + pattern string +} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. -func (c *Client) AuthenticatedCommand(ctx context.Context, args ...string) (*Command, error) { - preArgs := []string{"-c", "credential.helper="} +func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { + if credentialPattern.pattern == "" { + panic("get your shit together") + } + + preArgs := []string{"-c", fmt.Sprintf("credential.%s.helper=", credentialPattern.pattern)} if c.GhPath == "" { // Assumes that gh is in PATH. c.GhPath = "gh" } credHelper := fmt.Sprintf("!%q auth git-credential", c.GhPath) - preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.%s.helper=%s", credentialPattern.pattern, credHelper)) args = append(preArgs, args...) return c.Command(ctx, args...) } @@ -152,6 +164,19 @@ func (c *Client) UpdateRemoteURL(ctx context.Context, name, url string) error { return nil } +func (c *Client) GetRemoteURL(ctx context.Context, name string) (string, error) { + args := []string{"remote", "get-url", name} + cmd, err := c.Command(ctx, args...) + if err != nil { + return "", err + } + out, err := cmd.Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + func (c *Client) SetRemoteResolution(ctx context.Context, name, resolution string) error { args := []string{"config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution} cmd, err := c.Command(ctx, args...) @@ -545,11 +570,16 @@ func (c *Client) AddRemote(ctx context.Context, name, urlStr string, trackingBra // Below are commands that make network calls and need authentication credentials supplied from gh. func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods ...CommandModifier) error { + host, err := c.CredentialPatternFromRemote(ctx, remote) + if err != nil { + return err + } + args := []string{"fetch", remote} if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, host, args...) if err != nil { return err } @@ -560,11 +590,16 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods } func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...CommandModifier) error { + host, err := c.CredentialPatternFromRemote(ctx, remote) + if err != nil { + return err + } + args := []string{"pull", "--ff-only"} if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, host, args...) if err != nil { return err } @@ -575,8 +610,13 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman } func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { + host, err := c.CredentialPatternFromRemote(ctx, remote) + if err != nil { + return err + } + args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, host, args...) if err != nil { return err } @@ -587,6 +627,11 @@ func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...Co } func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods ...CommandModifier) (string, error) { + host, err := CredentialPatternFromGitURL(cloneURL) + if err != nil { + return "", err + } + cloneArgs, target := parseCloneArgs(args) cloneArgs = append(cloneArgs, cloneURL) // If the args contain an explicit target, pass it to clone otherwise, @@ -601,7 +646,7 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods } } cloneArgs = append([]string{"clone"}, cloneArgs...) - cmd, err := c.AuthenticatedCommand(ctx, cloneArgs...) + cmd, err := c.AuthenticatedCommand(ctx, host, cloneArgs...) if err != nil { return "", err } @@ -615,6 +660,17 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods return target, nil } +// WM-TODO: Bit of a weird method to hang off the client? +// WM-TODO: We need to make sure this handles command modifiers everywhere... +// WM-TODO: Are there any funny refspec usages that might not resolve via get-url +func (c *Client) CredentialPatternFromRemote(ctx context.Context, remote string) (CredentialPattern, error) { + gitURL, err := c.GetRemoteURL(ctx, remote) + if err != nil { + return CredentialPattern{}, err + } + return CredentialPatternFromGitURL(gitURL) +} + func resolveGitPath() (string, error) { path, err := safeexec.LookPath("git") if err != nil { @@ -729,3 +785,15 @@ var globReplacer = strings.NewReplacer( func escapeGlob(p string) string { return globReplacer.Replace(p) } + +// Cool cool cool... +// YOLO +func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { + normalizedURL, err := ParseURL(gitURL) + if err != nil { + return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err) + } + return CredentialPattern{ + pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"), + }, nil +} diff --git a/git/client_test.go b/git/client_test.go index 1e3032317..2bafaf78f 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -69,11 +69,11 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "adds credential helper config options", path: "path/to/gh", - wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, + wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, { name: "fallback when GhPath is not set", - wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, + wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"gh" auth git-credential`, "fetch"}, }, } for _, tt := range tests { @@ -82,7 +82,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { GhPath: tt.path, GitPath: "path/to/git", } - cmd, err := client.AuthenticatedCommand(context.Background(), "fetch") + cmd, err := client.AuthenticatedCommand(context.Background(), CredentialPattern{pattern: "https://github.com"}, "fetch") assert.NoError(t, err) assert.Equal(t, tt.wantArgs, cmd.Args) }) @@ -1064,13 +1064,13 @@ func TestClientUnsetRemoteResolution(t *testing.T) { name: "unset remote resolution", wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, }, - { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, - wantErrorMsg: "failed to run git: git error message", - }, + // { + // name: "git error", + // cmdExitStatus: 1, + // cmdStderr: "git error message", + // wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, + // wantErrorMsg: "failed to run git: git error message", + // }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1131,44 +1131,74 @@ func TestClientSetRemoteBranches(t *testing.T) { func TestClientFetch(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "fetch", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "fetch", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on get-url", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 1, + Stderr: "get-url error message", + }, + }, + wantErrorMsg: "failed to run git: get-url error message", + }, + { + name: "git error on fetch", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 1, + Stderr: "fetch error message", + }, + }, + wantErrorMsg: "failed to run git: fetch error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Fetch(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1176,44 +1206,74 @@ func TestClientFetch(t *testing.T) { func TestClientPull(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "pull", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "pull", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on get-url", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 1, + Stderr: "get-url error message", + }, + }, + wantErrorMsg: "failed to run git: get-url error message", + }, + { + name: "git error on fetch", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 1, + Stderr: "fetch error message", + }, + }, + wantErrorMsg: "failed to run git: fetch error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Pull(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1221,44 +1281,74 @@ func TestClientPull(t *testing.T) { func TestClientPush(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "push", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "push", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on get-url", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 1, + Stderr: "get-url error message", + }, + }, + wantErrorMsg: "failed to run git: get-url error message", + }, + { + name: "git error on fetch", + commands: map[args]commandResult{ + `path/to/git remote get-url origin`: { + ExitStatus: 0, + Stdout: "https://github.com/cli/nonexistent.git", + }, + `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 1, + Stderr: "fetch error message", + }, + }, + wantErrorMsg: "failed to run git: fetch error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Push(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1279,14 +1369,14 @@ func TestClientClone(t *testing.T) { { name: "clone", args: []string{}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { name: "accepts command modifiers", args: []string{}, mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { @@ -1294,19 +1384,19 @@ func TestClientClone(t *testing.T) { args: []string{}, cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantErrorMsg: "failed to run git: git error message", }, { name: "bare clone", args: []string{"--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli`, wantTarget: "cli.git", }, { name: "bare clone with explicit target", args: []string{"cli-bare", "--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli cli-bare`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli cli-bare`, wantTarget: "cli-bare", }, } @@ -1317,7 +1407,7 @@ func TestClientClone(t *testing.T) { GitPath: "path/to/git", commandContext: cmdCtx, } - target, err := client.Clone(context.Background(), "github.com/cli/cli", tt.args, tt.mods...) + target, err := client.Clone(context.Background(), "https://github.com/cli/cli", tt.args, tt.mods...) assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { assert.NoError(t, err) @@ -1442,6 +1532,49 @@ func initRepo(t *testing.T, dir string) { assert.NoError(t, err) } +type args string + +type commandResult struct { + ExitStatus int `json:"exitStatus"` + Stdout string `json:"out"` + Stderr string `json:"err"` +} + +type mockedCommands map[args]commandResult + +func TestCommandMocking(t *testing.T) { + if os.Getenv("GH_WANT_HELPER_PROCESS_RICH") != "1" { + return + } + + // WM-TODO: maybe don't use 255, I only picked it because it was distinct + jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") + if !ok { + fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(255) + } + + var commands mockedCommands + if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { + fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(255) + } + + // The discarded args are those for the go test binary itself, e.g. `-test.run=TestHelperProcessRich` + realArgs := os.Args[3:] + + commandResult, ok := commands[args(strings.Join(realArgs, " "))] + if !ok { + fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) + os.Exit(255) + } + + // WM-TODO: maybe pointer on these fields, or only print if not-empty + fmt.Fprint(os.Stdout, commandResult.Stdout) + fmt.Fprint(os.Stderr, commandResult.Stderr) + os.Exit(commandResult.ExitStatus) +} + func TestHelperProcess(t *testing.T) { if os.Getenv("GH_WANT_HELPER_PROCESS") != "1" { return @@ -1479,3 +1612,20 @@ func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) ( return cmd } } + +func createMockedCommandContext(t *testing.T, commands mockedCommands) commandCtx { + marshaledCommands, err := json.Marshal(commands) + require.NoError(t, err) + + return func(ctx context.Context, exe string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestCommandMocking", "--") + cmd.Env = []string{ + "GH_WANT_HELPER_PROCESS_RICH=1", + fmt.Sprintf("GH_HELPER_PROCESS_RICH_COMMANDS=%s", string(marshaledCommands)), + } + + cmd.Args = append(cmd.Args, exe) + cmd.Args = append(cmd.Args, args...) + return cmd + } +} diff --git a/git/command.go b/git/command.go index 8065ffd86..70a156912 100644 --- a/git/command.go +++ b/git/command.go @@ -98,3 +98,9 @@ func WithRepoDir(repoDir string) CommandModifier { gc.setRepoDir(repoDir) } } + +func WithExtraArgs(args ...string) CommandModifier { + return func(gc *Command) { + gc.Args = append(gc.Args, args...) + } +} diff --git a/internal/run/stub.go b/internal/run/stub.go index 49bf62d29..9499da8e2 100644 --- a/internal/run/stub.go +++ b/internal/run/stub.go @@ -9,7 +9,7 @@ import ( ) const ( - gitAuthRE = `-c credential.helper= -c credential.helper=!"[^"]+" auth git-credential ` + gitAuthRE = `-c credential\..+\.helper= -c credential\..+\.helper=!"[^"]+" auth git-credential ` ) type T interface { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index c35bcb845..28642eb51 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -249,7 +249,7 @@ func TestDevelopRun(t *testing.T) { expectedOut: heredoc.Doc(` Showing linked branches for OWNER/REPO#42 - + BRANCH URL foo https://github.com/OWNER/REPO/tree/foo bar https://github.com/OWNER/OTHER-REPO/tree/bar @@ -314,6 +314,7 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-issue-1:refs/remotes/origin/my-issue-1`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-issue-1\n", @@ -360,6 +361,7 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-issue-1:refs/remotes/origin/my-issue-1`, 0, "") }, expectedOut: "github.com/OWNER2/REPO/tree/my-issue-1\n", @@ -398,6 +400,7 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", @@ -467,9 +470,11 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") cs.Register(`git rev-parse --verify refs/heads/my-branch`, 0, "") cs.Register(`git checkout my-branch`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git pull --ff-only origin my-branch`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", @@ -509,6 +514,7 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") cs.Register(`git rev-parse --verify refs/heads/my-branch`, 1, "") cs.Register(`git checkout -b my-branch --track origin/my-branch`, 0, "") diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index f566cbe1d..4a732a409 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -130,7 +130,14 @@ func checkoutRun(opts *CheckoutOptions) error { cmdQueue = append(cmdQueue, []string{"submodule", "update", "--init", "--recursive"}) } - err = executeCmds(opts.GitClient, cmdQueue) + // Note that although we will probably be fetching from the headRemote, in practice, PR checkout can only + // ever point to one host, and we know baseRemote must be populated, where headRemote might be nil (e.g. when + // it was deleted). + credentialPattern, err := opts.GitClient.CredentialPatternFromRemote(context.Background(), baseRemote.Name) + if err != nil { + return err + } + err = executeCmds(opts.GitClient, credentialPattern, cmdQueue) if err != nil { return err } @@ -240,12 +247,12 @@ func localBranchExists(client *git.Client, b string) bool { return err == nil } -func executeCmds(client *git.Client, cmdQueue [][]string) error { +func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cmdQueue [][]string) error { for _, args := range cmdQueue { var err error var cmd *git.Command if args[0] == "fetch" || args[0] == "submodule" { - cmd, err = client.AuthenticatedCommand(context.Background(), args...) + cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) } else { cmd, err = client.Command(context.Background(), args...) } diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 1eda5dda2..55123fd59 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -94,6 +94,7 @@ func Test_checkoutRun(t *testing.T) { "origin": "OWNER/REPO", }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -124,6 +125,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git show-ref --verify -- refs/heads/foobar`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git checkout -b foobar --track origin/feature`, 0, "") }, @@ -151,6 +153,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git config branch\.foobar\.merge`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:foobar`, 0, "") cs.Register(`git checkout foobar`, 0, "") cs.Register(`git config branch\.foobar\.remote https://github.com/hubot/REPO.git`, 0, "") @@ -276,6 +279,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track origin/feature`, 0, "") @@ -296,6 +300,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -329,6 +334,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track robot-fork/feature`, 0, "") @@ -350,6 +356,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -373,6 +380,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -393,6 +401,7 @@ func TestPRCheckout_detachedHead(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -413,6 +422,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git merge --ff-only FETCH_HEAD`, 0, "") @@ -450,6 +460,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -472,6 +483,7 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -494,6 +506,7 @@ func TestPRCheckout_force(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -517,6 +530,7 @@ func TestPRCheckout_detach(t *testing.T) { defer cmdTeardown(t) cs.Register(`git checkout --detach FETCH_HEAD`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") output, err := runCommand(http, nil, "", `123 --detach`) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index d31174999..7a2ccb7e6 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -628,6 +628,7 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -692,6 +693,7 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -739,6 +741,7 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -791,6 +794,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1069,6 +1073,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1102,6 +1107,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 21df882b4..ff9f6ef5e 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -644,6 +644,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -695,6 +696,7 @@ func TestPrMerge_deleteBranch_nonDefault(t *testing.T) { cs.Register(`git checkout fruit`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -744,6 +746,7 @@ func TestPrMerge_deleteBranch_onlyLocally(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -794,6 +797,7 @@ func TestPrMerge_deleteBranch_checkoutNewBranch(t *testing.T) { cs.Register(`git checkout -b fruit --track origin/fruit`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -1081,6 +1085,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") pm := &prompter.PrompterMock{ @@ -1324,6 +1329,7 @@ func TestPRMergeTTY_withDeleteBranch(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") pm := &prompter.PrompterMock{ diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 471ed05dd..fe122690b 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -259,6 +259,7 @@ func Test_RepoClone_hasParent(t *testing.T) { cs.Register(`git clone https://github.com/OWNER/REPO.git`, 0, "") cs.Register(`git -C REPO remote add -t trunk upstream https://github.com/hubot/ORIG.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/hubot/ORIG.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO remote set-branches upstream *`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") @@ -299,6 +300,7 @@ func Test_RepoClone_hasParent_upstreamRemoteName(t *testing.T) { cs.Register(`git clone https://github.com/OWNER/REPO.git`, 0, "") cs.Register(`git -C REPO remote add -t trunk test https://github.com/hubot/ORIG.git`, 0, "") + cs.Register(`git -C REPO remote get-url test`, 0, "https://github.com/hubot/ORIG.git") cs.Register(`git -C REPO fetch test`, 0, "") cs.Register(`git -C REPO remote set-branches test *`, 0, "") cs.Register(`git -C REPO config --add remote.test.gh-resolved base`, 0, "") diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 79c349aa4..29cf453c7 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,7 +676,12 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), "push", baseRemote, "--mirror") + // WM-TODO: can we collapse this into opts.GitClient.Push? + credentialPattern, err := opts.GitClient.CredentialPatternFromRemote(context.Background(), baseRemote) + if err != nil { + return err + } + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), credentialPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index c33cfdad6..501e721fc 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -507,6 +507,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push origin --mirror`, 0, "") }, wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Mirrored all refs to https://github.com/OWNER/REPO.git\n", @@ -575,6 +576,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".git") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push --set-upstream origin HEAD`, 0, "") }, wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Pushed commits to https://github.com/OWNER/REPO.git\n", @@ -795,6 +797,7 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") + cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push origin --mirror`, 0, "") }, wantStdout: "https://github.com/OWNER/REPO\n", diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 0f94496f0..95b27ae60 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -442,6 +442,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone --depth 1 https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -474,6 +475,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/gamehendge/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -490,6 +492,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -526,6 +529,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -550,6 +554,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -586,6 +591,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -601,6 +607,7 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -691,6 +698,7 @@ func TestRepoFork(t *testing.T) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index 1f18d0fdb..3d48ad7b3 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -54,8 +54,13 @@ func (g *gitExecuter) CurrentBranch() (string, error) { } func (g *gitExecuter) Fetch(remote, ref string) error { + host, err := g.client.CredentialPatternFromRemote(context.Background(), remote) + if err != nil { + return err + } + args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), host, args...) if err != nil { return err } From 5f5c5270c9fe3f61b88ce53f0cc136b47b4c924b Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 21:38:25 +0100 Subject: [PATCH 02/17] Allow opt-in to insecure pattern --- git/client.go | 27 +++++++++++++++++---------- git/client_test.go | 28 +++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/git/client.go b/git/client.go index ce204c50e..8a8159134 100644 --- a/git/client.go +++ b/git/client.go @@ -95,27 +95,34 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } -// WM-TODO: not sure about this type, but I want to ensure that all call sites are provding the host, -// which is hard if the signature of AuthenticatedCommand is (context.Context, host string, args ...string) -// because this means AuthenticatedCommand(ctx, "fetch") will not be a compile error. type CredentialPattern struct { - pattern string + insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern + pattern string } +var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} +var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { - if credentialPattern.pattern == "" { - panic("get your shit together") - } - - preArgs := []string{"-c", fmt.Sprintf("credential.%s.helper=", credentialPattern.pattern)} if c.GhPath == "" { // Assumes that gh is in PATH. c.GhPath = "gh" } credHelper := fmt.Sprintf("!%q auth git-credential", c.GhPath) - preArgs = append(preArgs, "-c", fmt.Sprintf("credential.%s.helper=%s", credentialPattern.pattern, credHelper)) + + var preArgs []string + if credentialPattern == disallowedCredentialPattern { + return nil, fmt.Errorf("empty credential pattern is not allowed execept explicitly") + } else if credentialPattern == InsecureAllMatchingCredentialsPattern { + preArgs = []string{"-c", "credential.helper="} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + } else { + preArgs = []string{"-c", fmt.Sprintf("credential.%s.helper=", credentialPattern.pattern)} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.%s.helper=%s", credentialPattern.pattern, credHelper)) + } + args = append(preArgs, args...) return c.Command(ctx, args...) } diff --git a/git/client_test.go b/git/client_test.go index 2bafaf78f..cf9abc54c 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -64,16 +64,31 @@ func TestClientAuthenticatedCommand(t *testing.T) { tests := []struct { name string path string + pattern CredentialPattern wantArgs []string + wantErr error }{ { - name: "adds credential helper config options", + name: "when credential pattern is TODO, credential helper matches everything", path: "path/to/gh", + pattern: InsecureAllMatchingCredentialsPattern, + wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, + }, + { + name: "when credential pattern is set, credential helper only matches that pattern", + path: "path/to/gh", + pattern: CredentialPattern{pattern: "https://github.com"}, wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, { name: "fallback when GhPath is not set", - wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"gh" auth git-credential`, "fetch"}, + pattern: InsecureAllMatchingCredentialsPattern, + wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, + }, + { + name: "errors when attempting to use an empty pattern that isn't marked insecure", + pattern: CredentialPattern{insecure: false, pattern: ""}, + wantErr: fmt.Errorf("empty credential pattern is not allowed execept explicitly"), }, } for _, tt := range tests { @@ -82,9 +97,12 @@ func TestClientAuthenticatedCommand(t *testing.T) { GhPath: tt.path, GitPath: "path/to/git", } - cmd, err := client.AuthenticatedCommand(context.Background(), CredentialPattern{pattern: "https://github.com"}, "fetch") - assert.NoError(t, err) - assert.Equal(t, tt.wantArgs, cmd.Args) + cmd, err := client.AuthenticatedCommand(context.Background(), tt.pattern, "fetch") + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.Equal(t, tt.wantArgs, cmd.Args) }) } } From 75712de712cdca8ad11b585c67febf23dd742b24 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 21:50:19 +0100 Subject: [PATCH 03/17] Allow client pull to use insecure credential pattern --- git/client.go | 7 +---- git/client_test.go | 38 ++++++--------------------- internal/run/stub.go | 2 +- pkg/cmd/issue/develop/develop_test.go | 3 +-- pkg/cmd/pr/merge/merge_test.go | 6 ----- 5 files changed, 11 insertions(+), 45 deletions(-) diff --git a/git/client.go b/git/client.go index 8a8159134..658fb40cd 100644 --- a/git/client.go +++ b/git/client.go @@ -597,16 +597,11 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods } func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...CommandModifier) error { - host, err := c.CredentialPatternFromRemote(ctx, remote) - if err != nil { - return err - } - args := []string{"pull", "--ff-only"} if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, host, args...) + cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/git/client_test.go b/git/client_test.go index cf9abc54c..d48a6af08 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1157,11 +1157,7 @@ func TestClientFetch(t *testing.T) { { name: "fetch", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { ExitStatus: 0, }, }, @@ -1232,11 +1228,7 @@ func TestClientPull(t *testing.T) { { name: "pull", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { ExitStatus: 0, }, }, @@ -1249,34 +1241,20 @@ func TestClientPull(t *testing.T) { ExitStatus: 0, Stdout: "https://github.com/cli/nonexistent.git", }, - `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { ExitStatus: 0, }, }, }, { - name: "git error on get-url", + name: "git error on pull", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { ExitStatus: 1, - Stderr: "get-url error message", + Stderr: "pull error message", }, }, - wantErrorMsg: "failed to run git: get-url error message", - }, - { - name: "git error on fetch", - commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { - ExitStatus: 1, - Stderr: "fetch error message", - }, - }, - wantErrorMsg: "failed to run git: fetch error message", + wantErrorMsg: "failed to run git: pull error message", }, } @@ -1348,7 +1326,7 @@ func TestClientPush(t *testing.T) { }, `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { ExitStatus: 1, - Stderr: "fetch error message", + Stderr: "push error message", }, }, wantErrorMsg: "failed to run git: fetch error message", diff --git a/internal/run/stub.go b/internal/run/stub.go index 9499da8e2..5cd3c6de5 100644 --- a/internal/run/stub.go +++ b/internal/run/stub.go @@ -9,7 +9,7 @@ import ( ) const ( - gitAuthRE = `-c credential\..+\.helper= -c credential\..+\.helper=!"[^"]+" auth git-credential ` + gitAuthRE = `-c credential(?:\..+)?\.helper= -c credential(?:\..+)?\.helper=!"[^"]+" auth git-credential ` ) type T interface { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index 28642eb51..655668b46 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -314,7 +314,7 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/my-issue-1:refs/remotes/origin/my-issue-1`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-issue-1\n", @@ -474,7 +474,6 @@ func TestDevelopRun(t *testing.T) { cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") cs.Register(`git rev-parse --verify refs/heads/my-branch`, 0, "") cs.Register(`git checkout my-branch`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git pull --ff-only origin my-branch`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index ff9f6ef5e..21df882b4 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -644,7 +644,6 @@ func TestPrMerge_deleteBranch(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -696,7 +695,6 @@ func TestPrMerge_deleteBranch_nonDefault(t *testing.T) { cs.Register(`git checkout fruit`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -746,7 +744,6 @@ func TestPrMerge_deleteBranch_onlyLocally(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -797,7 +794,6 @@ func TestPrMerge_deleteBranch_checkoutNewBranch(t *testing.T) { cs.Register(`git checkout -b fruit --track origin/fruit`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) @@ -1085,7 +1081,6 @@ func TestPrMerge_alreadyMerged(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") pm := &prompter.PrompterMock{ @@ -1329,7 +1324,6 @@ func TestPRMergeTTY_withDeleteBranch(t *testing.T) { cs.Register(`git checkout main`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git pull --ff-only`, 0, "") pm := &prompter.PrompterMock{ From 7affcadb5e247f6408018a6d6865b8bc94ff5940 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 21:55:15 +0100 Subject: [PATCH 04/17] Allow client push to use insecure credential pattern --- git/client.go | 7 +---- git/client_test.go | 46 ++++++++---------------------- git/command.go | 6 ---- pkg/cmd/pr/create/create_test.go | 6 ---- pkg/cmd/repo/create/create.go | 7 +---- pkg/cmd/repo/create/create_test.go | 3 -- 6 files changed, 14 insertions(+), 61 deletions(-) diff --git a/git/client.go b/git/client.go index 658fb40cd..fc386d76f 100644 --- a/git/client.go +++ b/git/client.go @@ -612,13 +612,8 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman } func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { - host, err := c.CredentialPatternFromRemote(ctx, remote) - if err != nil { - return err - } - args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, host, args...) + cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/git/client_test.go b/git/client_test.go index d48a6af08..d680c2f00 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1082,13 +1082,13 @@ func TestClientUnsetRemoteResolution(t *testing.T) { name: "unset remote resolution", wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, }, - // { - // name: "git error", - // cmdExitStatus: 1, - // cmdStderr: "git error message", - // wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, - // wantErrorMsg: "failed to run git: git error message", - // }, + { + name: "git error", + cmdExitStatus: 1, + cmdStderr: "git error message", + wantCmdArgs: `path/to/git config --unset remote.origin.gh-resolved`, + wantErrorMsg: "failed to run git: git error message", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1285,11 +1285,7 @@ func TestClientPush(t *testing.T) { { name: "push", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { ExitStatus: 0, }, }, @@ -1298,38 +1294,20 @@ func TestClientPush(t *testing.T) { name: "accepts command modifiers", mods: []CommandModifier{WithRepoDir("/path/to/repo")}, commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { ExitStatus: 0, }, }, }, { - name: "git error on get-url", + name: "git error on push", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 1, - Stderr: "get-url error message", - }, - }, - wantErrorMsg: "failed to run git: get-url error message", - }, - { - name: "git error on fetch", - commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { ExitStatus: 1, Stderr: "push error message", }, }, - wantErrorMsg: "failed to run git: fetch error message", + wantErrorMsg: "failed to run git: push error message", }, } diff --git a/git/command.go b/git/command.go index 70a156912..8065ffd86 100644 --- a/git/command.go +++ b/git/command.go @@ -98,9 +98,3 @@ func WithRepoDir(repoDir string) CommandModifier { gc.setRepoDir(repoDir) } } - -func WithExtraArgs(args ...string) CommandModifier { - return func(gc *Command) { - gc.Args = append(gc.Args, args...) - } -} diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 7a2ccb7e6..d31174999 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -628,7 +628,6 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -693,7 +692,6 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -741,7 +739,6 @@ func Test_createRun(t *testing.T) { cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -794,7 +791,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1073,7 +1069,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1107,7 +1102,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 29cf453c7..c732a3759 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,12 +676,7 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - // WM-TODO: can we collapse this into opts.GitClient.Push? - credentialPattern, err := opts.GitClient.CredentialPatternFromRemote(context.Background(), baseRemote) - if err != nil { - return err - } - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), credentialPattern, "push", baseRemote, "--mirror") + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index 501e721fc..c33cfdad6 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -507,7 +507,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") - cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push origin --mirror`, 0, "") }, wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Mirrored all refs to https://github.com/OWNER/REPO.git\n", @@ -576,7 +575,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".git") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") - cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push --set-upstream origin HEAD`, 0, "") }, wantStdout: "✓ Created repository OWNER/REPO on GitHub\n https://github.com/OWNER/REPO\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Pushed commits to https://github.com/OWNER/REPO.git\n", @@ -797,7 +795,6 @@ func Test_createRun(t *testing.T) { cs.Register(`git -C . rev-parse --git-dir`, 0, ".") cs.Register(`git -C . rev-parse HEAD`, 0, "commithash") cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "") - cs.Register(`git -C . remote get-url origin`, 0, "https://github.com/OWNER/REPO") cs.Register(`git -C . push origin --mirror`, 0, "") }, wantStdout: "https://github.com/OWNER/REPO\n", From 6b7f1ff06072858daa35b378be3544f932e6eabd Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 22:08:45 +0100 Subject: [PATCH 05/17] Allow client fetch to use insecure credentials pattern --- git/client.go | 7 +------ git/client_test.go | 26 ++------------------------ pkg/cmd/issue/develop/develop_test.go | 5 ----- pkg/cmd/pr/checkout/checkout.go | 7 +++++-- pkg/cmd/repo/clone/clone_test.go | 2 -- pkg/cmd/repo/fork/fork_test.go | 8 -------- 6 files changed, 8 insertions(+), 47 deletions(-) diff --git a/git/client.go b/git/client.go index fc386d76f..d2919ad22 100644 --- a/git/client.go +++ b/git/client.go @@ -577,16 +577,11 @@ func (c *Client) AddRemote(ctx context.Context, name, urlStr string, trackingBra // Below are commands that make network calls and need authentication credentials supplied from gh. func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods ...CommandModifier) error { - host, err := c.CredentialPatternFromRemote(ctx, remote) - if err != nil { - return err - } - args := []string{"fetch", remote} if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, host, args...) + cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/git/client_test.go b/git/client_test.go index d680c2f00..614f05995 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1166,33 +1166,15 @@ func TestClientFetch(t *testing.T) { name: "accepts command modifiers", mods: []CommandModifier{WithRepoDir("/path/to/repo")}, commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { ExitStatus: 0, }, }, }, - { - name: "git error on get-url", - commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 1, - Stderr: "get-url error message", - }, - }, - wantErrorMsg: "failed to run git: get-url error message", - }, { name: "git error on fetch", commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, - `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential fetch origin trunk`: { + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { ExitStatus: 1, Stderr: "fetch error message", }, @@ -1237,10 +1219,6 @@ func TestClientPull(t *testing.T) { name: "accepts command modifiers", mods: []CommandModifier{WithRepoDir("/path/to/repo")}, commands: map[args]commandResult{ - `path/to/git remote get-url origin`: { - ExitStatus: 0, - Stdout: "https://github.com/cli/nonexistent.git", - }, `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { ExitStatus: 0, }, diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index 655668b46..abdebf0c8 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -314,7 +314,6 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/my-issue-1:refs/remotes/origin/my-issue-1`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-issue-1\n", @@ -361,7 +360,6 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-issue-1:refs/remotes/origin/my-issue-1`, 0, "") }, expectedOut: "github.com/OWNER2/REPO/tree/my-issue-1\n", @@ -400,7 +398,6 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", @@ -470,7 +467,6 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") cs.Register(`git rev-parse --verify refs/heads/my-branch`, 0, "") cs.Register(`git checkout my-branch`, 0, "") @@ -513,7 +509,6 @@ func TestDevelopRun(t *testing.T) { ) }, runStubs: func(cs *run.CommandStubber) { - cs.Register(`git remote get-url origin`, 0, "https://github.com/cli/cli.git") cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") cs.Register(`git rev-parse --verify refs/heads/my-branch`, 1, "") cs.Register(`git checkout -b my-branch --track origin/my-branch`, 0, "") diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 4a732a409..a4cb405c1 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -251,9 +251,12 @@ func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cm for _, args := range cmdQueue { var err error var cmd *git.Command - if args[0] == "fetch" || args[0] == "submodule" { + switch args[0] { + case "submodule": cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) - } else { + case "fetch": + cmd, err = client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) + default: cmd, err = client.Command(context.Background(), args...) } if err != nil { diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index fe122690b..471ed05dd 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -259,7 +259,6 @@ func Test_RepoClone_hasParent(t *testing.T) { cs.Register(`git clone https://github.com/OWNER/REPO.git`, 0, "") cs.Register(`git -C REPO remote add -t trunk upstream https://github.com/hubot/ORIG.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/hubot/ORIG.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO remote set-branches upstream *`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") @@ -300,7 +299,6 @@ func Test_RepoClone_hasParent_upstreamRemoteName(t *testing.T) { cs.Register(`git clone https://github.com/OWNER/REPO.git`, 0, "") cs.Register(`git -C REPO remote add -t trunk test https://github.com/hubot/ORIG.git`, 0, "") - cs.Register(`git -C REPO remote get-url test`, 0, "https://github.com/hubot/ORIG.git") cs.Register(`git -C REPO fetch test`, 0, "") cs.Register(`git -C REPO remote set-branches test *`, 0, "") cs.Register(`git -C REPO config --add remote.test.gh-resolved base`, 0, "") diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 95b27ae60..0f94496f0 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -442,7 +442,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone --depth 1 https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -475,7 +474,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/gamehendge/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -492,7 +490,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -529,7 +526,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -554,7 +550,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -591,7 +586,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -607,7 +601,6 @@ func TestRepoFork(t *testing.T) { execStubs: func(cs *run.CommandStubber) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, @@ -698,7 +691,6 @@ func TestRepoFork(t *testing.T) { cs.Register(`git clone https://github.com/someone/REPO\.git`, 128, "") cs.Register(`git clone https://github.com/someone/REPO\.git`, 0, "") cs.Register(`git -C REPO remote add upstream https://github\.com/OWNER/REPO\.git`, 0, "") - cs.Register(`git -C REPO remote get-url upstream`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, From 19d62826d6a53b28bdc626641fe032e59e412581 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 22:19:55 +0100 Subject: [PATCH 06/17] Allow repo sync fetch to use insecure credentials pattern --- pkg/cmd/repo/sync/git.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index 3d48ad7b3..b0bd26abd 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -54,13 +54,8 @@ func (g *gitExecuter) CurrentBranch() (string, error) { } func (g *gitExecuter) Fetch(remote, ref string) error { - host, err := g.client.CredentialPatternFromRemote(context.Background(), remote) - if err != nil { - return err - } - args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), host, args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) if err != nil { return err } From efd8ff6d469a80e2cb4f9e714be116b01bdf94f6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 22:21:05 +0100 Subject: [PATCH 07/17] General cleanup and docs --- git/client.go | 33 ++++++++++++++++++++++----------- git/client_test.go | 18 +++++++++++------- pkg/cmd/pr/checkout/checkout.go | 2 +- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/git/client.go b/git/client.go index d2919ad22..f1b357634 100644 --- a/git/client.go +++ b/git/client.go @@ -95,14 +95,36 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } +// CredentialPattern is used to indicate to AuthenticatedCommand which patterns git should match +// against when trying to find credentials. It is a little overengineered as a type because we +// want AuthenticatedCommand to have a clear complication error when this is not provided, +// as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. +// +// It is only usable when constructed by another function in the package because the empty pattern, +// without insecure set to true, will result in an error in AuthenticatedCommand. +// +// Callers can currently opt-in to an insecure mode for backwards compatability by using +// InsecureAllMatchingCredentialsPattern. type CredentialPattern struct { insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern pattern string } +// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatability. var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} +// WM-TODO: Should this handle command modifiers, e.g. RepoDir being set in Clone +// WM-TODO: Are there any funny remotes that might not resolve to a URL? +// WM-TODO: This should probably have its own tests +func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { + gitURL, err := c.GetRemoteURL(ctx, remote) + if err != nil { + return CredentialPattern{}, err + } + return CredentialPatternFromGitURL(gitURL) +} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { @@ -652,17 +674,6 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods return target, nil } -// WM-TODO: Bit of a weird method to hang off the client? -// WM-TODO: We need to make sure this handles command modifiers everywhere... -// WM-TODO: Are there any funny refspec usages that might not resolve via get-url -func (c *Client) CredentialPatternFromRemote(ctx context.Context, remote string) (CredentialPattern, error) { - gitURL, err := c.GetRemoteURL(ctx, remote) - if err != nil { - return CredentialPattern{}, err - } - return CredentialPatternFromGitURL(gitURL) -} - func resolveGitPath() (string, error) { path, err := safeexec.LookPath("git") if err != nil { diff --git a/git/client_test.go b/git/client_test.go index 614f05995..9cacf4e65 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1499,17 +1499,16 @@ func TestCommandMocking(t *testing.T) { return } - // WM-TODO: maybe don't use 255, I only picked it because it was distinct jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") if !ok { fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") - os.Exit(255) + os.Exit(1) } var commands mockedCommands if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") - os.Exit(255) + os.Exit(1) } // The discarded args are those for the go test binary itself, e.g. `-test.run=TestHelperProcessRich` @@ -1518,12 +1517,17 @@ func TestCommandMocking(t *testing.T) { commandResult, ok := commands[args(strings.Join(realArgs, " "))] if !ok { fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) - os.Exit(255) + os.Exit(1) + } + + if commandResult.Stdout != "" { + fmt.Fprint(os.Stdout, commandResult.Stdout) + } + + if commandResult.Stderr != "" { + fmt.Fprint(os.Stderr, commandResult.Stderr) } - // WM-TODO: maybe pointer on these fields, or only print if not-empty - fmt.Fprint(os.Stdout, commandResult.Stdout) - fmt.Fprint(os.Stderr, commandResult.Stderr) os.Exit(commandResult.ExitStatus) } diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index a4cb405c1..d7243af60 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -133,7 +133,7 @@ func checkoutRun(opts *CheckoutOptions) error { // Note that although we will probably be fetching from the headRemote, in practice, PR checkout can only // ever point to one host, and we know baseRemote must be populated, where headRemote might be nil (e.g. when // it was deleted). - credentialPattern, err := opts.GitClient.CredentialPatternFromRemote(context.Background(), baseRemote.Name) + credentialPattern, err := git.CredentialPatternFromRemote(context.Background(), opts.GitClient, baseRemote.Name) if err != nil { return err } From 0db05ff022aff65a3a3132c7445b865a9b5f8f1b Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 22:27:36 +0100 Subject: [PATCH 08/17] Add SSH remote todo --- git/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/git/client.go b/git/client.go index f1b357634..8cf4dff33 100644 --- a/git/client.go +++ b/git/client.go @@ -117,6 +117,7 @@ var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: "" // WM-TODO: Should this handle command modifiers, e.g. RepoDir being set in Clone // WM-TODO: Are there any funny remotes that might not resolve to a URL? // WM-TODO: This should probably have its own tests +// WM-TODO: Consider what to do if remote was SSH, it's probably not breaking, but maybe we want to do something better func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { gitURL, err := c.GetRemoteURL(ctx, remote) if err != nil { From ad397bd0a66e6865ea0ffc4d06c0d1ca204a49c8 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 26 Nov 2024 16:08:51 -0800 Subject: [PATCH 09/17] Fix typos and add tests for CredentialPatternFrom* functions --- git/client.go | 32 +++++++++--------- git/client_test.go | 83 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/git/client.go b/git/client.go index 8cf4dff33..9526d1249 100644 --- a/git/client.go +++ b/git/client.go @@ -96,21 +96,21 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) } // CredentialPattern is used to indicate to AuthenticatedCommand which patterns git should match -// against when trying to find credentials. It is a little overengineered as a type because we +// against when trying to find credentials. It is a little over-engineered as a type because we // want AuthenticatedCommand to have a clear complication error when this is not provided, // as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. // // It is only usable when constructed by another function in the package because the empty pattern, // without insecure set to true, will result in an error in AuthenticatedCommand. // -// Callers can currently opt-in to an insecure mode for backwards compatability by using +// Callers can currently opt-in to an insecure mode for backwards compatibility by using // InsecureAllMatchingCredentialsPattern. type CredentialPattern struct { insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern pattern string } -// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatability. +// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatibility. var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} @@ -126,6 +126,18 @@ func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) return CredentialPatternFromGitURL(gitURL) } +// Cool cool cool... +// YOLO +func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { + normalizedURL, err := ParseURL(gitURL) + if err != nil { + return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err) + } + return CredentialPattern{ + pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"), + }, nil +} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { @@ -137,7 +149,7 @@ func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern Cre var preArgs []string if credentialPattern == disallowedCredentialPattern { - return nil, fmt.Errorf("empty credential pattern is not allowed execept explicitly") + return nil, fmt.Errorf("empty credential pattern is not allowed unless provided explicitly") } else if credentialPattern == InsecureAllMatchingCredentialsPattern { preArgs = []string{"-c", "credential.helper="} preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) @@ -789,15 +801,3 @@ var globReplacer = strings.NewReplacer( func escapeGlob(p string) string { return globReplacer.Replace(p) } - -// Cool cool cool... -// YOLO -func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { - normalizedURL, err := ParseURL(gitURL) - if err != nil { - return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err) - } - return CredentialPattern{ - pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"), - }, nil -} diff --git a/git/client_test.go b/git/client_test.go index 9cacf4e65..72a780ae6 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -88,7 +88,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "errors when attempting to use an empty pattern that isn't marked insecure", pattern: CredentialPattern{insecure: false, pattern: ""}, - wantErr: fmt.Errorf("empty credential pattern is not allowed execept explicitly"), + wantErr: fmt.Errorf("empty credential pattern is not allowed except explicitly"), }, } for _, tt := range tests { @@ -1554,6 +1554,87 @@ func TestHelperProcess(t *testing.T) { os.Exit(0) } +func TestCredentialPatternFromGitURL(t *testing.T) { + tests := []struct { + name string + gitURL string + wantErr bool + wantCredentialPattern CredentialPattern + }{ + { + name: "Given a well formed gitURL, it returns the corresponding CredentialPattern", + gitURL: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + insecure: false, + }, + }, + { + name: "Given a malformed gitURL, it returns an error", + // This pattern is copied from the tests in ParseURL + // Unexpectedly, a non URL-like string did not error in ParseURL + gitURL: "ssh://git@[/tmp/git-repo", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credentialPattern, err := CredentialPatternFromGitURL(tt.gitURL) + if tt.wantErr { + assert.ErrorContains(t, err, "failed to parse remote URL") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + +func TestCredentialPatternFromRemote(t *testing.T) { + tests := []struct { + name string + remote string + wantCredentialPattern CredentialPattern + wantErr bool + }{ + { + name: "Given a well formed remote, it returns the corresponding CredentialPattern", + remote: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + insecure: false, + }, + }, + { + name: "Given an error from GetRemoteURL, it returns that error", + remote: "foo remote", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var cmdCtx func(ctx context.Context, name string, args ...string) *exec.Cmd + if tt.wantErr { + _, cmdCtx = createCommandContext(t, 1, tt.remote, "GetRemoteURL error") + } else { + _, cmdCtx = createCommandContext(t, 0, tt.remote, "") + } + + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + credentialPattern, err := CredentialPatternFromRemote(context.Background(), &client, tt.remote) + if tt.wantErr { + assert.ErrorContains(t, err, "GetRemoteURL error") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) (*exec.Cmd, commandCtx) { cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestHelperProcess", "--") cmd.Env = []string{ From 3773068f58ba7550a4b56d857abbeb67b2653278 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 12:06:17 +0100 Subject: [PATCH 10/17] Remove TODOs --- git/client.go | 11 ++++------- git/client_test.go | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/git/client.go b/git/client.go index 9526d1249..909de88d1 100644 --- a/git/client.go +++ b/git/client.go @@ -114,10 +114,7 @@ type CredentialPattern struct { var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} -// WM-TODO: Should this handle command modifiers, e.g. RepoDir being set in Clone // WM-TODO: Are there any funny remotes that might not resolve to a URL? -// WM-TODO: This should probably have its own tests -// WM-TODO: Consider what to do if remote was SSH, it's probably not breaking, but maybe we want to do something better func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { gitURL, err := c.GetRemoteURL(ctx, remote) if err != nil { @@ -126,8 +123,6 @@ func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) return CredentialPatternFromGitURL(gitURL) } -// Cool cool cool... -// YOLO func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { normalizedURL, err := ParseURL(gitURL) if err != nil { @@ -654,7 +649,9 @@ func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...Co } func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods ...CommandModifier) (string, error) { - host, err := CredentialPatternFromGitURL(cloneURL) + // Note that even if this is an SSH clone URL, we are setting the pattern anyway. + // We could write some code to prevent this, but it also doesn't seem harmful. + pattern, err := CredentialPatternFromGitURL(cloneURL) if err != nil { return "", err } @@ -673,7 +670,7 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods } } cloneArgs = append([]string{"clone"}, cloneArgs...) - cmd, err := c.AuthenticatedCommand(ctx, host, cloneArgs...) + cmd, err := c.AuthenticatedCommand(ctx, pattern, cloneArgs...) if err != nil { return "", err } diff --git a/git/client_test.go b/git/client_test.go index 72a780ae6..4ba6f0843 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -88,7 +88,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "errors when attempting to use an empty pattern that isn't marked insecure", pattern: CredentialPattern{insecure: false, pattern: ""}, - wantErr: fmt.Errorf("empty credential pattern is not allowed except explicitly"), + wantErr: fmt.Errorf("empty credential pattern is not allowed unless provided explicitly"), }, } for _, tt := range tests { From 0b4f087b466c47062ce6fa6a915558de207f22c7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 12:07:04 +0100 Subject: [PATCH 11/17] Fix CredentialPattern doc typos Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- git/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git/client.go b/git/client.go index 909de88d1..439199a46 100644 --- a/git/client.go +++ b/git/client.go @@ -95,9 +95,9 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } -// CredentialPattern is used to indicate to AuthenticatedCommand which patterns git should match +// CredentialPattern is used to inform AuthenticatedCommand which patterns Git should match // against when trying to find credentials. It is a little over-engineered as a type because we -// want AuthenticatedCommand to have a clear complication error when this is not provided, +// want AuthenticatedCommand to have a clear compilation error when this is not provided, // as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. // // It is only usable when constructed by another function in the package because the empty pattern, From 72a6fd00a47afc40f2e3030341223e8a2fc10c17 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 12:21:55 +0100 Subject: [PATCH 12/17] Rename backwards compatible credentials pattern --- git/client.go | 25 +++++++++++++------------ git/client_test.go | 16 ++++++++-------- pkg/cmd/pr/checkout/checkout.go | 2 +- pkg/cmd/repo/create/create.go | 2 +- pkg/cmd/repo/sync/git.go | 2 +- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/git/client.go b/git/client.go index 439199a46..b2cfbce45 100644 --- a/git/client.go +++ b/git/client.go @@ -101,18 +101,19 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) // as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. // // It is only usable when constructed by another function in the package because the empty pattern, -// without insecure set to true, will result in an error in AuthenticatedCommand. +// without allMatching set to true, will result in an error in AuthenticatedCommand. // -// Callers can currently opt-in to an insecure mode for backwards compatibility by using -// InsecureAllMatchingCredentialsPattern. +// Callers can currently opt-in to an slightly less secure mode for backwards compatibility by using +// AllMatchingCredentialsPattern. type CredentialPattern struct { - insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern - pattern string + allMatching bool // should only be constructable via AllMatchingCredentialsPattern + pattern string } -// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatibility. -var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""} -var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""} +// AllMatchingCredentialsPattern allows for setting gh as credential helper for all hosts. +// However, we should endeavour to remove it as it's less secure. +var AllMatchingCredentialsPattern = CredentialPattern{allMatching: true, pattern: ""} +var disallowedCredentialPattern = CredentialPattern{allMatching: false, pattern: ""} // WM-TODO: Are there any funny remotes that might not resolve to a URL? func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { @@ -145,7 +146,7 @@ func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern Cre var preArgs []string if credentialPattern == disallowedCredentialPattern { return nil, fmt.Errorf("empty credential pattern is not allowed unless provided explicitly") - } else if credentialPattern == InsecureAllMatchingCredentialsPattern { + } else if credentialPattern == AllMatchingCredentialsPattern { preArgs = []string{"-c", "credential.helper="} preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) } else { @@ -611,7 +612,7 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -626,7 +627,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -638,7 +639,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, InsecureAllMatchingCredentialsPattern, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } diff --git a/git/client_test.go b/git/client_test.go index 4ba6f0843..f643caa90 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -71,7 +71,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "when credential pattern is TODO, credential helper matches everything", path: "path/to/gh", - pattern: InsecureAllMatchingCredentialsPattern, + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, { @@ -82,12 +82,12 @@ func TestClientAuthenticatedCommand(t *testing.T) { }, { name: "fallback when GhPath is not set", - pattern: InsecureAllMatchingCredentialsPattern, + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, }, { - name: "errors when attempting to use an empty pattern that isn't marked insecure", - pattern: CredentialPattern{insecure: false, pattern: ""}, + name: "errors when attempting to use an empty pattern that isn't marked all matching", + pattern: CredentialPattern{allMatching: false, pattern: ""}, wantErr: fmt.Errorf("empty credential pattern is not allowed unless provided explicitly"), }, } @@ -1565,8 +1565,8 @@ func TestCredentialPatternFromGitURL(t *testing.T) { name: "Given a well formed gitURL, it returns the corresponding CredentialPattern", gitURL: "https://github.com/OWNER/REPO", wantCredentialPattern: CredentialPattern{ - pattern: "https://github.com", - insecure: false, + pattern: "https://github.com", + allMatching: false, }, }, { @@ -1601,8 +1601,8 @@ func TestCredentialPatternFromRemote(t *testing.T) { name: "Given a well formed remote, it returns the corresponding CredentialPattern", remote: "https://github.com/OWNER/REPO", wantCredentialPattern: CredentialPattern{ - pattern: "https://github.com", - insecure: false, + pattern: "https://github.com", + allMatching: false, }, }, { diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index d7243af60..c4549d89b 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -255,7 +255,7 @@ func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cm case "submodule": cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) case "fetch": - cmd, err = client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) + cmd, err = client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) default: cmd, err = client.Command(context.Background(), args...) } diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index c732a3759..ec7026759 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,7 +676,7 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, "push", baseRemote, "--mirror") + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index b0bd26abd..0a0be1ed6 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -55,7 +55,7 @@ func (g *gitExecuter) CurrentBranch() (string, error) { func (g *gitExecuter) Fetch(remote, ref string) error { args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), git.InsecureAllMatchingCredentialsPattern, args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) if err != nil { return err } From bd44d33eaabe6dc12db3e8f5a5d9b2298b19e736 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 13:06:35 +0100 Subject: [PATCH 13/17] Add checkout test that uses ssh git remote url --- pkg/cmd/pr/checkout/checkout_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 55123fd59..32f7b5b80 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -72,6 +72,32 @@ func Test_checkoutRun(t *testing.T) { wantStderr string wantErr bool }{ + { + name: "checkout with ssh remote URL", + opts: &CheckoutOptions{ + SelectorArg: "123", + Finder: func() shared.PRFinder { + baseRepo, pr := stubPR("OWNER/REPO:master", "OWNER/REPO:feature") + finder := shared.NewMockFinder("123", pr, baseRepo) + return finder + }(), + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + Branch: func() (string, error) { + return "main", nil + }, + }, + remotes: map[string]string{ + "origin": "OWNER/REPO", + }, + runStubs: func(cs *run.CommandStubber) { + cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") + cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") + cs.Register(`git checkout -b feature --track origin/feature`, 0, "") + }, + }, { name: "fork repo was deleted", opts: &CheckoutOptions{ From 21a14a7d1a6dc9561346b956c8ddc047d04fee9d Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 15:50:54 +0100 Subject: [PATCH 14/17] Update git/client_test.go Co-authored-by: Andy Feller --- git/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/client_test.go b/git/client_test.go index f643caa90..b39ae7acb 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -69,7 +69,7 @@ func TestClientAuthenticatedCommand(t *testing.T) { wantErr error }{ { - name: "when credential pattern is TODO, credential helper matches everything", + name: "when credential pattern allows for anything, credential helper matches everything", path: "path/to/gh", pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, From 622e283d2b3243686933c3a87d57344a7e56e6fd Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 15:51:38 +0100 Subject: [PATCH 15/17] Update git/client_test.go Co-authored-by: Andy Feller --- git/client_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/git/client_test.go b/git/client_test.go index b39ae7acb..65adf96da 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1494,6 +1494,7 @@ type commandResult struct { type mockedCommands map[args]commandResult +// TestCommandMocking is an invoked test helper that emulates expected behavior for predefined shell commands, erroring when unexpected conditions are encountered. func TestCommandMocking(t *testing.T) { if os.Getenv("GH_WANT_HELPER_PROCESS_RICH") != "1" { return From ec086a021b9feddf600c3fc0d35ef8e17023ee6b Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 27 Nov 2024 15:51:44 +0100 Subject: [PATCH 16/17] Update git/client_test.go Co-authored-by: Andy Feller --- git/client_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/git/client_test.go b/git/client_test.go index 65adf96da..0014ddc3f 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1655,6 +1655,7 @@ func createMockedCommandContext(t *testing.T, commands mockedCommands) commandCt marshaledCommands, err := json.Marshal(commands) require.NoError(t, err) + // invokes helper within current test binary, emulating desired behavior return func(ctx context.Context, exe string, args ...string) *exec.Cmd { cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestCommandMocking", "--") cmd.Env = []string{ From c94def8b515b113f38475f2e33b2520200a9854e Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 27 Nov 2024 15:54:38 -0500 Subject: [PATCH 17/17] Bump cli/go-gh for codespace fix --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 40193c4f8..a95ccacc0 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/cenkalti/backoff/v4 v4.3.0 github.com/charmbracelet/glamour v0.7.0 github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c - github.com/cli/go-gh/v2 v2.11.0 + github.com/cli/go-gh/v2 v2.11.1 github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 diff --git a/go.sum b/go.sum index d068d9d11..a7d87de7d 100644 --- a/go.sum +++ b/go.sum @@ -95,8 +95,8 @@ github.com/charmbracelet/x/exp/term v0.0.0-20240425164147-ba2a9512b05f/go.mod h1 github.com/cli/browser v1.0.0/go.mod h1:IEWkHYbLjkhtjwwWlwTHW2lGxeS5gezEQBMLTwDHf5Q= github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= -github.com/cli/go-gh/v2 v2.11.0 h1:TERLYMMWderKBO3lBff/JIu2+eSly2oFRgN2WvO+3eA= -github.com/cli/go-gh/v2 v2.11.0/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= +github.com/cli/go-gh/v2 v2.11.1 h1:amAyfqMWQTBdue8iTmDUegGZK7c8kk6WCxD9l/wLtGI= +github.com/cli/go-gh/v2 v2.11.1/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs=