From 68f3ef79cadfda52a5096f2651132e9f68b03d5e Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Fri, 18 Oct 2024 21:21:47 -0600 Subject: [PATCH 01/23] Handle missing "workflow" scope in createRelease --- pkg/cmd/release/create/create_test.go | 67 +++++++++++++++++++++++++++ pkg/cmd/release/create/http.go | 45 ++++++++++++++++++ pkg/httpmock/stub.go | 14 ++++++ 3 files changed, 126 insertions(+) diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 85c1c3f3f..aded1bea9 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -1082,6 +1083,72 @@ func Test_createRun(t *testing.T) { runStubs: defaultRunStubs, wantErr: "cannot generate release notes from tag v1.2.3 as it does not exist locally", }, + { + name: "API returns 404, OAuth token has no workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org`)) + }, + wantErr: heredoc.Doc(` + HTTP 404: Failed to create release, "workflow" scope may be required + To request it, run gh auth refresh -h github.com -s workflow + `), + }, + { + name: "API returns 404, OAuth token has workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org,workflow`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, + { + name: "API returns 404, not an OAuth token", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusStringResponse(404, `HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 84e95710f..1512b5341 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -8,12 +8,16 @@ import ( "io" "net/http" "net/url" + "strings" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/shurcooL/githubv4" + + ghauth "github.com/cli/go-gh/v2/pkg/auth" ) type tag struct { @@ -174,6 +178,26 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st } defer resp.Body.Close() + // This code checks if we received a 404 while attempting to create a release without + // the workflow scope, and if so, returns an error message that explains a possible + // solution to the user. + // + // If the same file (with both the same path and contents) exists + // on another branch in the repo, releases with workflow file changes can be + // created without the workflow scope. Otherwise, the workflow scope is + // required to create the release. + // + // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes + if resp.StatusCode == http.StatusNotFound && tokenMissingWorkflowScope(resp) { + normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) + errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` + HTTP 404: Failed to create release, "workflow" scope may be required + To request it, run gh auth refresh -h %[1]s -s workflow + `, normalizedHostname)) + + return nil, errMissingRequiredWorkflowScope + } + success := resp.StatusCode >= 200 && resp.StatusCode < 300 if !success { return nil, api.HandleHTTPError(resp) @@ -254,3 +278,24 @@ func deleteRelease(httpClient *http.Client, release *shared.Release) error { } return nil } + +// Given an http.Response, check if the token used in +// the request is missing the workflow scope. +func tokenMissingWorkflowScope(resp *http.Response) bool { + scopes := resp.Header.Get("X-Oauth-Scopes") + + // Return false when no scopes are present - no scopes in this header + // means that the user is probably authenticating with a token type other + // than an OAuth token, and we don't know what this token's scopes actually are. + if scopes == "" { + return false + } + + for _, s := range strings.Split(scopes, ",") { + if s == "workflow" { + return false + } + } + + return true +} diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 787cdcf9d..d1a27d835 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -238,6 +238,20 @@ func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) } } +// StatusScopesResponder returns a response with the given status code and OAuth scopes. +func StatusScopesResponder(status int, scopes string) func(*http.Request) (*http.Response, error) { + return func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: status, + Request: req, + Header: map[string][]string{ + "X-Oauth-Scopes": {scopes}, + }, + Body: io.NopCloser(bytes.NewBufferString("")), + }, nil + } +} + func httpResponse(status int, req *http.Request, body io.Reader) *http.Response { return httpResponseWithHeader(status, req, body, http.Header{}) } From 677ed2cdcfd6cefef65f1a59a6f44b9a8f0ba1c4 Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:20:30 -0700 Subject: [PATCH 02/23] Refactor command documentation to use heredoc --- pkg/cmd/cache/delete/delete.go | 24 ++++++++++++------------ pkg/cmd/codespace/rebuild.go | 10 +++++++--- pkg/cmd/repo/delete/delete.go | 13 ++++++++----- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 4794d1fbe..1835d87b1 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -35,23 +35,23 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete [| | --all]", Short: "Delete GitHub Actions caches", - Long: ` - Delete GitHub Actions caches. + Long: heredoc.Doc(` + Delete GitHub Actions caches. - Deletion requires authorization with the "repo" scope. -`, + Deletion requires authorization with the "repo" scope. + `), Example: heredoc.Doc(` - # Delete a cache by id - $ gh cache delete 1234 + # Delete a cache by id + $ gh cache delete 1234 - # Delete a cache by key - $ gh cache delete cache-key + # Delete a cache by key + $ gh cache delete cache-key - # Delete a cache by id in a specific repo - $ gh cache delete 1234 --repo cli/cli + # Delete a cache by id in a specific repo + $ gh cache delete 1234 --repo cli/cli - # Delete all caches - $ gh cache delete --all + # Delete all caches + $ gh cache delete --all `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 93d43bf29..565406edc 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/internal/codespaces/portforwarder" @@ -20,9 +21,12 @@ func newRebuildCmd(app *App) *cobra.Command { rebuildCmd := &cobra.Command{ Use: "rebuild", Short: "Rebuild a codespace", - Long: `Rebuilding recreates your codespace. Your code and any current changes will be -preserved. Your codespace will be rebuilt using your working directory's -dev container. A full rebuild also removes cached Docker images.`, + Long: heredoc.Doc(` + Rebuilding recreates your codespace. + + Your code and any current changes will be preserved. Your codespace will be rebuilt using + your working directory's dev container. A full rebuild also removes cached Docker images. + `), Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { return app.Rebuild(cmd.Context(), selector, fullRebuild) diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index 722d748b9..e6fbf6363 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -38,12 +39,14 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete []", Short: "Delete a repository", - Long: `Delete a GitHub repository. + Long: heredoc.Doc(` + Delete a GitHub repository. + + With no argument, deletes the current repository. Otherwise, deletes the specified repository. -With no argument, deletes the current repository. Otherwise, deletes the specified repository. - -Deletion requires authorization with the "delete_repo" scope. -To authorize, run "gh auth refresh -s delete_repo"`, + Deletion requires authorization with the "delete_repo" scope. + To authorize, run "gh auth refresh -s delete_repo" + `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { From 74f13a9b4f8e735a14ca227a21c31231db4fc41c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:55:35 -0700 Subject: [PATCH 03/23] Apply suggestions from code review Co-authored-by: Andy Feller --- pkg/cmd/cache/delete/delete.go | 6 +++--- pkg/cmd/repo/delete/delete.go | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 1835d87b1..65a9d696a 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -35,11 +35,11 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete [| | --all]", Short: "Delete GitHub Actions caches", - Long: heredoc.Doc(` + Long: heredoc.Docf(` Delete GitHub Actions caches. - Deletion requires authorization with the "repo" scope. - `), + Deletion requires authorization with the %[1]srepo%[1]s scope. + `, "`"), Example: heredoc.Doc(` # Delete a cache by id $ gh cache delete 1234 diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index e6fbf6363..7c6476f1d 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -39,14 +39,14 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete []", Short: "Delete a repository", - Long: heredoc.Doc(` + Long: heredoc.Docf(` Delete a GitHub repository. With no argument, deletes the current repository. Otherwise, deletes the specified repository. - Deletion requires authorization with the "delete_repo" scope. - To authorize, run "gh auth refresh -s delete_repo" - `), + Deletion requires authorization with the %[1]sdelete_repo%[1]s scope. + To authorize, run %[1]sgh auth refresh -s delete_repo%[1]s + `, "`"), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { From a4f96d29e32214c13a0f5c5830a39a714bea94ba Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:52:15 -0700 Subject: [PATCH 04/23] Refactor `workflow` scope checking Refactor the logic for checking `workflow` scope checking in releases to be in the positive - check if the scope is there, not check if it isn't there. Then, when the function is called we invert it. Also update comments to be more imperative. This refactor also incorporates @andyfeller's suggestion to use `slices`. Co-Authored-By: Andy Feller --- pkg/cmd/release/create/http.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 1512b5341..1162e7080 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/url" + "slices" "strings" "github.com/MakeNowJust/heredoc" @@ -178,17 +179,18 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st } defer resp.Body.Close() - // This code checks if we received a 404 while attempting to create a release without - // the workflow scope, and if so, returns an error message that explains a possible + // Check if we received a 404 while attempting to create a release without + // the workflow scope, and if so, return an error message that explains a possible // solution to the user. // // If the same file (with both the same path and contents) exists // on another branch in the repo, releases with workflow file changes can be // created without the workflow scope. Otherwise, the workflow scope is - // required to create the release. + // required to create the release, but the API does not indicate this criteria + // beyond returning a 404. // // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes - if resp.StatusCode == http.StatusNotFound && tokenMissingWorkflowScope(resp) { + if resp.StatusCode == http.StatusNotFound && !tokenHasWorkflowScope(resp) { normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` HTTP 404: Failed to create release, "workflow" scope may be required @@ -279,23 +281,17 @@ func deleteRelease(httpClient *http.Client, release *shared.Release) error { return nil } -// Given an http.Response, check if the token used in -// the request is missing the workflow scope. -func tokenMissingWorkflowScope(resp *http.Response) bool { +// tokenHasWorkflowScope checks if the given http.Response's token has the workflow scope. +// Tokens that do not have OAuth scopes are assumed to have the workflow scope. +func tokenHasWorkflowScope(resp *http.Response) bool { scopes := resp.Header.Get("X-Oauth-Scopes") - // Return false when no scopes are present - no scopes in this header + // Return true when no scopes are present - no scopes in this header // means that the user is probably authenticating with a token type other // than an OAuth token, and we don't know what this token's scopes actually are. if scopes == "" { - return false + return true } - for _, s := range strings.Split(scopes, ",") { - if s == "workflow" { - return false - } - } - - return true + return slices.Contains(strings.Split(scopes, ","), "workflow") } From 11dc6df88b93e5e767c4f82ca3245693afddc86a Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Sat, 23 Nov 2024 13:19:45 -0700 Subject: [PATCH 05/23] ScopesResponder wraps StatusScopesResponder --- pkg/httpmock/stub.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index d1a27d835..196a047d8 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -225,17 +225,9 @@ func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responde } } +// ScopesResponder returns a response with a 200 status code and the given OAuth scopes. func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) { - return func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: 200, - Request: req, - Header: map[string][]string{ - "X-Oauth-Scopes": {scopes}, - }, - Body: io.NopCloser(bytes.NewBufferString("")), - }, nil - } + return StatusScopesResponder(http.StatusOK, scopes) } // StatusScopesResponder returns a response with the given status code and OAuth scopes. From deb34d6456d14efe73ccd61fcb88e063da5f4794 Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Sat, 23 Nov 2024 16:59:49 -0700 Subject: [PATCH 06/23] Refactor error handling for missing "workflow" scope in createRelease --- pkg/cmd/release/create/create.go | 15 +++++++++++++++ pkg/cmd/release/create/create_test.go | 9 +++++---- pkg/cmd/release/create/http.go | 18 +++++++++++------- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 0b7c99b23..8e77546d0 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -477,6 +477,21 @@ func createRun(opts *CreateOptions) error { } newRelease, err := createRelease(httpClient, baseRepo, params) + + var errMissingRequiredWorkflowScope *errMissingRequiredWorkflowScope + if errors.As(err, &errMissingRequiredWorkflowScope) { + host := errMissingRequiredWorkflowScope.Hostname + refreshInstructions := fmt.Sprintf("gh auth refresh -h %[1]s -s workflow", host) + cs := opts.IO.ColorScheme() + + var sb strings.Builder + sb.WriteString(fmt.Sprintf("%s Failed to create release, \"workflow\" scope may be required.\n", cs.WarningIcon())) + sb.WriteString(fmt.Sprintf("To request it, run:\n%s\n", cs.Bold(refreshInstructions))) + fmt.Fprint(opts.IO.ErrOut, sb.String()) + + return cmdutil.SilentError + } + if err != nil { return err } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index aded1bea9..c9e9a8c8a 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -1102,10 +1102,12 @@ func Test_createRun(t *testing.T) { httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusScopesResponder(404, `repo,read:org`)) }, - wantErr: heredoc.Doc(` - HTTP 404: Failed to create release, "workflow" scope may be required - To request it, run gh auth refresh -h github.com -s workflow + wantStderr: heredoc.Doc(` + ! Failed to create release, "workflow" scope may be required. + To request it, run: + gh auth refresh -h github.com -s workflow `), + wantErr: cmdutil.SilentError.Error(), }, { name: "API returns 404, OAuth token has workflow scope", @@ -1182,7 +1184,6 @@ func Test_createRun(t *testing.T) { err := createRun(&tt.opts) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) - return } else { require.NoError(t, err) } diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 1162e7080..3bb55f39e 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -11,7 +11,6 @@ import ( "slices" "strings" - "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" @@ -32,6 +31,14 @@ type releaseNotes struct { var notImplementedError = errors.New("not implemented") +type errMissingRequiredWorkflowScope struct { + Hostname string +} + +func (e errMissingRequiredWorkflowScope) Error() string { + return "workflow scope may be required" +} + func remoteTagExists(httpClient *http.Client, repo ghrepo.Interface, tagName string) (bool, error) { gql := api.NewClientFromHTTP(httpClient) qualifiedTagName := fmt.Sprintf("refs/tags/%s", tagName) @@ -192,12 +199,9 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes if resp.StatusCode == http.StatusNotFound && !tokenHasWorkflowScope(resp) { normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) - errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` - HTTP 404: Failed to create release, "workflow" scope may be required - To request it, run gh auth refresh -h %[1]s -s workflow - `, normalizedHostname)) - - return nil, errMissingRequiredWorkflowScope + return nil, &errMissingRequiredWorkflowScope{ + Hostname: normalizedHostname, + } } success := resp.StatusCode >= 200 && resp.StatusCode < 300 From 46922694dcd0754a9852c26468026e69659d1d66 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 26 Nov 2024 13:31:34 +0100 Subject: [PATCH 07/23] 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 08/23] 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 09/23] 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 10/23] 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 11/23] 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 12/23] 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 13/23] 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 14/23] 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 15/23] 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 16/23] 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 17/23] 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 18/23] 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 19/23] 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 20/23] 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 21/23] 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 22/23] 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 23/23] 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=