From 3ea71e6eb68a6385522a4f6c1b0c2a54333a2353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Jun 2020 19:51:26 +0200 Subject: [PATCH 01/27] Add ability to parse non-GitHub.com git remotes --- api/queries_repo.go | 6 ++++ command/pr_create.go | 3 +- command/pr_create_test.go | 16 ++++------ context/blank_context.go | 3 +- context/remote.go | 15 ++++++---- context/remote_test.go | 18 +++++------ internal/ghrepo/repo.go | 40 ++++++++++++++++++------- internal/ghrepo/repo_test.go | 58 +++++++++++++++++++++++++++++++++--- 8 files changed, 114 insertions(+), 45 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 22d42cfbb..d0ff60aa9 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -51,6 +51,12 @@ func (r Repository) RepoName() string { return r.Name } +// RepoHost is the GitHub hostname of the repository +func (r Repository) RepoHost() string { + // FIXME: inherit hostname from the server + return "github.com" +} + // IsFork is true when this repository has a parent repository func (r Repository) IsFork() bool { return r.Parent != nil diff --git a/command/pr_create.go b/command/pr_create.go index b2dd48697..d46916f5e 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -304,8 +304,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } headRemote = &context.Remote{ Remote: gitRemote, - Owner: headRepo.RepoOwner(), - Repo: headRepo.RepoName(), + Repo: headRepo, } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 2eb0872c7..ff4f3bf1e 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" ) @@ -759,13 +760,11 @@ func Test_determineTrackingBranch_noMatch(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, - Owner: "octocat", - Repo: "Spoon-Knife", + Repo: ghrepo.New("octocat", "Spoon-Knife"), }, } @@ -786,13 +785,11 @@ func Test_determineTrackingBranch_hasMatch(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, - Owner: "octocat", - Repo: "Spoon-Knife", + Repo: ghrepo.New("octocat", "Spoon-Knife"), }, } @@ -819,8 +816,7 @@ func Test_determineTrackingBranch_respectTrackingConfig(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, } diff --git a/context/blank_context.go b/context/blank_context.go index 3ea657abe..151e5a9ed 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -62,8 +62,7 @@ func (c *blankContext) SetRemotes(stubs map[string]string) { ownerWithName := strings.SplitN(repo, "/", 2) c.remotes = append(c.remotes, &Remote{ Remote: &git.Remote{Name: remoteName}, - Owner: ownerWithName[0], - Repo: ownerWithName[1], + Repo: ghrepo.New(ownerWithName[0], ownerWithName[1]), }) } } diff --git a/context/remote.go b/context/remote.go index b40533a54..79ba0e8c8 100644 --- a/context/remote.go +++ b/context/remote.go @@ -57,18 +57,22 @@ func (r Remotes) Less(i, j int) bool { // Remote represents a git remote mapped to a GitHub repository type Remote struct { *git.Remote - Owner string - Repo string + Repo ghrepo.Interface } // RepoName is the name of the GitHub repository func (r Remote) RepoName() string { - return r.Repo + return r.Repo.RepoName() } // RepoOwner is the name of the GitHub account that owns the repo func (r Remote) RepoOwner() string { - return r.Owner + return r.Repo.RepoOwner() +} + +// RepoHost is the GitHub hostname that the remote points to +func (r Remote) RepoHost() string { + return r.Repo.RepoHost() } // TODO: accept an interface instead of git.RemoteSet @@ -86,8 +90,7 @@ func translateRemotes(gitRemotes git.RemoteSet, urlTranslate func(*url.URL) *url } remotes = append(remotes, &Remote{ Remote: r, - Owner: repo.RepoOwner(), - Repo: repo.RepoName(), + Repo: repo, }) } return diff --git a/context/remote_test.go b/context/remote_test.go index 1e0f47ba0..6d5801e26 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -22,9 +22,9 @@ func eq(t *testing.T, got interface{}, expected interface{}) { func Test_Remotes_FindByName(t *testing.T) { list := Remotes{ - &Remote{Remote: &git.Remote{Name: "mona"}, Owner: "monalisa", Repo: "myfork"}, - &Remote{Remote: &git.Remote{Name: "origin"}, Owner: "monalisa", Repo: "octo-cat"}, - &Remote{Remote: &git.Remote{Name: "upstream"}, Owner: "hubot", Repo: "tools"}, + &Remote{Remote: &git.Remote{Name: "mona"}, Repo: ghrepo.New("monalisa", "myfork")}, + &Remote{Remote: &git.Remote{Name: "origin"}, Repo: ghrepo.New("monalisa", "octo-cat")}, + &Remote{Remote: &git.Remote{Name: "upstream"}, Repo: ghrepo.New("hubot", "tools")}, } r, err := list.FindByName("upstream", "origin") @@ -84,13 +84,11 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, &Remote{ Remote: &git.Remote{Name: "fork"}, - Owner: "MYSELF", - Repo: "REPO", + Repo: ghrepo.New("MYSELF", "REPO"), }, }, Network: api.RepoNetworkResult{ @@ -157,8 +155,7 @@ func Test_resolvedRemotes_forkLookup(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, }, Network: api.RepoNetworkResult{ @@ -190,8 +187,7 @@ func Test_resolvedRemotes_clonedFork(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, }, Network: api.RepoNetworkResult{ diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 37fecbe8d..93493a3da 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -6,13 +6,13 @@ import ( "strings" ) -// TODO these are sprinkled across command, context, config, and ghrepo const defaultHostname = "github.com" // Interface describes an object that represents a GitHub repository type Interface interface { RepoName() string RepoOwner() string + RepoHost() string } // New instantiates a GitHub repository from owner and name arguments @@ -39,32 +39,52 @@ func FromFullName(nwo string) (Interface, error) { return &r, nil } -// FromURL extracts the GitHub repository information from a URL +// FromURL extracts the GitHub repository information from a git remote URL func FromURL(u *url.URL) (Interface, error) { - if !strings.EqualFold(u.Hostname(), defaultHostname) && !strings.EqualFold(u.Hostname(), "www."+defaultHostname) { - return nil, fmt.Errorf("unsupported hostname: %s", u.Hostname()) + if u.Hostname() == "" { + return nil, fmt.Errorf("no hostname detected") } - parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 3) - if len(parts) < 2 { + + parts := strings.SplitN(strings.Trim(u.Path, "/"), "/", 3) + if len(parts) != 2 { return nil, fmt.Errorf("invalid path: %s", u.Path) } - return New(parts[0], strings.TrimSuffix(parts[1], ".git")), nil + + return &ghRepo{ + owner: parts[0], + name: strings.TrimSuffix(parts[1], ".git"), + hostname: normalizeHostname(u.Hostname()), + }, nil +} + +func normalizeHostname(h string) string { + return strings.ToLower(strings.TrimPrefix(h, "www.")) } // IsSame compares two GitHub repositories func IsSame(a, b Interface) bool { return strings.EqualFold(a.RepoOwner(), b.RepoOwner()) && - strings.EqualFold(a.RepoName(), b.RepoName()) + strings.EqualFold(a.RepoName(), b.RepoName()) && + normalizeHostname(a.RepoHost()) == normalizeHostname(b.RepoHost()) } type ghRepo struct { - owner string - name string + owner string + name string + hostname string } func (r ghRepo) RepoOwner() string { return r.owner } + func (r ghRepo) RepoName() string { return r.name } + +func (r ghRepo) RepoHost() string { + if r.hostname != "" { + return r.hostname + } + return defaultHostname +} diff --git a/internal/ghrepo/repo_test.go b/internal/ghrepo/repo_test.go index fef04fb8c..3c421bd64 100644 --- a/internal/ghrepo/repo_test.go +++ b/internal/ghrepo/repo_test.go @@ -12,31 +12,78 @@ func Test_repoFromURL(t *testing.T) { name string input string result string + host string err error }{ { name: "github.com URL", input: "https://github.com/monalisa/octo-cat.git", result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com URL with trailing slash", + input: "https://github.com/monalisa/octo-cat/", + result: "monalisa/octo-cat", + host: "github.com", err: nil, }, { name: "www.github.com URL", input: "http://www.GITHUB.com/monalisa/octo-cat.git", result: "monalisa/octo-cat", + host: "github.com", err: nil, }, { - name: "unsupported hostname", - input: "https://example.com/one/two", + name: "too many path components", + input: "https://github.com/monalisa/octo-cat/pulls", result: "", - err: errors.New("unsupported hostname: example.com"), + host: "", + err: errors.New("invalid path: /monalisa/octo-cat/pulls"), + }, + { + name: "non-GitHub hostname", + input: "https://example.com/one/two", + result: "one/two", + host: "example.com", + err: nil, }, { name: "filesystem path", input: "/path/to/file", result: "", - err: errors.New("unsupported hostname: "), + host: "", + err: errors.New("no hostname detected"), + }, + { + name: "filesystem path with scheme", + input: "file:///path/to/file", + result: "", + host: "", + err: errors.New("no hostname detected"), + }, + { + name: "github.com SSH URL", + input: "ssh://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com HTTPS+SSH URL", + input: "https+ssh://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com git URL", + input: "git://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, }, } @@ -61,6 +108,9 @@ func Test_repoFromURL(t *testing.T) { if tt.result != got { t.Errorf("expected %q, got %q", tt.result, got) } + if tt.host != repo.RepoHost() { + t.Errorf("expected %q, got %q", tt.host, repo.RepoHost()) + } }) } } From e4448c73e4cabe601c904a467c5520ed82065e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Jun 2020 20:06:50 +0200 Subject: [PATCH 02/27] Respect original hostname in `formatRemoteURL()` --- command/pr_checkout.go | 6 ++++-- command/pr_create.go | 2 +- command/repo.go | 14 +++++++++----- command/root.go | 11 +++++------ command/root_test.go | 6 ++++-- 5 files changed, 23 insertions(+), 16 deletions(-) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 8ffa39210..ec60d3de0 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -33,7 +33,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) // baseRemoteSpec is a repository URL or a remote name to be used in git fetch - baseURLOrName := formatRemoteURL(cmd, ghrepo.FullName(baseRepo)) + baseURLOrName := formatRemoteURL(cmd, baseRepo) if baseRemote != nil { baseURLOrName = baseRemote.Name } @@ -84,7 +84,9 @@ func prCheckout(cmd *cobra.Command, args []string) error { remote := baseURLOrName mergeRef := ref if pr.MaintainerCanModify { - remote = formatRemoteURL(cmd, fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name)) + // FIXME: inherit hostname from the server + headRepo := ghrepo.New(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) + remote = formatRemoteURL(cmd, headRepo) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } if mc, err := git.Config(fmt.Sprintf("branch.%s.merge", newBranchName)); err != nil || mc == "" { diff --git a/command/pr_create.go b/command/pr_create.go index d46916f5e..11d9ec523 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -295,7 +295,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { // In either case, we want to add the head repo as a new git remote so we // can push to it. if headRemote == nil { - headRepoURL := formatRemoteURL(cmd, ghrepo.FullName(headRepo)) + headRepoURL := formatRemoteURL(cmd, headRepo) // TODO: prevent clashes with another remote of a same name gitRemote, err := git.AddRemote("fork", headRepoURL) diff --git a/command/repo.go b/command/repo.go index 7c15945aa..83788fe78 100644 --- a/command/repo.go +++ b/command/repo.go @@ -186,7 +186,11 @@ func repoClone(cmd *cobra.Command, args []string) error { } cloneURL = currentUser + "/" + cloneURL } - cloneURL = formatRemoteURL(cmd, cloneURL) + repo, err := ghrepo.FromFullName(cloneURL) + if err != nil { + return err + } + cloneURL = formatRemoteURL(cmd, repo) } var repo ghrepo.Interface @@ -221,7 +225,7 @@ func repoClone(cmd *cobra.Command, args []string) error { } func addUpstreamRemote(cmd *cobra.Command, parentRepo ghrepo.Interface, cloneDir string) error { - upstreamURL := formatRemoteURL(cmd, ghrepo.FullName(parentRepo)) + upstreamURL := formatRemoteURL(cmd, parentRepo) cloneCmd := git.GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL) cloneCmd.Stdout = os.Stdout @@ -322,7 +326,7 @@ func repoCreate(cmd *cobra.Command, args []string) error { fmt.Fprintln(out, repo.URL) } - remoteURL := formatRemoteURL(cmd, ghrepo.FullName(repo)) + remoteURL := formatRemoteURL(cmd, repo) if projectDirErr == nil { _, err = git.AddRemote("origin", remoteURL) @@ -497,7 +501,7 @@ func repoFork(cmd *cobra.Command, args []string) error { fmt.Fprintf(out, "%s Renamed %s remote to %s\n", greenCheck, utils.Bold(remoteName), utils.Bold(renameTarget)) } - forkedRepoCloneURL := formatRemoteURL(cmd, ghrepo.FullName(forkedRepo)) + forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) _, err = git.AddRemote(remoteName, forkedRepoCloneURL) if err != nil { @@ -515,7 +519,7 @@ func repoFork(cmd *cobra.Command, args []string) error { } } if cloneDesired { - forkedRepoCloneURL := formatRemoteURL(cmd, ghrepo.FullName(forkedRepo)) + forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) cloneDir, err := runClone(forkedRepoCloneURL, []string{}) if err != nil { return fmt.Errorf("failed to clone fork: %w", err) diff --git a/command/root.go b/command/root.go index 693603e05..3d53c3a71 100644 --- a/command/root.go +++ b/command/root.go @@ -336,23 +336,22 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } -func formatRemoteURL(cmd *cobra.Command, fullRepoName string) string { +func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { ctx := contextForCommand(cmd) - protocol := "https" + var protocol string cfg, err := ctx.Config() if err != nil { fmt.Fprintf(colorableErr(cmd), "%s failed to load config: %s. using defaults\n", utils.Yellow("!"), err) } else { - cfgProtocol, _ := cfg.Get(defaultHostname, "git_protocol") - protocol = cfgProtocol + protocol, _ = cfg.Get(repo.RepoHost(), "git_protocol") } if protocol == "ssh" { - return fmt.Sprintf("git@%s:%s.git", defaultHostname, fullRepoName) + return fmt.Sprintf("git@%s:%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } - return fmt.Sprintf("https://%s/%s.git", defaultHostname, fullRepoName) + return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } func determineEditor(cmd *cobra.Command) (string, error) { diff --git a/command/root_test.go b/command/root_test.go index dbed9628d..7ca938d6c 100644 --- a/command/root_test.go +++ b/command/root_test.go @@ -2,6 +2,8 @@ package command import ( "testing" + + "github.com/cli/cli/internal/ghrepo" ) func TestChangelogURL(t *testing.T) { @@ -43,7 +45,7 @@ func TestChangelogURL(t *testing.T) { func TestRemoteURLFormatting_no_config(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - result := formatRemoteURL(repoForkCmd, "OWNER/REPO") + result := formatRemoteURL(repoForkCmd, ghrepo.New("OWNER", "REPO")) eq(t, result, "https://github.com/OWNER/REPO.git") } @@ -56,6 +58,6 @@ hosts: git_protocol: ssh ` initBlankContext(cfg, "OWNER/REPO", "master") - result := formatRemoteURL(repoForkCmd, "OWNER/REPO") + result := formatRemoteURL(repoForkCmd, ghrepo.New("OWNER", "REPO")) eq(t, result, "git@github.com:OWNER/REPO.git") } From 446a4111f768e8c0b4718e35a6a6dd273018fc98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 2 Jul 2020 20:36:10 +0200 Subject: [PATCH 03/27] Respect hostnames when resolving git remotes and URL args to repos --- api/queries_repo.go | 31 +++++++++++++++----- command/issue.go | 48 ++++--------------------------- command/issue_lookup.go | 64 +++++++++++++++++++++++++++++++++++++++++ command/issue_test.go | 18 ++++-------- command/pr_checkout.go | 3 +- command/pr_lookup.go | 26 ++++++++++++----- context/context.go | 22 ++++++++++---- internal/ghrepo/repo.go | 9 ++++++ 8 files changed, 143 insertions(+), 78 deletions(-) create mode 100644 command/issue_lookup.go diff --git a/api/queries_repo.go b/api/queries_repo.go index d0ff60aa9..ba3256f9c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -34,6 +34,9 @@ type Repository struct { } Parent *Repository + + // pseudo-field that keeps track of host name of this repo + hostname string } // RepositoryOwner is the owner of a GitHub repository @@ -53,8 +56,7 @@ func (r Repository) RepoName() string { // RepoHost is the GitHub hostname of the repository func (r Repository) RepoHost() string { - // FIXME: inherit hostname from the server - return "github.com" + return r.hostname } // IsFork is true when this repository has a parent repository @@ -109,7 +111,7 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { return nil, err } - return &result.Repository, nil + return initRepoHostname(&result.Repository, repo.RepoHost()), nil } // RepoParent finds out the parent repository of a fork @@ -139,7 +141,7 @@ func RepoParent(client *Client, repo ghrepo.Interface) (ghrepo.Interface, error) return nil, nil } - parent := ghrepo.New(query.Repository.Parent.Owner.Login, query.Repository.Parent.Name) + parent := ghrepo.NewWithHost(query.Repository.Parent.Owner.Login, query.Repository.Parent.Name, repo.RepoHost()) return parent, nil } @@ -151,6 +153,11 @@ type RepoNetworkResult struct { // RepoNetwork inspects the relationship between multiple GitHub repositories func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, error) { + var hostname string + if len(repos) > 0 { + hostname = repos[0].RepoHost() + } + queries := make([]string, 0, len(repos)) for i, repo := range repos { queries = append(queries, fmt.Sprintf(` @@ -233,7 +240,7 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e if err := decoder.Decode(&repo); err != nil { return result, err } - result.Repositories = append(result.Repositories, &repo) + result.Repositories = append(result.Repositories, initRepoHostname(&repo, hostname)) } else { return result, fmt.Errorf("unknown GraphQL result key %q", name) } @@ -241,6 +248,14 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e return result, nil } +func initRepoHostname(repo *Repository, hostname string) *Repository { + repo.hostname = hostname + if repo.Parent != nil { + repo.Parent.hostname = hostname + } + return repo +} + // repositoryV3 is the repository result from GitHub API v3 type repositoryV3 struct { NodeID string @@ -271,6 +286,7 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { Login: result.Owner.Login, }, ViewerPermission: "WRITE", + hostname: repo.RepoHost(), }, nil } @@ -312,7 +328,7 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { // `affiliations` condition, to guard against versions of GitHub with a // faulty `affiliations` implementation if len(forks) > 0 && forks[0].ViewerCanPush() { - return &forks[0], nil + return initRepoHostname(&forks[0], repo.RepoHost()), nil } return nil, &NotFoundError{errors.New("no fork found")} } @@ -374,7 +390,8 @@ func RepoCreate(client *Client, input RepoCreateInput) (*Repository, error) { return nil, err } - return &response.CreateRepository.Repository, nil + // FIXME: support Enterprise hosts + return initRepoHostname(&response.CreateRepository.Repository, "github.com"), nil } func RepositoryReadme(client *Client, fullName string) (string, error) { diff --git a/command/issue.go b/command/issue.go index 814daf3ae..f790b3d05 100644 --- a/command/issue.go +++ b/command/issue.go @@ -1,11 +1,9 @@ package command import ( - "errors" "fmt" "io" "net/url" - "regexp" "strconv" "strings" "time" @@ -244,12 +242,7 @@ func issueView(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return err - } - - issue, err := issueFromArg(apiClient, baseRepo, args[0]) + issue, _, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } @@ -341,21 +334,6 @@ func printIssuePreview(out io.Writer, issue *api.Issue) error { return nil } -var issueURLRE = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/issues/(\d+)`) - -func issueFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*api.Issue, error) { - if issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")); err == nil { - return api.IssueByNumber(apiClient, baseRepo, issueNumber) - } - - if m := issueURLRE.FindStringSubmatch(arg); m != nil { - issueNumber, _ := strconv.Atoi(m[3]) - return api.IssueByNumber(apiClient, baseRepo, issueNumber) - } - - return nil, fmt.Errorf("invalid issue format: %q", arg) -} - func issueCreate(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) @@ -685,19 +663,11 @@ func issueClose(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + issue, baseRepo, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } - issue, err := issueFromArg(apiClient, baseRepo, args[0]) - var idErr *api.IssuesDisabledError - if errors.As(err, &idErr) { - return fmt.Errorf("issues disabled for %s", ghrepo.FullName(baseRepo)) - } else if err != nil { - return fmt.Errorf("failed to find issue #%d: %w", issue.Number, err) - } - if issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d is already closed\n", utils.Yellow("!"), issue.Number) return nil @@ -705,7 +675,7 @@ func issueClose(cmd *cobra.Command, args []string) error { err = api.IssueClose(apiClient, baseRepo, *issue) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return err } fmt.Fprintf(colorableErr(cmd), "%s Closed issue #%d\n", utils.Red("✔"), issue.Number) @@ -720,19 +690,11 @@ func issueReopen(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + issue, baseRepo, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } - issue, err := issueFromArg(apiClient, baseRepo, args[0]) - var idErr *api.IssuesDisabledError - if errors.As(err, &idErr) { - return fmt.Errorf("issues disabled for %s", ghrepo.FullName(baseRepo)) - } else if err != nil { - return fmt.Errorf("failed to find issue #%d: %w", issue.Number, err) - } - if !issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d is already open\n", utils.Yellow("!"), issue.Number) return nil @@ -740,7 +702,7 @@ func issueReopen(cmd *cobra.Command, args []string) error { err = api.IssueReopen(apiClient, baseRepo, *issue) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return err } fmt.Fprintf(colorableErr(cmd), "%s Reopened issue #%d\n", utils.Green("✔"), issue.Number) diff --git a/command/issue_lookup.go b/command/issue_lookup.go new file mode 100644 index 000000000..aa8e0b12c --- /dev/null +++ b/command/issue_lookup.go @@ -0,0 +1,64 @@ +package command + +import ( + "fmt" + "net/url" + "regexp" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/ghrepo" + "github.com/spf13/cobra" +) + +func issueFromArg(ctx context.Context, apiClient *api.Client, cmd *cobra.Command, arg string) (*api.Issue, ghrepo.Interface, error) { + issue, baseRepo, err := issueFromURL(apiClient, arg) + if err != nil { + return nil, nil, err + } + if issue != nil { + return issue, baseRepo, nil + } + + baseRepo, err = determineBaseRepo(apiClient, cmd, ctx) + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + + issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")) + if err != nil { + return nil, nil, fmt.Errorf("invalid issue format: %q", arg) + } + + issue, err = issueFromNumber(apiClient, baseRepo, issueNumber) + return issue, baseRepo, err +} + +var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/issues/(\d+)`) + +func issueFromURL(apiClient *api.Client, s string) (*api.Issue, ghrepo.Interface, error) { + u, err := url.Parse(s) + if err != nil { + return nil, nil, nil + } + + if u.Scheme != "https" && u.Scheme != "http" { + return nil, nil, nil + } + + m := issueURLRE.FindStringSubmatch(u.Path) + if m == nil { + return nil, nil, nil + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + issueNumber, _ := strconv.Atoi(m[3]) + issue, err := issueFromNumber(apiClient, repo, issueNumber) + return issue, repo, err +} + +func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber int) (*api.Issue, error) { + return api.IssueByNumber(apiClient, repo, issueNumber) +} diff --git a/command/issue_test.go b/command/issue_test.go index 418de25d3..105ff8997 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -387,6 +387,7 @@ func TestIssueView_Preview(t *testing.T) { func TestIssueView_web_notFound(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "errors": [ @@ -432,7 +433,6 @@ func TestIssueView_disabledIssues(t *testing.T) { func TestIssueView_web_urlArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "hasIssuesEnabled": true, "issue": { @@ -857,12 +857,8 @@ func TestIssueClose_issuesDisabled(t *testing.T) { `)) _, err := RunCommand("issue close 13") - if err == nil { - t.Fatalf("expected error when issues are disabled") - } - - if !strings.Contains(err.Error(), "issues disabled") { - t.Fatalf("got unexpected error: %s", err) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Fatalf("got error: %v", err) } } @@ -930,11 +926,7 @@ func TestIssueReopen_issuesDisabled(t *testing.T) { `)) _, err := RunCommand("issue reopen 2") - if err == nil { - t.Fatalf("expected error when issues are disabled") - } - - if !strings.Contains(err.Error(), "issues disabled") { - t.Fatalf("got unexpected error: %s", err) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Fatalf("got error: %v", err) } } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index ec60d3de0..c81bf4f76 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -84,8 +84,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { remote := baseURLOrName mergeRef := ref if pr.MaintainerCanModify { - // FIXME: inherit hostname from the server - headRepo := ghrepo.New(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) + headRepo := ghrepo.NewWithHost(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name, baseRepo.RepoHost()) remote = formatRemoteURL(cmd, headRepo) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } diff --git a/command/pr_lookup.go b/command/pr_lookup.go index a1b0c9638..cf59f0717 100644 --- a/command/pr_lookup.go +++ b/command/pr_lookup.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "net/url" "regexp" "strconv" "strings" @@ -55,16 +56,27 @@ func prFromNumberString(ctx context.Context, apiClient *api.Client, repo ghrepo. return nil, nil } +var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) + func prFromURL(ctx context.Context, apiClient *api.Client, s string) (*api.PullRequest, ghrepo.Interface, error) { - r := regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) - if m := r.FindStringSubmatch(s); m != nil { - repo := ghrepo.New(m[1], m[2]) - prNumberString := m[3] - pr, err := prFromNumberString(ctx, apiClient, repo, prNumberString) - return pr, repo, err + u, err := url.Parse(s) + if err != nil { + return nil, nil, nil } - return nil, nil, nil + if u.Scheme != "https" && u.Scheme != "http" { + return nil, nil, nil + } + + m := pullURLRE.FindStringSubmatch(u.Path) + if m == nil { + return nil, nil, nil + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + prNumberString := m[3] + pr, err := prFromNumberString(ctx, apiClient, repo, prNumberString) + return pr, repo, err } func prForCurrentBranch(ctx context.Context, apiClient *api.Client, repo ghrepo.Interface) (*api.PullRequest, error) { diff --git a/context/context.go b/context/context.go index 236f9e722..74253e1d2 100644 --- a/context/context.go +++ b/context/context.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "sort" + "strings" "github.com/cli/cli/api" "github.com/cli/cli/git" @@ -31,22 +32,31 @@ type Context interface { // unusually large number of git remotes const maxRemotesForLookup = 5 +// ResolveRemotesToRepos takes in a list of git remotes and fetches more information about the repositories they map to. +// Only the git remotes belonging to the same hostname are ever looked up; all others are ignored. func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (ResolvedRemotes, error) { sort.Stable(remotes) - lenRemotesForLookup := len(remotes) - if lenRemotesForLookup > maxRemotesForLookup { - lenRemotesForLookup = maxRemotesForLookup - } hasBaseOverride := base != "" baseOverride, _ := ghrepo.FromFullName(base) foundBaseOverride := false - repos := make([]ghrepo.Interface, 0, lenRemotesForLookup) - for _, r := range remotes[:lenRemotesForLookup] { + + var hostname string + var repos []ghrepo.Interface + for i, r := range remotes { + if i == 0 { + hostname = r.RepoHost() + } else if !strings.EqualFold(r.RepoHost(), hostname) { + // ignore all remotes for a hostname different to that of the 1st remote + continue + } repos = append(repos, r) if ghrepo.IsSame(r, baseOverride) { foundBaseOverride = true } + if len(repos) == maxRemotesForLookup { + break + } } if hasBaseOverride && !foundBaseOverride { // additionally, look up the explicitly specified base repo if it's not diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 93493a3da..fbe748204 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -23,6 +23,15 @@ func New(owner, repo string) Interface { } } +// NewWithHost is like New with an explicit host name +func NewWithHost(owner, repo, hostname string) Interface { + return &ghRepo{ + owner: owner, + name: repo, + hostname: hostname, + } +} + // FullName serializes a GitHub repository into an "OWNER/REPO" string func FullName(r Interface) string { return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName()) From a8c37a1f5c3af8a4fd9c854b83075d58e59f085b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 2 Jul 2020 20:49:45 +0200 Subject: [PATCH 04/27] Fix hostname being respected in web browse commands --- command/issue.go | 13 ++++++++++--- command/pr_create.go | 7 +------ command/repo.go | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/command/issue.go b/command/issue.go index 15c74f82a..4cd1f6d8d 100644 --- a/command/issue.go +++ b/command/issue.go @@ -403,8 +403,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } if isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { - // TODO: move URL generation into GitHubRepository - openURL := fmt.Sprintf("https://github.com/%s/issues/new", ghrepo.FullName(baseRepo)) + openURL := generateRepoURL(baseRepo, "issues/new") if title != "" || body != "" { milestone := "" if len(milestoneTitles) > 0 { @@ -476,7 +475,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } if action == PreviewAction { - openURL := fmt.Sprintf("https://github.com/%s/issues/new/", ghrepo.FullName(baseRepo)) + openURL := generateRepoURL(baseRepo, "issues/new") milestone := "" if len(milestoneTitles) > 0 { milestone = milestoneTitles[0] @@ -512,6 +511,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } +func generateRepoURL(repo ghrepo.Interface, p string, args ...interface{}) string { + baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) + if p != "" { + return baseURL + "/" + fmt.Sprintf(p, args...) + } + return baseURL +} + func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *issueMetadataState) error { if !tb.HasMetadata() { return nil diff --git a/command/pr_create.go b/command/pr_create.go index 11d9ec523..450eb7a2a 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -437,12 +437,7 @@ func withPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, p } func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestone string) (string, error) { - u := fmt.Sprintf( - "https://github.com/%s/compare/%s...%s?expand=1", - ghrepo.FullName(r), - base, - head, - ) + u := generateRepoURL(r, "compare/%s...%s?expand=1", base, head) url, err := withPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestone) if err != nil { return "", err diff --git a/command/repo.go b/command/repo.go index 83788fe78..9d4a1dc6b 100644 --- a/command/repo.go +++ b/command/repo.go @@ -592,7 +592,7 @@ func repoView(cmd *cobra.Command, args []string) error { fullName := ghrepo.FullName(toView) - openURL := fmt.Sprintf("https://github.com/%s", fullName) + openURL := generateRepoURL(toView, "") if web { fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) return utils.OpenInBrowser(openURL) From 55d31303ea18f0720a772424bb34cf57ca17488b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 13 Jul 2020 14:18:28 +0200 Subject: [PATCH 05/27] Have `admin:org` scope satisfy `read:org` requirement `admin:org` is inclusive of `read:org`, so if we find the former listed in response headers, we can conclude that the token has necessary scopes instead of letting a warning notice be shown. --- api/client.go | 14 +++++++-- api/client_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/api/client.go b/api/client.go index f307b2bf5..3d2e77dc3 100644 --- a/api/client.go +++ b/api/client.go @@ -91,6 +91,11 @@ var issuedScopesWarning bool // CheckScopes checks whether an OAuth scope is present in a response func CheckScopes(wantedScope string, cb func(string) error) ClientOption { + wantedCandidates := []string{wantedScope} + if strings.HasPrefix(wantedScope, "read:") { + wantedCandidates = append(wantedCandidates, "admin:"+strings.TrimPrefix(wantedScope, "read:")) + } + return func(tr http.RoundTripper) http.RoundTripper { return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { res, err := tr.RoundTrip(req) @@ -102,10 +107,13 @@ func CheckScopes(wantedScope string, cb func(string) error) ClientOption { hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") hasWanted := false + outer: for _, s := range hasScopes { - if wantedScope == strings.TrimSpace(s) { - hasWanted = true - break + for _, w := range wantedCandidates { + if w == strings.TrimSpace(s) { + hasWanted = true + break outer + } } } diff --git a/api/client_test.go b/api/client_test.go index b7c226c8f..4a56fc277 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "io/ioutil" + "net/http" "reflect" "testing" @@ -91,5 +92,79 @@ func TestRESTError(t *testing.T) { } if httpErr.Error() != "HTTP 422: OH NO (https://api.github.com/repos/branch)" { t.Errorf("got %q", httpErr.Error()) + + } +} + +func Test_CheckScopes(t *testing.T) { + tests := []struct { + name string + wantScope string + responseApp string + responseScopes string + expectCallback bool + }{ + { + name: "missing read:org", + wantScope: "read:org", + responseApp: "APPID", + responseScopes: "repo, gist", + expectCallback: true, + }, + { + name: "has read:org", + wantScope: "read:org", + responseApp: "APPID", + responseScopes: "repo, read:org, gist", + expectCallback: false, + }, + { + name: "has admin:org", + wantScope: "read:org", + responseApp: "APPID", + responseScopes: "repo, admin:org, gist", + expectCallback: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tr := &httpmock.Registry{} + tr.Register(httpmock.MatchAny, func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Header: http.Header{ + "X-Oauth-Client-Id": []string{tt.responseApp}, + "X-Oauth-Scopes": []string{tt.responseScopes}, + }, + }, nil + }) + + callbackInvoked := false + var gotAppID string + fn := CheckScopes(tt.wantScope, func(appID string) error { + callbackInvoked = true + gotAppID = appID + return nil + }) + + rt := fn(tr) + req, err := http.NewRequest("GET", "https://api.github.com/hello", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + issuedScopesWarning = false + _, err = rt.RoundTrip(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if tt.expectCallback != callbackInvoked { + t.Fatalf("expected CheckScopes callback: %v", tt.expectCallback) + } + if tt.expectCallback && gotAppID != tt.responseApp { + t.Errorf("unexpected app ID: %q", gotAppID) + } + }) } } From 28cd3481763088354846f114790a349a8f56d9f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 13 Jul 2020 15:53:53 +0200 Subject: [PATCH 06/27] Only check OAuth scopes when `X-Oauth-Scopes` header is present --- api/client.go | 12 +++++++++--- api/client_test.go | 10 ++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/api/client.go b/api/client.go index 3d2e77dc3..89f8e75a0 100644 --- a/api/client.go +++ b/api/client.go @@ -89,6 +89,11 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption { var issuedScopesWarning bool +const ( + httpOAuthAppID = "X-Oauth-Client-Id" + httpOAuthScopes = "X-Oauth-Scopes" +) + // CheckScopes checks whether an OAuth scope is present in a response func CheckScopes(wantedScope string, cb func(string) error) ClientOption { wantedCandidates := []string{wantedScope} @@ -99,12 +104,13 @@ func CheckScopes(wantedScope string, cb func(string) error) ClientOption { return func(tr http.RoundTripper) http.RoundTripper { return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { res, err := tr.RoundTrip(req) - if err != nil || res.StatusCode > 299 || issuedScopesWarning { + _, hasHeader := res.Header[httpOAuthAppID] + if err != nil || res.StatusCode > 299 || !hasHeader || issuedScopesWarning { return res, err } - appID := res.Header.Get("X-Oauth-Client-Id") - hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") + appID := res.Header.Get(httpOAuthAppID) + hasScopes := strings.Split(res.Header.Get(httpOAuthScopes), ",") hasWanted := false outer: diff --git a/api/client_test.go b/api/client_test.go index 4a56fc277..7f692eee1 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -125,11 +125,21 @@ func Test_CheckScopes(t *testing.T) { responseScopes: "repo, admin:org, gist", expectCallback: false, }, + { + name: "no scopes in response", + wantScope: "read:org", + responseApp: "", + responseScopes: "", + expectCallback: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { tr := &httpmock.Registry{} tr.Register(httpmock.MatchAny, func(*http.Request) (*http.Response, error) { + if tt.responseScopes == "" { + return &http.Response{StatusCode: 200}, nil + } return &http.Response{ StatusCode: 200, Header: http.Header{ From 36ade42ba30322e18cf6bcf63861cb4a23dece65 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 1 Jul 2020 16:00:12 -0500 Subject: [PATCH 07/27] scriptability improvements: issue commands This commit is part of work to make gh more scriptable. It includes both some general purpose helpers towards this goal as well as improvements to the issue commands. Other commands will follow. - Adds `utils/terminal.go` for finding out about gh's execution environment - introduces `stubTerminal` for either faking being attached to a tty or not during tests - updates issue commands to behave better when not attached to a tty: - issue list doesn't print fuzzy dates - issue list doesn't print header - issue list prints state explicitly - issue create no longer hangs - issue create fails with clear error unless both -t and -b are specified - issue view prints raw issue body - issue view prints metadata in a consistent, linewise format --- command/issue.go | 48 +++++++++++-- command/issue_test.go | 154 +++++++++++++++++++++++++++++++++++++---- command/root.go | 4 ++ command/testing.go | 24 +++++++ utils/color.go | 21 ++---- utils/table_printer.go | 35 ++++------ utils/terminal.go | 44 ++++++++++++ 7 files changed, 273 insertions(+), 57 deletions(-) create mode 100644 utils/terminal.go diff --git a/command/issue.go b/command/issue.go index 67c315a9d..720d81669 100644 --- a/command/issue.go +++ b/command/issue.go @@ -184,8 +184,9 @@ func issueList(cmd *cobra.Command, args []string) error { }) title := listHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, hasFilters) - // TODO: avoid printing header if piped to a script - fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + } out := cmd.OutOrStdout() @@ -278,8 +279,11 @@ func issueView(cmd *cobra.Command, args []string) error { fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } - out := colorableOut(cmd) - return printIssuePreview(out, issue) + if connectedToTerminal(cmd) { + return printHumanIssuePreview(colorableOut(cmd), issue) + } + + return printRawIssuePreview(cmd.OutOrStdout(), issue) } func issueStateTitleWithColor(state string) string { @@ -306,7 +310,28 @@ func listHeader(repoName string, itemName string, matchCount int, totalMatchCoun return fmt.Sprintf("Showing %d of %s in %s", matchCount, utils.Pluralize(totalMatchCount, itemName), repoName) } -func printIssuePreview(out io.Writer, issue *api.Issue) error { +func printRawIssuePreview(out io.Writer, issue *api.Issue) error { + assignees := issueAssigneeList(*issue) + labels := issueLabelList(*issue) + projects := issueProjectList(*issue) + + // Print empty strings for empty values so the number of metadata lines is always consistent (ie + // so head -n8 can be relied on) + fmt.Fprintf(out, "title:\t%s\n", issue.Title) + fmt.Fprintf(out, "state:\t%s\n", issue.State) + fmt.Fprintf(out, "author:\t%s\n", issue.Author.Login) + fmt.Fprintf(out, "labels:\t%s\n", labels) + fmt.Fprintf(out, "comments:\t%d\n", issue.Comments.TotalCount) + fmt.Fprintf(out, "assignees:\t%s\n", assignees) + fmt.Fprintf(out, "projects:\t%s\n", projects) + fmt.Fprintf(out, "milestone:\t%s\n", issue.Milestone.Title) + + fmt.Fprintln(out, "--") + fmt.Fprintln(out, issue.Body) + return nil +} + +func printHumanIssuePreview(out io.Writer, issue *api.Issue) error { now := time.Now() ago := now.Sub(issue.CreatedAt) @@ -464,6 +489,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) + if interactive && !connectedToTerminal(cmd) { + return fmt.Errorf("can't run non-interactively without both --title and --body") + } + if interactive { var legacyTemplateFile *string if baseOverride == "" { @@ -625,9 +654,16 @@ func printIssues(w io.Writer, prefix string, totalCount int, issues []api.Issue) now := time.Now() ago := now.Sub(issue.UpdatedAt) table.AddField(issueNum, nil, colorFuncForState(issue.State)) + if !table.IsTTY() { + table.AddField(issue.State, nil, nil) + } table.AddField(replaceExcessiveWhitespace(issue.Title), nil, nil) table.AddField(labels, nil, utils.Gray) - table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) + if table.IsTTY() { + table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) + } else { + table.AddField(fmt.Sprintf("%d", int(ago.Minutes())), nil, nil) + } table.EndRow() } _ = table.Render() diff --git a/command/issue_test.go b/command/issue_test.go index a3ad56b3a..7ed580c90 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -18,6 +18,7 @@ import ( func TestIssueStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -107,8 +108,31 @@ func TestIssueStatus_disabledIssues(t *testing.T) { } } -func TestIssueList(t *testing.T) { +func TestIssueList_nontty(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open("../test/fixtures/issueList.json") + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand("issue list") + if err != nil { + t.Errorf("error running command `issue list`: %v", err) + } + + eq(t, output.Stderr(), "") + test.ExpectLines(t, output.String(), + `1[\t]+number won[\t]+label[\t]+\d+`, + `2[\t]+number too[\t]+label[\t]+\d+`, + `4[\t]+number fore[\t]+label[\t]+\d+`) +} + +func TestIssueList_tty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -125,21 +149,14 @@ Showing 3 of 3 issues in OWNER/REPO `) - expectedIssues := []*regexp.Regexp{ - regexp.MustCompile(`(?m)^1\t.*won`), - regexp.MustCompile(`(?m)^2\t.*too`), - regexp.MustCompile(`(?m)^4\t.*fore`), - } - - for _, r := range expectedIssues { - if !r.MatchString(output.String()) { - t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) - return - } - } + test.ExpectLines(t, output.String(), + "number won", + "number too", + "number fore") } -func TestIssueList_withFlags(t *testing.T) { +func TestIssueList_tty_withFlags(t *testing.T) { + defer stubTerminal(true)() initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -296,7 +313,92 @@ func TestIssueView_web_numberArgWithHash(t *testing.T) { eq(t, url, "https://github.com/OWNER/REPO/issues/123") } -func TestIssueView_Preview(t *testing.T) { +func TestIssueView_nontty_Preview(t *testing.T) { + defer stubTerminal(false)() + tests := map[string]struct { + ownerRepo string + command string + fixture string + expectedOutputs []string + }{ + "Open issue without metadata": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_preview.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tOPEN`, + `comments:\t9`, + `author:\tmarseilles`, + `assignees:`, + `\*\*bold story\*\*`, + }, + }, + "Open issue with metadata": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewWithMetadata.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `assignees:\tmarseilles, monaco`, + `author:\tmarseilles`, + `state:\tOPEN`, + `comments:\t9`, + `labels:\tone, two, three, four, five`, + `projects:\tProject 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\), Project 4 \(Awaiting triage\)\n`, + `milestone:\tuluru\n`, + `\*\*bold story\*\*`, + }, + }, + "Open issue with empty body": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewWithEmptyBody.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tOPEN`, + `author:\tmarseilles`, + `labels:\ttarot`, + }, + }, + "Closed issue": { + ownerRepo: "master", + command: "issue view 123", + fixture: "../test/fixtures/issueView_previewClosedState.json", + expectedOutputs: []string{ + `title:\tix of coins`, + `state:\tCLOSED`, + `\*\*bold story\*\*`, + `author:\tmarseilles`, + `labels:\ttarot`, + `\*\*bold story\*\*`, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + initBlankContext("", "OWNER/REPO", tc.ownerRepo) + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open(tc.fixture) + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand(tc.command) + if err != nil { + t.Errorf("error running command `%v`: %v", tc.command, err) + } + + eq(t, output.Stderr(), "") + + test.ExpectLines(t, output.String(), tc.expectedOutputs...) + }) + } +} + +func TestIssueView_tty_Preview(t *testing.T) { + defer stubTerminal(true)() tests := map[string]struct { ownerRepo string command string @@ -448,6 +550,28 @@ func TestIssueView_web_urlArg(t *testing.T) { eq(t, url, "https://github.com/OWNER/REPO/issues/123") } +func TestIssueCreate_nontty_error(t *testing.T) { + defer stubTerminal(false)() + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + + _, err := RunCommand(`issue create -t hello`) + if err == nil { + t.Fatal("expected error running command `issue create`") + } + + assert.Equal(t, "can't run non-interactively without both --title and --body", err.Error()) + +} + func TestIssueCreate(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() diff --git a/command/root.go b/command/root.go index 3693fd6f0..df607a50d 100644 --- a/command/root.go +++ b/command/root.go @@ -407,3 +407,7 @@ func ExpandAlias(args []string) ([]string, error) { return args[1:], nil } + +func connectedToTerminal(cmd *cobra.Command) bool { + return utils.IsTerminal(cmd.InOrStdin()) && utils.IsTerminal(cmd.OutOrStdout()) +} diff --git a/command/testing.go b/command/testing.go index af713aa29..d9599e9b3 100644 --- a/command/testing.go +++ b/command/testing.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/config" "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/spf13/pflag" ) @@ -169,3 +170,26 @@ func (s errorStub) Output() ([]byte, error) { func (s errorStub) Run() error { return errors.New(s.message) } + +func stubTerminal(connected bool) func() { + isTerminal := utils.IsTerminal + utils.IsTerminal = func(_ interface{}) bool { + return connected + } + + terminalSize := utils.TerminalSize + if connected { + utils.TerminalSize = func(_ interface{}) (int, int, error) { + return 80, 20, nil + } + } else { + utils.TerminalSize = func(_ interface{}) (int, int, error) { + return 0, 0, fmt.Errorf("terminal connection stubbed to false") + } + } + + return func() { + utils.IsTerminal = isTerminal + utils.TerminalSize = terminalSize + } +} diff --git a/utils/color.go b/utils/color.go index dd9a7d11c..43a3277f9 100644 --- a/utils/color.go +++ b/utils/color.go @@ -5,7 +5,6 @@ import ( "os" "github.com/mattn/go-colorable" - "github.com/mattn/go-isatty" "github.com/mgutz/ansi" ) @@ -23,22 +22,12 @@ var ( Bold = makeColorFunc("default+b") ) -func isStdoutTerminal() bool { - if !checkedTerminal { - _isStdoutTerminal = IsTerminal(os.Stdout) - checkedTerminal = true - } - return _isStdoutTerminal -} - -// IsTerminal reports whether the file descriptor is connected to a terminal -func IsTerminal(f *os.File) bool { - return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) -} - // NewColorable returns an output stream that handles ANSI color sequences on Windows -func NewColorable(f *os.File) io.Writer { - return colorable.NewColorable(f) +func NewColorable(w io.Writer) io.Writer { + if f, isFile := w.(*os.File); isFile { + return colorable.NewColorable(f) + } + return w } func makeColorFunc(color string) func(string) string { diff --git a/utils/table_printer.go b/utils/table_printer.go index 217c97ef6..823a4b533 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -9,8 +9,6 @@ import ( "strings" "github.com/cli/cli/pkg/text" - "github.com/mattn/go-isatty" - "golang.org/x/crypto/ssh/terminal" ) type TablePrinter interface { @@ -21,26 +19,23 @@ type TablePrinter interface { } func NewTablePrinter(w io.Writer) TablePrinter { - if outFile, isFile := w.(*os.File); isFile { - // TODO: use utils.IsTerminal() - isCygwin := isatty.IsCygwinTerminal(outFile.Fd()) - if isatty.IsTerminal(outFile.Fd()) || isCygwin { - ttyWidth := 80 - if w, _, err := terminal.GetSize(int(outFile.Fd())); err == nil { - ttyWidth = w - } else if isCygwin { - tputCmd := exec.Command("tput", "cols") - tputCmd.Stdin = os.Stdin - if out, err := tputCmd.Output(); err == nil { - if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { - ttyWidth = w - } + if IsTerminal(w) { + isCygwin := IsCygwinTerminal(w) + ttyWidth := 80 + if termWidth, _, err := TerminalSize(w); err == nil { + ttyWidth = termWidth + } else if isCygwin { + tputCmd := exec.Command("tput", "cols") + tputCmd.Stdin = os.Stdin + if out, err := tputCmd.Output(); err == nil { + if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { + ttyWidth = w } } - return &ttyTablePrinter{ - out: NewColorable(outFile), - maxWidth: ttyWidth, - } + } + return &ttyTablePrinter{ + out: NewColorable(w), + maxWidth: ttyWidth, } } return &tsvTablePrinter{ diff --git a/utils/terminal.go b/utils/terminal.go new file mode 100644 index 000000000..29d36a184 --- /dev/null +++ b/utils/terminal.go @@ -0,0 +1,44 @@ +package utils + +import ( + "fmt" + "os" + + "github.com/mattn/go-isatty" + "golang.org/x/crypto/ssh/terminal" +) + +func isStdoutTerminal() bool { + if !checkedTerminal { + _isStdoutTerminal = IsTerminal(os.Stdout) + checkedTerminal = true + } + return _isStdoutTerminal +} + +// TODO I don't like this use of interface{} but we need to accept both io.Writer and io.Reader +// interfaces. + +var IsTerminal = func(w interface{}) bool { + if f, isFile := w.(*os.File); isFile { + return isatty.IsTerminal(f.Fd()) || IsCygwinTerminal(f) + } + + return false +} + +func IsCygwinTerminal(w interface{}) bool { + if f, isFile := w.(*os.File); isFile { + return isatty.IsCygwinTerminal(f.Fd()) + } + + return false +} + +var TerminalSize = func(w interface{}) (int, int, error) { + if f, isFile := w.(*os.File); isFile { + return terminal.GetSize(int(f.Fd())) + } + + return 0, 0, fmt.Errorf("%v is not a file", w) +} From 97107661feb955f9481fa3d21423fde1ba353658 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 12:05:59 -0500 Subject: [PATCH 08/27] review feedback --- command/issue.go | 8 ++++---- command/issue_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/command/issue.go b/command/issue.go index 720d81669..6a1f6db3a 100644 --- a/command/issue.go +++ b/command/issue.go @@ -315,8 +315,8 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { labels := issueLabelList(*issue) projects := issueProjectList(*issue) - // Print empty strings for empty values so the number of metadata lines is always consistent (ie - // so head -n8 can be relied on) + // Print empty strings for empty values so the number of metadata lines is consistent when + // processing many issues with head and grep. fmt.Fprintf(out, "title:\t%s\n", issue.Title) fmt.Fprintf(out, "state:\t%s\n", issue.State) fmt.Fprintf(out, "author:\t%s\n", issue.Author.Login) @@ -490,7 +490,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if interactive && !connectedToTerminal(cmd) { - return fmt.Errorf("can't run non-interactively without both --title and --body") + return fmt.Errorf("must provide --title and --body when not attached to a terminal") } if interactive { @@ -662,7 +662,7 @@ func printIssues(w io.Writer, prefix string, totalCount int, issues []api.Issue) if table.IsTTY() { table.AddField(utils.FuzzyAgo(ago), nil, utils.Gray) } else { - table.AddField(fmt.Sprintf("%d", int(ago.Minutes())), nil, nil) + table.AddField(issue.UpdatedAt.String(), nil, nil) } table.EndRow() } diff --git a/command/issue_test.go b/command/issue_test.go index 7ed580c90..68f979171 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -568,7 +568,7 @@ func TestIssueCreate_nontty_error(t *testing.T) { t.Fatal("expected error running command `issue create`") } - assert.Equal(t, "can't run non-interactively without both --title and --body", err.Error()) + assert.Equal(t, "must provide --title and --body when not attached to a terminal", err.Error()) } From 754dcb744caa892a8566ebd6d368a6d60a2e9b09 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 12:36:54 -0500 Subject: [PATCH 09/27] catch up to trunk --- command/issue_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/command/issue_test.go b/command/issue_test.go index 68f979171..0b05c26a0 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -114,9 +114,9 @@ func TestIssueList_nontty(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - jsonFile, _ := os.Open("../test/fixtures/issueList.json") - defer jsonFile.Close() - http.StubResponse(200, jsonFile) + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.FileResponse("../test/fixtures/issueList.json")) output, err := RunCommand("issue list") if err != nil { @@ -381,9 +381,7 @@ func TestIssueView_nontty_Preview(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - jsonFile, _ := os.Open(tc.fixture) - defer jsonFile.Close() - http.StubResponse(200, jsonFile) + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) output, err := RunCommand(tc.command) if err != nil { From 3a9167cfe40cf3531d14db1840417433519dc144 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Fri, 12 Jun 2020 14:24:25 -0500 Subject: [PATCH 10/27] Implement shell aliases This command adds --shell to `gh alias set`, allowing specified aliases to be run through a shell interpreter. --- cmd/gh/main.go | 6 +++ command/alias.go | 85 +++++++++++++++++++---------------- command/alias_test.go | 101 ++++++++++++++++++++++++++++++++++-------- command/root.go | 34 +++++++++++--- 4 files changed, 162 insertions(+), 64 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index bac165995..a52d23f21 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -45,6 +45,12 @@ func main() { fmt.Fprintf(stderr, "failed to process aliases: %s\n", err) os.Exit(2) } + + if expandedArgs == nil && err == nil { + // It was an external alias; we ran it and are now done. + os.Exit(0) + } + if hasDebug { fmt.Fprintf(stderr, "%v -> %v\n", originalArgs, expandedArgs) } diff --git a/command/alias.go b/command/alias.go index 1b21fcc61..b4df2411d 100644 --- a/command/alias.go +++ b/command/alias.go @@ -16,21 +16,35 @@ func init() { aliasCmd.AddCommand(aliasSetCmd) aliasCmd.AddCommand(aliasListCmd) aliasCmd.AddCommand(aliasDeleteCmd) + + aliasSetCmd.Flags().BoolP("shell", "s", false, "Declare an alias to be passed through a shell interpreter") } var aliasCmd = &cobra.Command{ Use: "alias", - Short: "Create shortcuts for gh commands", + Short: "Create command shortcuts", + Long: heredoc.Doc(` + Aliases can be used to make shortcuts for gh commands or to compose multiple commands. + + Run "gh help alias set" to learn more. + `), } var aliasSetCmd = &cobra.Command{ Use: "set ", Short: "Create a shortcut for a gh command", - Long: `Declare a word as a command alias that will expand to the specified command. + Long: heredoc.Doc(` + Declare a word as a command alias that will expand to the specified command(s). -The expansion may specify additional arguments and flags. If the expansion -includes positional placeholders such as '$1', '$2', etc., any extra arguments -that follow the invocation of an alias will be inserted appropriately.`, + The expansion may specify additional arguments and flags. If the expansion + includes positional placeholders such as '$1', '$2', etc., any extra arguments + that follow the invocation of an alias will be inserted appropriately. + + If '--shell' is specified, the alias will be run through a shell interpreter (sh). This allows you + to compose commands with "|" or redirect output with ">". Note that extra arguments are not passed + to shell-interpreted aliases; only placeholders ("$1", "$2", etc) are supported. + + Quotes must always be used when defining a command as in the examples.`), Example: heredoc.Doc(` $ gh alias set pv 'pr view' $ gh pv -w 123 @@ -41,13 +55,12 @@ that follow the invocation of an alias will be inserted appropriately.`, $ gh alias set epicsBy 'issue list --author="$1" --label="epic"' $ gh epicsBy vilmibm #=> gh issue list --author="vilmibm" --label="epic" - `), - Args: cobra.MinimumNArgs(2), - RunE: aliasSet, - // NB: this allows a user to eschew quotes when specifying an alias expansion. We'll have to - // revisit it if we ever want to add flags to alias set but we have no current plans for that. - DisableFlagParsing: true, + $ gh alias set --shell igrep 'gh issue list --label="$1" | grep $2' + $ gh igrep epic foo + #=> gh issue list --label="epic" | grep "foo"`), + Args: cobra.ExactArgs(2), + RunE: aliasSet, } func aliasSet(cmd *cobra.Command, args []string) error { @@ -63,19 +76,26 @@ func aliasSet(cmd *cobra.Command, args []string) error { } alias := args[0] - expansion := processArgs(args[1:]) - - expansionStr := strings.Join(expansion, " ") + expansion := args[1] out := colorableOut(cmd) - fmt.Fprintf(out, "- Adding alias for %s: %s\n", utils.Bold(alias), utils.Bold(expansionStr)) + fmt.Fprintf(out, "- Adding alias for %s: %s\n", utils.Bold(alias), utils.Bold(expansion)) - if validCommand([]string{alias}) { + shell, err := cmd.Flags().GetBool("shell") + if err != nil { + return err + } + if shell && !strings.HasPrefix(expansion, "!") { + expansion = "!" + expansion + } + isExternal := strings.HasPrefix(expansion, "!") + + if validCommand(alias) { return fmt.Errorf("could not create alias: %q is already a gh command", alias) } - if !validCommand(expansion) { - return fmt.Errorf("could not create alias: %s does not correspond to a gh command", utils.Bold(expansionStr)) + if !isExternal && !validCommand(expansion) { + return fmt.Errorf("could not create alias: %s does not correspond to a gh command", expansion) } successMsg := fmt.Sprintf("%s Added alias.", utils.Green("✓")) @@ -86,11 +106,11 @@ func aliasSet(cmd *cobra.Command, args []string) error { utils.Green("✓"), utils.Bold(alias), utils.Bold(oldExpansion), - utils.Bold(expansionStr), + utils.Bold(expansion), ) } - err = aliasCfg.Add(alias, expansionStr) + err = aliasCfg.Add(alias, expansion) if err != nil { return fmt.Errorf("could not create alias: %s", err) } @@ -100,28 +120,15 @@ func aliasSet(cmd *cobra.Command, args []string) error { return nil } -func validCommand(expansion []string) bool { - cmd, _, err := RootCmd.Traverse(expansion) +func validCommand(expansion string) bool { + split, err := shlex.Split(expansion) + if err != nil { + return false + } + cmd, _, err := RootCmd.Traverse(split) return err == nil && cmd != RootCmd } -func processArgs(args []string) []string { - if len(args) == 1 { - split, _ := shlex.Split(args[0]) - return split - } - - newArgs := []string{} - for _, a := range args { - if !strings.HasPrefix(a, "-") && strings.Contains(a, " ") { - a = fmt.Sprintf("%q", a) - } - newArgs = append(newArgs, a) - } - - return newArgs -} - var aliasListCmd = &cobra.Command{ Use: "list", Short: "List your aliases", diff --git a/command/alias_test.go b/command/alias_test.go index a0d894e20..346e40c13 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -2,11 +2,13 @@ package command import ( "bytes" + "fmt" "strings" "testing" "github.com/cli/cli/internal/config" "github.com/cli/cli/test" + "github.com/stretchr/testify/assert" ) func TestAliasSet_gh_command(t *testing.T) { @@ -16,7 +18,7 @@ func TestAliasSet_gh_command(t *testing.T) { hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - _, err := RunCommand("alias set pr pr status") + _, err := RunCommand("alias set pr 'pr status'") if err == nil { t.Fatal("expected error") } @@ -35,7 +37,7 @@ editor: vim hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - output, err := RunCommand("alias set co pr checkout") + output, err := RunCommand("alias set co 'pr checkout'") if err != nil { t.Fatalf("unexpected error: %s", err) @@ -65,7 +67,7 @@ aliases: hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - output, err := RunCommand("alias set co pr checkout -Rcool/repo") + output, err := RunCommand("alias set co 'pr checkout -Rcool/repo'") if err != nil { t.Fatalf("unexpected error: %s", err) @@ -81,7 +83,7 @@ func TestAliasSet_space_args(t *testing.T) { hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - output, err := RunCommand(`alias set il issue list -l 'cool story'`) + output, err := RunCommand(`alias set il 'issue list -l "cool story"'`) if err != nil { t.Fatalf("unexpected error: %s", err) @@ -99,23 +101,17 @@ func TestAliasSet_arg_processing(t *testing.T) { ExpectedOutputLine string ExpectedConfigLine string }{ - {"alias set co pr checkout", "- Adding alias for co: pr checkout", "co: pr checkout"}, - {`alias set il "issue list"`, "- Adding alias for il: issue list", "il: issue list"}, {`alias set iz 'issue list'`, "- Adding alias for iz: issue list", "iz: issue list"}, - {`alias set iy issue list --author=\$1 --label=\$2`, - `- Adding alias for iy: issue list --author=\$1 --label=\$2`, - `iy: issue list --author=\$1 --label=\$2`}, - {`alias set ii 'issue list --author="$1" --label="$2"'`, - `- Adding alias for ii: issue list --author=\$1 --label=\$2`, - `ii: issue list --author=\$1 --label=\$2`}, + `- Adding alias for ii: issue list --author="\$1" --label="\$2"`, + `ii: issue list --author="\$1" --label="\$2"`}, - {`alias set ix issue list --author='$1' --label='$2'`, - `- Adding alias for ix: issue list --author=\$1 --label=\$2`, - `ix: issue list --author=\$1 --label=\$2`}, + {`alias set ix "issue list --author='\$1' --label='\$2'"`, + `- Adding alias for ix: issue list --author='\$1' --label='\$2'`, + `ix: issue list --author='\$1' --label='\$2'`}, } for _, c := range cases { @@ -143,7 +139,7 @@ editor: vim hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - output, err := RunCommand("alias set diff pr diff") + output, err := RunCommand("alias set diff 'pr diff'") if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -167,7 +163,7 @@ aliases: hostsBuf := bytes.Buffer{} defer config.StubWriteConfig(&mainBuf, &hostsBuf)() - output, err := RunCommand("alias set view pr view") + output, err := RunCommand("alias set view 'pr view'") if err != nil { t.Fatalf("unexpected error: %s", err) } @@ -181,6 +177,35 @@ aliases: } +func TestExpandAlias_external(t *testing.T) { + cfg := `--- +aliases: + co: pr checkout + il: issue list --author="$1" --label="$2" + ia: issue list --author="$1" --assignee="$1" + ig: '!gh issue list --label=$1 | grep' +` + initBlankContext(cfg, "OWNER/REPO", "trunk") + + cs, teardown := test.InitCmdStubber() + defer teardown() + cs.Stub("") + + _, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + assert.Equal(t, 1, len(cs.Calls)) + + fmt.Printf("DEBUG %#v\n", cs.Calls[0]) + + expected := []string{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} + + assert.Equal(t, expected, cs.Calls[0].Args) +} + func TestExpandAlias(t *testing.T) { cfg := `--- aliases: @@ -230,7 +255,7 @@ aliases: func TestAliasSet_invalid_command(t *testing.T) { initBlankContext("", "OWNER/REPO", "trunk") - _, err := RunCommand("alias set co pe checkout") + _, err := RunCommand("alias set co 'pe checkout'") if err == nil { t.Fatal("expected error") } @@ -319,3 +344,43 @@ aliases: eq(t, mainBuf.String(), expected) } + +func TestShellAlias_flag(t *testing.T) { + initBlankContext("", "OWNER/REPO", "trunk") + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + output, err := RunCommand("alias set --shell igrep 'gh issue list | grep'") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + test.ExpectLines(t, output.String(), "Adding alias for igrep") + expected := `aliases: + igrep: '!gh issue list | grep' +` + + eq(t, mainBuf.String(), expected) +} + +func TestShellAlias_bang(t *testing.T) { + initBlankContext("", "OWNER/REPO", "trunk") + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + output, err := RunCommand("alias set igrep '!gh issue list | grep'") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + test.ExpectLines(t, output.String(), "Adding alias for igrep") + expected := `aliases: + igrep: '!gh issue list | grep' +` + + eq(t, mainBuf.String(), expected) +} diff --git a/command/root.go b/command/root.go index df607a50d..fa93ca89e 100644 --- a/command/root.go +++ b/command/root.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "os" + "os/exec" "regexp" "runtime/debug" "strings" @@ -15,6 +16,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/internal/run" apiCmd "github.com/cli/cli/pkg/cmd/api" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -364,24 +366,43 @@ func determineEditor(cmd *cobra.Command) (string, error) { } func ExpandAlias(args []string) ([]string, error) { - empty := []string{} if len(args) < 2 { // the command is lacking a subcommand - return empty, nil + return []string{}, nil } ctx := initContext() cfg, err := ctx.Config() if err != nil { - return empty, err + return nil, err } aliases, err := cfg.Aliases() if err != nil { - return empty, err + return nil, err } expansion, ok := aliases.Get(args[1]) if ok { + if strings.HasPrefix(expansion, "!") { + shellArgs := []string{"-c", expansion[1:]} + if len(args[2:]) > 0 { + shellArgs = append(shellArgs, "--") + shellArgs = append(shellArgs, args[2:]...) + } + externalCmd := exec.Command("sh", shellArgs...) + externalCmd.Stderr = os.Stderr + externalCmd.Stdout = os.Stdout + externalCmd.Stdin = os.Stdin + preparedCmd := run.PrepareCmd(externalCmd) + + err := preparedCmd.Run() + if err != nil { + return nil, fmt.Errorf("failed to run external command: %w", err) + } + + return nil, nil + } + extraArgs := []string{} for i, a := range args[2:] { if !strings.Contains(expansion, "$") { @@ -392,7 +413,7 @@ func ExpandAlias(args []string) ([]string, error) { } lingeringRE := regexp.MustCompile(`\$\d`) if lingeringRE.MatchString(expansion) { - return empty, fmt.Errorf("not enough arguments for alias: %s", expansion) + return nil, fmt.Errorf("not enough arguments for alias: %s", expansion) } newArgs, err := shlex.Split(expansion) @@ -400,9 +421,8 @@ func ExpandAlias(args []string) ([]string, error) { return nil, err } - newArgs = append(newArgs, extraArgs...) + return append(newArgs, extraArgs...), nil - return newArgs, nil } return args[1:], nil From c3a5384895879e7e86c5d15a1f3bdd31c05abf77 Mon Sep 17 00:00:00 2001 From: nate smith Date: Thu, 2 Jul 2020 20:12:46 -0500 Subject: [PATCH 11/27] add experimental powershell support for shell aliases --- command/alias.go | 15 +++++-- command/alias_test.go | 94 +++++++++++++++++++++++++++++++++++++++---- command/root.go | 27 +++++++++++-- 3 files changed, 121 insertions(+), 15 deletions(-) diff --git a/command/alias.go b/command/alias.go index b4df2411d..680763bfc 100644 --- a/command/alias.go +++ b/command/alias.go @@ -40,9 +40,10 @@ var aliasSetCmd = &cobra.Command{ includes positional placeholders such as '$1', '$2', etc., any extra arguments that follow the invocation of an alias will be inserted appropriately. - If '--shell' is specified, the alias will be run through a shell interpreter (sh). This allows you - to compose commands with "|" or redirect output with ">". Note that extra arguments are not passed - to shell-interpreted aliases; only placeholders ("$1", "$2", etc) are supported. + If '--shell' is specified, the alias will be run through a shell interpreter (sh or pwsh). This allows you + to compose commands with "|" or redirect output. Note that extra arguments are not passed to + shell-interpreted aliases; only placeholders ("$1", "$2" on *nix, "$args" in powershell) are + supported. Quotes must always be used when defining a command as in the examples.`), Example: heredoc.Doc(` @@ -56,9 +57,15 @@ var aliasSetCmd = &cobra.Command{ $ gh epicsBy vilmibm #=> gh issue list --author="vilmibm" --label="epic" + # On macOS and Linux: $ gh alias set --shell igrep 'gh issue list --label="$1" | grep $2' $ gh igrep epic foo - #=> gh issue list --label="epic" | grep "foo"`), + #=> gh issue list --label="epic" | grep "foo" + + # On Windows (Powershell): + $ gh alias set --shell igrep 'gh issue list --label=$args[0] | Select-String -Pattern $args[1] + $ gh igrep epic foo + #=> gh issue list --label=epic | Select-String -Pattern foo`), Args: cobra.ExactArgs(2), RunE: aliasSet, } diff --git a/command/alias_test.go b/command/alias_test.go index 346e40c13..6f5077aa4 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -2,7 +2,7 @@ package command import ( "bytes" - "fmt" + "runtime" "strings" "testing" @@ -177,12 +177,39 @@ aliases: } -func TestExpandAlias_external(t *testing.T) { +func TestExpandAlias_shell_nix(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on windows") + } + cfg := `--- +aliases: + ig: '!gh issue list | grep cool' +` + initBlankContext(cfg, "OWNER/REPO", "trunk") + + cs, teardown := test.InitCmdStubber() + defer teardown() + cs.Stub("") + + _, err := ExpandAlias([]string{"gh", "ig"}) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + assert.Equal(t, 1, len(cs.Calls)) + + expected := []string{"sh", "-c", "gh issue list | grep cool"} + + assert.Equal(t, expected, cs.Calls[0].Args) +} + +func TestExpandAlias_shell_nix_extra_args(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skipping test on windows") + } cfg := `--- aliases: - co: pr checkout - il: issue list --author="$1" --label="$2" - ia: issue list --author="$1" --assignee="$1" ig: '!gh issue list --label=$1 | grep' ` initBlankContext(cfg, "OWNER/REPO", "trunk") @@ -199,13 +226,66 @@ aliases: assert.Equal(t, 1, len(cs.Calls)) - fmt.Printf("DEBUG %#v\n", cs.Calls[0]) - expected := []string{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} assert.Equal(t, expected, cs.Calls[0].Args) } +func TestExpandAlias_shell_windows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("skipping test on non-windows") + } + cfg := `--- +aliases: + ig: '!gh issue list | select-string -Pattern cool' +` + initBlankContext(cfg, "OWNER/REPO", "trunk") + + cs, teardown := test.InitCmdStubber() + defer teardown() + cs.Stub("") + + _, err := ExpandAlias([]string{"gh", "ig"}) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + assert.Equal(t, 1, len(cs.Calls)) + + expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list | select-string -Pattern cool } "} + + assert.Equal(t, expected, cs.Calls[0].Args) +} + +func TestExpandAlias_shell_windows_extra_args(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("skipping test on non-windows") + } + cfg := `--- +aliases: + co: pr checkout + ig: '!gh issue list --label=$args[0] | select-string -Pattern $args[1]' +` + initBlankContext(cfg, "OWNER/REPO", "trunk") + + cs, teardown := test.InitCmdStubber() + defer teardown() + cs.Stub("") + + _, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + assert.Equal(t, 1, len(cs.Calls)) + + expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list --label=$args[0] | select-string -Pattern $args[1] } -ArgumentList @('bug','foo')"} + + assert.Equal(t, expected, cs.Calls[0].Args) +} + func TestExpandAlias(t *testing.T) { cfg := `--- aliases: diff --git a/command/root.go b/command/root.go index fa93ca89e..d8e6db028 100644 --- a/command/root.go +++ b/command/root.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "regexp" + "runtime" "runtime/debug" "strings" @@ -385,11 +386,29 @@ func ExpandAlias(args []string) ([]string, error) { if ok { if strings.HasPrefix(expansion, "!") { shellArgs := []string{"-c", expansion[1:]} - if len(args[2:]) > 0 { - shellArgs = append(shellArgs, "--") - shellArgs = append(shellArgs, args[2:]...) + shellCmd := "sh" + if runtime.GOOS == "windows" { + shellCmd = "pwsh" + argList := "" + if len(args[2:]) > 0 { + argList = " -ArgumentList @(" + for i, arg := range args[2:] { + argList += fmt.Sprintf("'%s'", arg) + if i < len(args[2:])-1 { + argList += "," + } + } + argList += ")" + } + invoke := fmt.Sprintf("Invoke-Command -ScriptBlock { %s } %s", expansion[1:], argList) + shellArgs = []string{"-Command", invoke} + } else { + if len(args[2:]) > 0 { + shellArgs = append(shellArgs, "--") + shellArgs = append(shellArgs, args[2:]...) + } } - externalCmd := exec.Command("sh", shellArgs...) + externalCmd := exec.Command(shellCmd, shellArgs...) externalCmd.Stderr = os.Stderr externalCmd.Stdout = os.Stdout externalCmd.Stdin = os.Stdin From bbd756a99f9e77590ee4a5718193d50e83a550b3 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 7 Jul 2020 16:10:32 -0500 Subject: [PATCH 12/27] split shell alias execution into new function --- cmd/gh/main.go | 11 +++++-- command/alias.go | 6 ++-- command/alias_test.go | 44 +++++++++++----------------- command/root.go | 68 ++++++++++++++++++++++++++----------------- 4 files changed, 69 insertions(+), 60 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index a52d23f21..2e9614a79 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -40,14 +40,19 @@ func main() { cmd, _, err := command.RootCmd.Traverse(expandedArgs) if err != nil || cmd == command.RootCmd { originalArgs := expandedArgs - expandedArgs, err = command.ExpandAlias(os.Args) + expandedArgs, isShell, err := command.ExpandAlias(os.Args) if err != nil { fmt.Fprintf(stderr, "failed to process aliases: %s\n", err) os.Exit(2) } - if expandedArgs == nil && err == nil { - // It was an external alias; we ran it and are now done. + if isShell { + err = command.ExecuteShellAlias(expandedArgs) + if err != nil { + fmt.Fprintf(stderr, "failed to run alias %q: %s\n", expandedArgs, err) + os.Exit(3) + } + os.Exit(0) } diff --git a/command/alias.go b/command/alias.go index 680763bfc..5baceff33 100644 --- a/command/alias.go +++ b/command/alias.go @@ -41,9 +41,9 @@ var aliasSetCmd = &cobra.Command{ that follow the invocation of an alias will be inserted appropriately. If '--shell' is specified, the alias will be run through a shell interpreter (sh or pwsh). This allows you - to compose commands with "|" or redirect output. Note that extra arguments are not passed to - shell-interpreted aliases; only placeholders ("$1", "$2" on *nix, "$args" in powershell) are - supported. + to compose commands with "|" or redirect output. Note that extra arguments following the alias + will not be automatically passed to the expanded expression. To have a shell alias receive + arguments, you must explicitly accept them using "$1", "$2", etc or "$@" to accept all of them. Quotes must always be used when defining a command as in the examples.`), Example: heredoc.Doc(` diff --git a/command/alias_test.go b/command/alias_test.go index 6f5077aa4..f140a13f8 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -187,21 +187,17 @@ aliases: ` initBlankContext(cfg, "OWNER/REPO", "trunk") - cs, teardown := test.InitCmdStubber() - defer teardown() - cs.Stub("") + expanded, isShell, err := ExpandAlias([]string{"gh", "ig"}) - _, err := ExpandAlias([]string{"gh", "ig"}) + assert.True(t, isShell) if err != nil { t.Fatalf("unexpected error: %s", err) } - assert.Equal(t, 1, len(cs.Calls)) - expected := []string{"sh", "-c", "gh issue list | grep cool"} - assert.Equal(t, expected, cs.Calls[0].Args) + assert.Equal(t, expected, expanded) } func TestExpandAlias_shell_nix_extra_args(t *testing.T) { @@ -214,21 +210,17 @@ aliases: ` initBlankContext(cfg, "OWNER/REPO", "trunk") - cs, teardown := test.InitCmdStubber() - defer teardown() - cs.Stub("") + expanded, isShell, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) - _, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) + assert.True(t, isShell) if err != nil { t.Fatalf("unexpected error: %s", err) } - assert.Equal(t, 1, len(cs.Calls)) - expected := []string{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} - assert.Equal(t, expected, cs.Calls[0].Args) + assert.Equal(t, expected, expanded) } func TestExpandAlias_shell_windows(t *testing.T) { @@ -245,17 +237,17 @@ aliases: defer teardown() cs.Stub("") - _, err := ExpandAlias([]string{"gh", "ig"}) + expanded, isShell, err := ExpandAlias([]string{"gh", "ig"}) + + assert.True(t, isShell) if err != nil { t.Fatalf("unexpected error: %s", err) } - assert.Equal(t, 1, len(cs.Calls)) - expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list | select-string -Pattern cool } "} - assert.Equal(t, expected, cs.Calls[0].Args) + assert.Equal(t, expected, expanded) } func TestExpandAlias_shell_windows_extra_args(t *testing.T) { @@ -269,21 +261,17 @@ aliases: ` initBlankContext(cfg, "OWNER/REPO", "trunk") - cs, teardown := test.InitCmdStubber() - defer teardown() - cs.Stub("") + expanded, isShell, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) - _, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) + assert.True(t, isShell) if err != nil { t.Fatalf("unexpected error: %s", err) } - assert.Equal(t, 1, len(cs.Calls)) - expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list --label=$args[0] | select-string -Pattern $args[1] } -ArgumentList @('bug','foo')"} - assert.Equal(t, expected, cs.Calls[0].Args) + assert.Equal(t, expected, expanded) } func TestExpandAlias(t *testing.T) { @@ -317,7 +305,9 @@ aliases: args = strings.Split(c.Args, " ") } - out, err := ExpandAlias(args) + expanded, isShell, err := ExpandAlias(args) + + assert.False(t, isShell) if err == nil && c.Err != "" { t.Errorf("expected error %s for %s", c.Err, c.Args) @@ -329,7 +319,7 @@ aliases: continue } - eq(t, out, c.ExpectedArgs) + assert.Equal(t, c.ExpectedArgs, expanded) } } diff --git a/command/root.go b/command/root.go index d8e6db028..7d26cfb88 100644 --- a/command/root.go +++ b/command/root.go @@ -366,29 +366,50 @@ func determineEditor(cmd *cobra.Command) (string, error) { return editorCommand, nil } -func ExpandAlias(args []string) ([]string, error) { +func ExecuteShellAlias(args []string) error { + externalCmd := exec.Command(args[0], args[1:]...) + externalCmd.Stderr = os.Stderr + externalCmd.Stdout = os.Stdout + externalCmd.Stdin = os.Stdin + preparedCmd := run.PrepareCmd(externalCmd) + + err := preparedCmd.Run() + if err != nil { + return fmt.Errorf("failed to run external command: %w", err) + } + + return nil +} + +// ExpandAlias processes argv to see if it should be rewritten according to a user's aliases. The +// second return value indicates whether the alias should be executed in a new shell process instead +// of running gh itself. +func ExpandAlias(args []string) (expanded []string, isShell bool, err error) { + err = nil + isShell = false + expanded = []string{} + if len(args) < 2 { // the command is lacking a subcommand - return []string{}, nil + return } ctx := initContext() cfg, err := ctx.Config() if err != nil { - return nil, err + return } aliases, err := cfg.Aliases() if err != nil { - return nil, err + return } expansion, ok := aliases.Get(args[1]) if ok { if strings.HasPrefix(expansion, "!") { - shellArgs := []string{"-c", expansion[1:]} - shellCmd := "sh" + isShell = true + expanded = []string{"sh", "-c", expansion[1:]} if runtime.GOOS == "windows" { - shellCmd = "pwsh" argList := "" if len(args[2:]) > 0 { argList = " -ArgumentList @(" @@ -401,25 +422,15 @@ func ExpandAlias(args []string) ([]string, error) { argList += ")" } invoke := fmt.Sprintf("Invoke-Command -ScriptBlock { %s } %s", expansion[1:], argList) - shellArgs = []string{"-Command", invoke} + expanded = []string{"pwsh", "-Command", invoke} } else { if len(args[2:]) > 0 { - shellArgs = append(shellArgs, "--") - shellArgs = append(shellArgs, args[2:]...) + expanded = append(expanded, "--") + expanded = append(expanded, args[2:]...) } } - externalCmd := exec.Command(shellCmd, shellArgs...) - externalCmd.Stderr = os.Stderr - externalCmd.Stdout = os.Stdout - externalCmd.Stdin = os.Stdin - preparedCmd := run.PrepareCmd(externalCmd) - err := preparedCmd.Run() - if err != nil { - return nil, fmt.Errorf("failed to run external command: %w", err) - } - - return nil, nil + return } extraArgs := []string{} @@ -432,19 +443,22 @@ func ExpandAlias(args []string) ([]string, error) { } lingeringRE := regexp.MustCompile(`\$\d`) if lingeringRE.MatchString(expansion) { - return nil, fmt.Errorf("not enough arguments for alias: %s", expansion) + err = fmt.Errorf("not enough arguments for alias: %s", expansion) + return } - newArgs, err := shlex.Split(expansion) + var newArgs []string + newArgs, err = shlex.Split(expansion) if err != nil { - return nil, err + return } - return append(newArgs, extraArgs...), nil - + expanded = append(newArgs, extraArgs...) + return } - return args[1:], nil + expanded = args[1:] + return } func connectedToTerminal(cmd *cobra.Command) bool { From f99b54a7311677381df85270df2892584b7abb59 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 13 Jul 2020 16:03:28 -0500 Subject: [PATCH 13/27] WIP: experimental bash support for windows --- command/root.go | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/command/root.go b/command/root.go index 7d26cfb88..92a933595 100644 --- a/command/root.go +++ b/command/root.go @@ -410,19 +410,27 @@ func ExpandAlias(args []string) (expanded []string, isShell bool, err error) { isShell = true expanded = []string{"sh", "-c", expansion[1:]} if runtime.GOOS == "windows" { - argList := "" - if len(args[2:]) > 0 { - argList = " -ArgumentList @(" - for i, arg := range args[2:] { - argList += fmt.Sprintf("'%s'", arg) - if i < len(args[2:])-1 { - argList += "," - } - } - argList += ")" + //argList := "" + //if len(args[2:]) > 0 { + // argList = " -ArgumentList @(" + // for i, arg := range args[2:] { + // argList += fmt.Sprintf("'%s'", arg) + // if i < len(args[2:])-1 { + // argList += "," + // } + // } + // argList += ")" + //} + //invoke := fmt.Sprintf("Invoke-Command -ScriptBlock { %s } %s", expansion[1:], argList) + //expanded = []string{"pwsh", "-Command", invoke} + executable, err := os.Executable() + if err != nil { + return } - invoke := fmt.Sprintf("Invoke-Command -ScriptBlock { %s } %s", expansion[1:], argList) - expanded = []string{"pwsh", "-Command", invoke} + invoke := fmt.Sprintf("gh () { %s \"$@\" }; %s", executable, expansion[1:]) + + expanded = []string{"bash", "--posix", "-c", invoke, "--"} + expanded = append(expanded, args[2:]...) } else { if len(args[2:]) > 0 { expanded = append(expanded, "--") From 4bd0435c38b05941ce5dad461f7038a0f3f7da78 Mon Sep 17 00:00:00 2001 From: nate smith Date: Tue, 14 Jul 2020 15:26:15 -0500 Subject: [PATCH 14/27] successfully use sh for windows aliases --- command/alias_test.go | 8 ++++---- command/root.go | 38 +++++++++++++++----------------------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/command/alias_test.go b/command/alias_test.go index f140a13f8..753e94055 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -229,7 +229,7 @@ func TestExpandAlias_shell_windows(t *testing.T) { } cfg := `--- aliases: - ig: '!gh issue list | select-string -Pattern cool' + ig: '!gh issue list | grep cool' ` initBlankContext(cfg, "OWNER/REPO", "trunk") @@ -245,7 +245,7 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list | select-string -Pattern cool } "} + expected := []string{"C:\\Program Files\\Git\\bin\\sh.exe", "-c", "gh issue list | grep cool"} assert.Equal(t, expected, expanded) } @@ -257,7 +257,7 @@ func TestExpandAlias_shell_windows_extra_args(t *testing.T) { cfg := `--- aliases: co: pr checkout - ig: '!gh issue list --label=$args[0] | select-string -Pattern $args[1]' + ig: '!gh issue list --label=$1 | grep $2' ` initBlankContext(cfg, "OWNER/REPO", "trunk") @@ -269,7 +269,7 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"pwsh", "-Command", "Invoke-Command -ScriptBlock { gh issue list --label=$args[0] | select-string -Pattern $args[1] } -ArgumentList @('bug','foo')"} + expected := []string{"C:\\Program Files\\Git\\bin\\sh.exe", "-c", "gh issue list --label=$1 | grep $2", "--", "bug", "foo"} assert.Equal(t, expected, expanded) } diff --git a/command/root.go b/command/root.go index 92a933595..71de36589 100644 --- a/command/root.go +++ b/command/root.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "os/exec" + "path/filepath" "regexp" "runtime" "runtime/debug" @@ -410,32 +411,23 @@ func ExpandAlias(args []string) (expanded []string, isShell bool, err error) { isShell = true expanded = []string{"sh", "-c", expansion[1:]} if runtime.GOOS == "windows" { - //argList := "" - //if len(args[2:]) > 0 { - // argList = " -ArgumentList @(" - // for i, arg := range args[2:] { - // argList += fmt.Sprintf("'%s'", arg) - // if i < len(args[2:])-1 { - // argList += "," - // } - // } - // argList += ")" - //} - //invoke := fmt.Sprintf("Invoke-Command -ScriptBlock { %s } %s", expansion[1:], argList) - //expanded = []string{"pwsh", "-Command", invoke} - executable, err := os.Executable() - if err != nil { - return + // Need to use absolute path for sh on windows + shPath, lookErr := exec.LookPath("sh") + if lookErr != nil { + gitPath, lookErr := exec.LookPath("git") + if lookErr != nil { + // TODO this error could be better probably + err = fmt.Errorf("unable to find sh. you will not be able to use shell aliases on this platform.") + return + } + shPath = filepath.Join(filepath.Dir(gitPath), "..", "bin", "sh.exe") } - invoke := fmt.Sprintf("gh () { %s \"$@\" }; %s", executable, expansion[1:]) + expanded[0] = shPath + } - expanded = []string{"bash", "--posix", "-c", invoke, "--"} + if len(args[2:]) > 0 { + expanded = append(expanded, "--") expanded = append(expanded, args[2:]...) - } else { - if len(args[2:]) > 0 { - expanded = append(expanded, "--") - expanded = append(expanded, args[2:]...) - } } return From a9d93f8c57ed2d85ccefd145ab0c205ed1f8ea83 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 19:42:05 -0500 Subject: [PATCH 15/27] correct docs --- command/alias.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/command/alias.go b/command/alias.go index 5baceff33..87573fad2 100644 --- a/command/alias.go +++ b/command/alias.go @@ -41,10 +41,13 @@ var aliasSetCmd = &cobra.Command{ that follow the invocation of an alias will be inserted appropriately. If '--shell' is specified, the alias will be run through a shell interpreter (sh or pwsh). This allows you - to compose commands with "|" or redirect output. Note that extra arguments following the alias + to compose commands with "|" or redirect with ">". Note that extra arguments following the alias will not be automatically passed to the expanded expression. To have a shell alias receive arguments, you must explicitly accept them using "$1", "$2", etc or "$@" to accept all of them. + Platform note: on Windows, shell aliases are executed via "sh" as installed by Git For Windows. If + you have installed git on Windows in some other way, shell aliases may not work for you. + Quotes must always be used when defining a command as in the examples.`), Example: heredoc.Doc(` $ gh alias set pv 'pr view' @@ -57,15 +60,9 @@ var aliasSetCmd = &cobra.Command{ $ gh epicsBy vilmibm #=> gh issue list --author="vilmibm" --label="epic" - # On macOS and Linux: $ gh alias set --shell igrep 'gh issue list --label="$1" | grep $2' $ gh igrep epic foo - #=> gh issue list --label="epic" | grep "foo" - - # On Windows (Powershell): - $ gh alias set --shell igrep 'gh issue list --label=$args[0] | Select-String -Pattern $args[1] - $ gh igrep epic foo - #=> gh issue list --label=epic | Select-String -Pattern foo`), + #=> gh issue list --label="epic" | grep "foo"`), Args: cobra.ExactArgs(2), RunE: aliasSet, } From e886df594b626f9cd46ef2393fd4940c91d6cd90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 14:12:57 +0200 Subject: [PATCH 16/27] Add a note about `help-wanted` in triage docs --- docs/triage.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/triage.md b/docs/triage.md index 6f37b6a0c..d8aa83343 100644 --- a/docs/triage.md +++ b/docs/triage.md @@ -25,6 +25,10 @@ just imagine a flowchart - e.g. have already discussed not wanting to do or duplicate issue - comment acknowledging receipt - close +- do we want someone in the community to do it? + - e.g. the task is relatively straightforward, but no people on our team have the bandwidth to take it on at the moment + - ensure that the thread contains all the context necessary for someone new to pick this up + - add `help-wanted` label - do we want to do it, but not in the next year? - comment acknowledging it, but that we don't plan on working on it this year. - add `future` label From ee928a14a558dc51cc0421dd176d0be5dc73ad45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 14:13:21 +0200 Subject: [PATCH 17/27] Add more notes to our releasing docs --- docs/releasing.md | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/docs/releasing.md b/docs/releasing.md index 3fc91b0ba..442e68c3a 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -1,24 +1,37 @@ # Releasing -_First create a prerelease to verify the relase infrastructure_ +Our build system automatically compiles and attaches cross-platform binaries to any git tag named `vX.Y.Z`. The automated changelog is generated from commit messages starting with “Merge pull request …” that landed between this tag and the previous one (as determined topologically by git). -1. `git tag v1.2.3-pre` -2. `git push origin v1.2.3-pre` -3. Wait several minutes for the build to run -4. Verify the prerelease succeeded and has the correct artifacts at +Users who run official builds of `gh` on their machines will get notified about the new version within a 24 hour period. -_Next create a the production release_ +To test out the build system, publish a prerelease tag with a name such as `vX.Y.Z-pre.0` or `vX.Y.Z-rc.1`. Note that such a release will still be public, but it will be marked as a "prerelease", meaning that it will never show up as a "latest" release nor trigger upgrade notifications for users. -5. `git tag v1.2.3` -6. `git push origin v1.2.3` -7. Wait several minutes for the build to run -8. Check -9. Verify the marketing site was updated https://cli.github.com/ -10. Delete the prerelease on GitHub +## General guidelines + +* Features to be released should be reviewed and approved at least one day prior to the release. +* Feature releases should bump up the minor version number. + +## Tagging a new release + +1. `git tag v1.2.3 && git push origin v1.2.3` +2. Wait several minutes for builds to run: +3. Check +4. Scan generated release notes and optionally add a human touch by grouping items under topic sections +5. Verify the marketing site was updated: +6. (Optional) Delete any pre-releases related to this release. + +A successful build will result in changes across several repositories: +* +* +* +* + +If the build fails, there is not a clean way to re-run it. The easiest way would be to start over by deleting the partial release on GitHub and re-publishing the tag. Note that this might be disruptive to users or tooling that were already notified about an upgrade. If a functional release and its binaries are already out there, it might be better to try to manually fix up only the specific workflow tasks that failed. Use your best judgement depending on the failure type. ## Release locally for debugging A local release can be created for testing without creating anything official on the release page. +0. Make sure GoReleaser is installed: `brew install goreleaser` 1. `goreleaser --skip-validate --skip-publish --rm-dist` -2. Check and test files in `dist/` +2. Find the built products under `dist/`. From 6825944cadc3b4dd54cb91c2f5c115ad9ba86e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 14:54:30 +0200 Subject: [PATCH 18/27] Reference named queries in `pr checkout` HTTP stubs --- command/pr_checkout_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index 16b7429be..cf86d6f7c 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -1,7 +1,6 @@ package command import ( - "bytes" "encoding/json" "io/ioutil" "os/exec" @@ -10,6 +9,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" ) @@ -25,7 +25,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -76,7 +76,7 @@ func TestPRCheckout_urlArg(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -124,7 +124,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -187,7 +187,7 @@ func TestPRCheckout_branchArg(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequests": { "nodes": [ { "number": 123, "headRefName": "feature", @@ -237,7 +237,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -290,7 +290,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -343,7 +343,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -396,7 +396,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -447,7 +447,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -498,7 +498,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", From 305cd290ee7c2fe922e40db33120dd45321a31d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 15:35:42 +0200 Subject: [PATCH 19/27] Fix `pr checkout :` when it matches the default branch First, consolidate the functionality between `pr merge` and `pr checkout` that resolves the default branch name of the base repo. With an added bonus, the new approach avoids an API request when one isn't necessary. Then, ensure that checking out 3rd-party PRs will result in local branch name such as `/` when the head branch of the repository matches the default branch of the base repository. We already have had code in place to take care of this, but it only took effect in the `pr checkout `-style invocation. --- api/queries_pr.go | 3 - api/queries_repo.go | 12 ++++ command/pr.go | 22 +++---- command/pr_checkout.go | 8 ++- command/pr_checkout_test.go | 55 +++++----------- command/pr_test.go | 124 ++++++++++-------------------------- pkg/httpmock/registry.go | 2 + 7 files changed, 81 insertions(+), 145 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 999d67126..46fa014cd 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -432,9 +432,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu } headRepository { name - defaultBranchRef { - name - } } isCrossRepository isDraft diff --git a/api/queries_repo.go b/api/queries_repo.go index 049e305d8..3469ea80e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -114,6 +114,18 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { return initRepoHostname(&result.Repository, repo.RepoHost()), nil } +func RepoDefaultBranch(client *Client, repo ghrepo.Interface) (string, error) { + if r, ok := repo.(*Repository); ok && r.DefaultBranchRef.Name != "" { + return r.DefaultBranchRef.Name, nil + } + + r, err := GitHubRepo(client, repo) + if err != nil { + return "", err + } + return r.DefaultBranchRef.Name, nil +} + // RepoParent finds out the parent repository of a fork func RepoParent(client *Client, repo ghrepo.Interface) (ghrepo.Interface, error) { var query struct { diff --git a/command/pr.go b/command/pr.go index 96cbee4ae..e9427d1c4 100644 --- a/command/pr.go +++ b/command/pr.go @@ -493,23 +493,21 @@ func prMerge(cmd *cobra.Command, args []string) error { fmt.Fprintf(colorableOut(cmd), "%s %s pull request #%d (%s)\n", utils.Magenta("✔"), action, pr.Number, pr.Title) if deleteBranch { - repo, err := api.GitHubRepo(apiClient, baseRepo) - if err != nil { - return err - } - - currentBranch, err := ctx.Branch() - if err != nil { - return err - } - branchSwitchString := "" if deleteLocalBranch && !crossRepoPR { + currentBranch, err := ctx.Branch() + if err != nil { + return err + } + var branchToSwitchTo string if currentBranch == pr.HeadRefName { - branchToSwitchTo = repo.DefaultBranchRef.Name - err = git.CheckoutBranch(repo.DefaultBranchRef.Name) + branchToSwitchTo, err = api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return err + } + err = git.CheckoutBranch(branchToSwitchTo) if err != nil { return err } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index c81bf4f76..2c12cb6c3 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/cobra" + "github.com/cli/cli/api" "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" @@ -65,8 +66,13 @@ func prCheckout(cmd *cobra.Command, args []string) error { } else { // no git remote for PR head + defaultBranchName, err := api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return err + } + // avoid naming the new branch the same as the default branch - if newBranchName == pr.HeadRepository.DefaultBranchRef.Name { + if newBranchName == defaultBranchName { newBranchName = fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, newBranchName) } diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index cf86d6f7c..d7f0e3e18 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -33,10 +33,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -84,10 +81,7 @@ func TestPRCheckout_urlArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -132,15 +126,17 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "POE", - "defaultBranchRef": { - "name": "master" - } + "name": "POE" }, "isCrossRepository": false, "maintainerCanModify": false } } } } `)) + http.Register(httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` + { "data": { "repository": { + "defaultBranchRef": {"name": "master"} + } } } + `)) ranCommands := [][]string{} restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -195,10 +191,7 @@ func TestPRCheckout_branchArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false } @@ -245,10 +238,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -298,10 +288,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -351,10 +338,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -404,10 +388,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -455,10 +436,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -506,10 +484,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": true diff --git a/command/pr_test.go b/command/pr_test.go index c3403252e..0963ce2cb 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -937,28 +937,25 @@ func TestPRReopen_alreadyMerged(t *testing.T) { func TestPrMerge(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 1, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 1, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -987,6 +984,7 @@ func TestPrMerge(t *testing.T) { func TestPrMerge_withRepoFlag(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.GraphQLQuery(` @@ -1002,18 +1000,6 @@ func TestPrMerge_withRepoFlag(t *testing.T) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) - http.Register( - httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), - httpmock.StringResponse(`{}`)) cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() @@ -1035,6 +1021,7 @@ func TestPrMerge_withRepoFlag(t *testing.T) { func TestPrMerge_deleteBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1045,15 +1032,6 @@ func TestPrMerge_deleteBranch(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1078,6 +1056,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "another-branch") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1088,15 +1067,6 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1119,6 +1089,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { func TestPrMerge_noPrNumberGiven(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1129,15 +1100,6 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1166,28 +1128,25 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { func TestPrMerge_rebase(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 2, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 2, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "REBASE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1215,28 +1174,25 @@ func TestPrMerge_rebase(t *testing.T) { func TestPrMerge_squash(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 3, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 3, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1254,16 +1210,14 @@ func TestPrMerge_squash(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Squashed and merged pull request #3`) - - if !r.MatchString(output.String()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.String()) - } + expected := "✔ Squashed and merged pull request #3 (The title of the PR)\n✔ Deleted branch blueberries\n" + assert.Equal(t, expected, output.String()) } func TestPrMerge_alreadyMerged(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), @@ -1295,6 +1249,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) { func TestPRMerge_interactive(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1311,15 +1266,6 @@ func TestPRMerge_interactive(t *testing.T) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) diff --git a/pkg/httpmock/registry.go b/pkg/httpmock/registry.go index 486d79a06..134c88d48 100644 --- a/pkg/httpmock/registry.go +++ b/pkg/httpmock/registry.go @@ -21,6 +21,7 @@ func (r *Registry) Register(m Matcher, resp Responder) { type Testing interface { Errorf(string, ...interface{}) + Helper() } func (r *Registry) Verify(t Testing) { @@ -31,6 +32,7 @@ func (r *Registry) Verify(t Testing) { } } if n > 0 { + t.Helper() // NOTE: stubs offer no useful reflection, so we can't print details // about dead stubs and what they were trying to match t.Errorf("%d unmatched HTTP stubs", n) From c232c8872c5e36d4f48ac89bfafd2fdd66fc31b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 16:16:38 +0200 Subject: [PATCH 20/27] Abort `pr checkout` when PR has an invalid head branch name --- command/pr_checkout.go | 4 ++++ command/pr_checkout_test.go | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 2c12cb6c3..256a3dfe8 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + "strings" "github.com/spf13/cobra" @@ -46,6 +47,9 @@ func prCheckout(cmd *cobra.Command, args []string) error { var cmdQueue [][]string newBranchName := pr.HeadRefName + if strings.HasPrefix(newBranchName, "-") { + return fmt.Errorf("invalid branch name: %q", newBranchName) + } if headRemote != nil { // there is an existing git remote for PR head diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index d7f0e3e18..eeda8cee9 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" + "github.com/stretchr/testify/assert" ) func TestPRCheckout_sameRepo(t *testing.T) { @@ -464,6 +465,47 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { eq(t, strings.Join(ranCommands[1], " "), "git merge --ff-only FETCH_HEAD") } +func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { + ctx := context.NewBlank() + ctx.SetBranch("feature") + ctx.SetRemotes(map[string]string{ + "origin": "OWNER/REPO", + }) + initContext = func() context.Context { + return ctx + } + http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") + + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` + { "data": { "repository": { "pullRequest": { + "number": 123, + "headRefName": "-foo", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO" + }, + "isCrossRepository": true, + "maintainerCanModify": false + } } } } + `)) + + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { + t.Errorf("unexpected external invocation: %v", cmd.Args) + return &test.OutputStub{} + }) + defer restoreCmd() + + output, err := RunCommand(`pr checkout 123`) + if assert.Errorf(t, err, "expected command to fail") { + assert.Equal(t, `invalid branch name: "-foo"`, err.Error()) + } + assert.Equal(t, "", output.Stderr()) +} + func TestPRCheckout_maintainerCanModify(t *testing.T) { ctx := context.NewBlank() ctx.SetBranch("master") From dfb774cd38006b811ac8a166ff11eec3a3fb97b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 16:18:36 +0200 Subject: [PATCH 21/27] Verify HTTP stubs in `pr checkout` tests --- command/pr_checkout_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index eeda8cee9..525225802 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -24,6 +24,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -74,6 +75,7 @@ func TestPRCheckout_urlArg(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, @@ -119,6 +121,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, @@ -182,6 +185,7 @@ func TestPRCheckout_branchArg(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` @@ -229,6 +233,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -279,6 +284,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -329,6 +335,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -379,6 +386,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -427,6 +435,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -516,6 +525,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` From 07bee6a63c091ecf6359785c06bb77ace6cf797d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 18:16:16 +0200 Subject: [PATCH 22/27] Improve support for legacy issue/PR template names Now supports names such as `PULL-REQUEST-TEMPLATE` (dashes instead of underscores) and `issue_template.txt` (any file extension, including no extension), is valid. --- pkg/githubtemplate/github_template.go | 5 +++- pkg/githubtemplate/github_template_test.go | 34 ++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index e14ad04c8..cd54ae210 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -1,6 +1,7 @@ package githubtemplate import ( + "fmt" "io/ioutil" "path" "regexp" @@ -53,6 +54,8 @@ mainLoop: // FindLegacy returns the file path of the default(legacy) template func FindLegacy(rootDir string, name string) *string { + namePattern := regexp.MustCompile(fmt.Sprintf(`(?i)^%s(\.|$)`, strings.ReplaceAll(name, "_", "[_-]"))) + // https://help.github.com/en/github/building-a-strong-community/creating-a-pull-request-template-for-your-repository candidateDirs := []string{ path.Join(rootDir, ".github"), @@ -67,7 +70,7 @@ func FindLegacy(rootDir string, name string) *string { // detect a single template file for _, file := range files { - if strings.EqualFold(file.Name(), name+".md") { + if namePattern.MatchString(file.Name()) && !file.IsDir() { result := path.Join(dir, file.Name()) return &result } diff --git a/pkg/githubtemplate/github_template_test.go b/pkg/githubtemplate/github_template_test.go index ee5dfb625..d1c73fec7 100644 --- a/pkg/githubtemplate/github_template_test.go +++ b/pkg/githubtemplate/github_template_test.go @@ -160,7 +160,6 @@ func TestFindLegacy(t *testing.T) { name: "Template in root", prepare: []string{ "README.md", - "ISSUE_TEMPLATE", "issue_template.md", "issue_template.txt", "pull_request_template.md", @@ -172,6 +171,32 @@ func TestFindLegacy(t *testing.T) { }, want: path.Join(tmpdir, "issue_template.md"), }, + { + name: "No extension", + prepare: []string{ + "README.md", + "issue_template", + "docs/issue_template.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: path.Join(tmpdir, "issue_template"), + }, + { + name: "Dash instead of underscore", + prepare: []string{ + "README.md", + "issue-template.txt", + "docs/issue_template.md", + }, + args: args{ + rootDir: tmpdir, + name: "ISSUE_TEMPLATE", + }, + want: path.Join(tmpdir, "issue-template.txt"), + }, { name: "Template in .github takes precedence", prepare: []string{ @@ -224,8 +249,11 @@ func TestFindLegacy(t *testing.T) { file.Close() } - if got := FindLegacy(tt.args.rootDir, tt.args.name); *got != tt.want { - t.Errorf("Find() = %v, want %v", got, tt.want) + got := FindLegacy(tt.args.rootDir, tt.args.name) + if got == nil { + t.Errorf("FindLegacy() = nil, want %v", tt.want) + } else if *got != tt.want { + t.Errorf("FindLegacy() = %v, want %v", *got, tt.want) } }) os.RemoveAll(tmpdir) From acaaa28fd727fea03fee1a7cf86ba757314100ff Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 15 Jul 2020 11:30:20 -0500 Subject: [PATCH 23/27] helper function for finding sh --- command/alias_test.go | 4 ++-- command/root.go | 46 ++++++++++++++++++++++++++++++------------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/command/alias_test.go b/command/alias_test.go index 753e94055..ca1832c16 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -195,7 +195,7 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"sh", "-c", "gh issue list | grep cool"} + expected := []string{"/usr/bin/sh", "-c", "gh issue list | grep cool"} assert.Equal(t, expected, expanded) } @@ -218,7 +218,7 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} + expected := []string{"/usr/bin/sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} assert.Equal(t, expected, expanded) } diff --git a/command/root.go b/command/root.go index 71de36589..2b3aa6a4d 100644 --- a/command/root.go +++ b/command/root.go @@ -382,6 +382,32 @@ func ExecuteShellAlias(args []string) error { return nil } +func findSh() (string, error) { + shPath, err := exec.LookPath("sh") + if err == nil { + return shPath, nil + } + + if runtime.GOOS == "windows" { + winNotFoundErr := errors.New("unable to locate sh to execute the shell alias with. The sh.exe interpreter is typically distributed with Git for Windows.") + // We can try and find a sh executable in a Git for Windows install + gitPath, err := exec.LookPath("git") + if err != nil { + return "", winNotFoundErr + } + + shPath = filepath.Join(filepath.Dir(gitPath), "..", "bin", "sh.exe") + _, err = os.Stat(shPath) + if err != nil { + return "", winNotFoundErr + } + + return shPath, nil + } + + return "", errors.New("unable to locate sh to execute shell alias with") +} + // ExpandAlias processes argv to see if it should be rewritten according to a user's aliases. The // second return value indicates whether the alias should be executed in a new shell process instead // of running gh itself. @@ -409,22 +435,14 @@ func ExpandAlias(args []string) (expanded []string, isShell bool, err error) { if ok { if strings.HasPrefix(expansion, "!") { isShell = true - expanded = []string{"sh", "-c", expansion[1:]} - if runtime.GOOS == "windows" { - // Need to use absolute path for sh on windows - shPath, lookErr := exec.LookPath("sh") - if lookErr != nil { - gitPath, lookErr := exec.LookPath("git") - if lookErr != nil { - // TODO this error could be better probably - err = fmt.Errorf("unable to find sh. you will not be able to use shell aliases on this platform.") - return - } - shPath = filepath.Join(filepath.Dir(gitPath), "..", "bin", "sh.exe") - } - expanded[0] = shPath + shPath, shErr := findSh() + if shErr != nil { + err = shErr + return } + expanded = []string{shPath, "-c", expansion[1:]} + if len(args[2:]) > 0 { expanded = append(expanded, "--") expanded = append(expanded, args[2:]...) From cfb8eebf306f7ec7206997b8862767e6e7825c43 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 15 Jul 2020 11:39:48 -0500 Subject: [PATCH 24/27] quietly return exit code of external command --- cmd/gh/main.go | 7 ++++++- command/root.go | 7 +------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 2e9614a79..05c2f27bc 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -6,6 +6,7 @@ import ( "io" "net" "os" + "os/exec" "path" "strings" @@ -49,7 +50,11 @@ func main() { if isShell { err = command.ExecuteShellAlias(expandedArgs) if err != nil { - fmt.Fprintf(stderr, "failed to run alias %q: %s\n", expandedArgs, err) + if ee, ok := err.(*exec.ExitError); ok { + os.Exit(ee.ExitCode()) + } + + fmt.Fprintf(stderr, "failed to run external command: %s", err) os.Exit(3) } diff --git a/command/root.go b/command/root.go index 2b3aa6a4d..ddd4d9834 100644 --- a/command/root.go +++ b/command/root.go @@ -374,12 +374,7 @@ func ExecuteShellAlias(args []string) error { externalCmd.Stdin = os.Stdin preparedCmd := run.PrepareCmd(externalCmd) - err := preparedCmd.Run() - if err != nil { - return fmt.Errorf("failed to run external command: %w", err) - } - - return nil + return preparedCmd.Run() } func findSh() (string, error) { From cf3af450ebaaa324d0367294a449bd70a00e971f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 15 Jul 2020 11:40:35 -0500 Subject: [PATCH 25/27] minor docs --- command/alias.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/alias.go b/command/alias.go index 87573fad2..e5cec0b0b 100644 --- a/command/alias.go +++ b/command/alias.go @@ -40,10 +40,10 @@ var aliasSetCmd = &cobra.Command{ includes positional placeholders such as '$1', '$2', etc., any extra arguments that follow the invocation of an alias will be inserted appropriately. - If '--shell' is specified, the alias will be run through a shell interpreter (sh or pwsh). This allows you + If '--shell' is specified, the alias will be run through a shell interpreter (sh). This allows you to compose commands with "|" or redirect with ">". Note that extra arguments following the alias will not be automatically passed to the expanded expression. To have a shell alias receive - arguments, you must explicitly accept them using "$1", "$2", etc or "$@" to accept all of them. + arguments, you must explicitly accept them using "$1", "$2", etc., or "$@" to accept all of them. Platform note: on Windows, shell aliases are executed via "sh" as installed by Git For Windows. If you have installed git on Windows in some other way, shell aliases may not work for you. From 86912b31b224fcf57be7272c176f5adf7e6a8d68 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 15 Jul 2020 12:17:36 -0500 Subject: [PATCH 26/27] stub sh lookup --- command/alias_test.go | 78 +++++++++---------------------------------- command/root.go | 2 +- 2 files changed, 17 insertions(+), 63 deletions(-) diff --git a/command/alias_test.go b/command/alias_test.go index ca1832c16..871a540a4 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -2,7 +2,6 @@ package command import ( "bytes" - "runtime" "strings" "testing" @@ -11,6 +10,16 @@ import ( "github.com/stretchr/testify/assert" ) +func stubSh(value string) func() { + orig := findSh + findSh = func() (string, error) { + return value, nil + } + return func() { + findSh = orig + } +} + func TestAliasSet_gh_command(t *testing.T) { initBlankContext("", "OWNER/REPO", "trunk") @@ -177,10 +186,8 @@ aliases: } -func TestExpandAlias_shell_nix(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("skipping test on windows") - } +func TestExpandAlias_shell(t *testing.T) { + defer stubSh("sh")() cfg := `--- aliases: ig: '!gh issue list | grep cool' @@ -195,15 +202,13 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"/usr/bin/sh", "-c", "gh issue list | grep cool"} + expected := []string{"sh", "-c", "gh issue list | grep cool"} assert.Equal(t, expected, expanded) } -func TestExpandAlias_shell_nix_extra_args(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("skipping test on windows") - } +func TestExpandAlias_shell_extra_args(t *testing.T) { + defer stubSh("sh")() cfg := `--- aliases: ig: '!gh issue list --label=$1 | grep' @@ -218,58 +223,7 @@ aliases: t.Fatalf("unexpected error: %s", err) } - expected := []string{"/usr/bin/sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} - - assert.Equal(t, expected, expanded) -} - -func TestExpandAlias_shell_windows(t *testing.T) { - if runtime.GOOS != "windows" { - t.Skip("skipping test on non-windows") - } - cfg := `--- -aliases: - ig: '!gh issue list | grep cool' -` - initBlankContext(cfg, "OWNER/REPO", "trunk") - - cs, teardown := test.InitCmdStubber() - defer teardown() - cs.Stub("") - - expanded, isShell, err := ExpandAlias([]string{"gh", "ig"}) - - assert.True(t, isShell) - - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - - expected := []string{"C:\\Program Files\\Git\\bin\\sh.exe", "-c", "gh issue list | grep cool"} - - assert.Equal(t, expected, expanded) -} - -func TestExpandAlias_shell_windows_extra_args(t *testing.T) { - if runtime.GOOS != "windows" { - t.Skip("skipping test on non-windows") - } - cfg := `--- -aliases: - co: pr checkout - ig: '!gh issue list --label=$1 | grep $2' -` - initBlankContext(cfg, "OWNER/REPO", "trunk") - - expanded, isShell, err := ExpandAlias([]string{"gh", "ig", "bug", "foo"}) - - assert.True(t, isShell) - - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - - expected := []string{"C:\\Program Files\\Git\\bin\\sh.exe", "-c", "gh issue list --label=$1 | grep $2", "--", "bug", "foo"} + expected := []string{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} assert.Equal(t, expected, expanded) } diff --git a/command/root.go b/command/root.go index ddd4d9834..f71019352 100644 --- a/command/root.go +++ b/command/root.go @@ -377,7 +377,7 @@ func ExecuteShellAlias(args []string) error { return preparedCmd.Run() } -func findSh() (string, error) { +var findSh = func() (string, error) { shPath, err := exec.LookPath("sh") if err == nil { return shPath, nil From 3f5893bcb41154ff98d6b1fb9124e6e1d9e82b05 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 15 Jul 2020 12:36:45 -0500 Subject: [PATCH 27/27] fix variable shadowing --- cmd/gh/main.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 05c2f27bc..5336033fd 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -41,7 +41,8 @@ func main() { cmd, _, err := command.RootCmd.Traverse(expandedArgs) if err != nil || cmd == command.RootCmd { originalArgs := expandedArgs - expandedArgs, isShell, err := command.ExpandAlias(os.Args) + isShell := false + expandedArgs, isShell, err = command.ExpandAlias(os.Args) if err != nil { fmt.Fprintf(stderr, "failed to process aliases: %s\n", err) os.Exit(2)