diff --git a/api/client.go b/api/client.go index f307b2bf5..89f8e75a0 100644 --- a/api/client.go +++ b/api/client.go @@ -89,23 +89,37 @@ 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} + 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) - 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: 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..7f692eee1 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,89 @@ 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, + }, + { + 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{ + "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) + } + }) } } 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 f9044b5e1..3469ea80e 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 @@ -51,6 +54,11 @@ func (r Repository) RepoName() string { return r.Name } +// RepoHost is the GitHub hostname of the repository +func (r Repository) RepoHost() string { + return r.hostname +} + // IsFork is true when this repository has a parent repository func (r Repository) IsFork() bool { return r.Parent != nil @@ -103,7 +111,19 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { return nil, err } - return &result.Repository, nil + 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 @@ -133,7 +153,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 } @@ -145,6 +165,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(` @@ -227,7 +252,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) } @@ -235,6 +260,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 @@ -265,6 +298,7 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { Login: result.Owner.Login, }, ViewerPermission: "WRITE", + hostname: repo.RepoHost(), }, nil } @@ -306,7 +340,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")} } @@ -368,7 +402,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/cmd/gh/main.go b/cmd/gh/main.go index bac165995..5336033fd 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -6,6 +6,7 @@ import ( "io" "net" "os" + "os/exec" "path" "strings" @@ -40,11 +41,27 @@ func main() { cmd, _, err := command.RootCmd.Traverse(expandedArgs) if err != nil || cmd == command.RootCmd { originalArgs := expandedArgs - expandedArgs, 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) } + + if isShell { + err = command.ExecuteShellAlias(expandedArgs) + if err != nil { + if ee, ok := err.(*exec.ExitError); ok { + os.Exit(ee.ExitCode()) + } + + fmt.Fprintf(stderr, "failed to run external command: %s", err) + os.Exit(3) + } + + 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..e5cec0b0b 100644 --- a/command/alias.go +++ b/command/alias.go @@ -16,21 +16,39 @@ 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 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' $ gh pv -w 123 @@ -41,13 +59,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 +80,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 +110,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 +124,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..871a540a4 100644 --- a/command/alias_test.go +++ b/command/alias_test.go @@ -7,8 +7,19 @@ import ( "github.com/cli/cli/internal/config" "github.com/cli/cli/test" + "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") @@ -16,7 +27,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 +46,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 +76,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 +92,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 +110,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 +148,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 +172,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 +186,48 @@ aliases: } +func TestExpandAlias_shell(t *testing.T) { + defer stubSh("sh")() + cfg := `--- +aliases: + ig: '!gh issue list | grep cool' +` + initBlankContext(cfg, "OWNER/REPO", "trunk") + + expanded, isShell, err := ExpandAlias([]string{"gh", "ig"}) + + assert.True(t, isShell) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + expected := []string{"sh", "-c", "gh issue list | grep cool"} + + assert.Equal(t, expected, expanded) +} + +func TestExpandAlias_shell_extra_args(t *testing.T) { + defer stubSh("sh")() + cfg := `--- +aliases: + ig: '!gh issue list --label=$1 | grep' +` + 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{"sh", "-c", "gh issue list --label=$1 | grep", "--", "bug", "foo"} + + assert.Equal(t, expected, expanded) +} + func TestExpandAlias(t *testing.T) { cfg := `--- aliases: @@ -212,7 +259,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) @@ -224,13 +273,13 @@ aliases: continue } - eq(t, out, c.ExpectedArgs) + assert.Equal(t, c.ExpectedArgs, expanded) } } 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 +368,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/issue.go b/command/issue.go index e6acd9f1f..56388f35b 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" @@ -256,8 +254,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() @@ -330,12 +329,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 } @@ -350,8 +344,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 { @@ -378,7 +375,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 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) + 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) @@ -427,21 +445,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) @@ -497,8 +500,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 { @@ -536,6 +538,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("must provide --title and --body when not attached to a terminal") + } + if interactive { var legacyTemplateFile *string if baseOverride == "" { @@ -570,7 +576,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] @@ -606,6 +612,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 @@ -697,9 +711,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(issue.UpdatedAt.String(), nil, nil) + } table.EndRow() } _ = table.Render() @@ -771,19 +792,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 err - } - if issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d (%s) is already closed\n", utils.Yellow("!"), issue.Number, issue.Title) return nil @@ -791,7 +804,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 (%s)\n", utils.Red("✔"), issue.Number, issue.Title) @@ -806,19 +819,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 err - } - if !issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d (%s) is already open\n", utils.Yellow("!"), issue.Number, issue.Title) return nil @@ -826,7 +831,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 (%s)\n", utils.Green("✔"), issue.Number, issue.Title) 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 61383321e..50041a4ec 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -19,6 +19,7 @@ import ( func TestIssueStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -108,8 +109,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") + + http.Register( + httpmock.GraphQL(`query IssueList\b`), + httpmock.FileResponse("../test/fixtures/issueList.json")) + + 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( @@ -126,21 +150,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") @@ -326,7 +343,90 @@ 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") + + http.Register(httpmock.GraphQL(`query IssueByNumber\b`), httpmock.FileResponse(tc.fixture)) + + 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 @@ -403,6 +503,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": [ @@ -448,7 +549,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": { @@ -478,6 +578,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, "must provide --title and --body when not attached to a terminal", err.Error()) + +} + func TestIssueCreate(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() @@ -873,12 +995,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) } } @@ -946,11 +1064,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.go b/command/pr.go index a77ce41e5..67f02221a 100644 --- a/command/pr.go +++ b/command/pr.go @@ -519,23 +519,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 8ffa39210..256a3dfe8 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -5,9 +5,11 @@ import ( "fmt" "os" "os/exec" + "strings" "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" @@ -33,7 +35,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 } @@ -45,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 @@ -65,8 +70,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) } @@ -84,7 +94,8 @@ 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)) + headRepo := ghrepo.NewWithHost(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name, baseRepo.RepoHost()) + 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_checkout_test.go b/command/pr_checkout_test.go index 16b7429be..525225802 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,7 +9,9 @@ import ( "github.com/cli/cli/context" "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) { @@ -23,9 +24,10 @@ func TestPRCheckout_sameRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -33,10 +35,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -76,7 +75,8 @@ func TestPRCheckout_urlArg(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + defer http.Verify(t) + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -84,10 +84,7 @@ func TestPRCheckout_urlArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -124,7 +121,8 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + defer http.Verify(t) + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -132,15 +130,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 { @@ -185,9 +185,10 @@ func TestPRCheckout_branchArg(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -195,10 +196,7 @@ func TestPRCheckout_branchArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false } @@ -235,9 +233,10 @@ func TestPRCheckout_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -245,10 +244,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -288,9 +284,10 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -298,10 +295,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -341,9 +335,10 @@ func TestPRCheckout_differentRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -351,10 +346,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -394,9 +386,10 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -404,10 +397,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -445,9 +435,10 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -455,10 +446,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -486,6 +474,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") @@ -496,9 +525,10 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) 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", @@ -506,10 +536,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_create.go b/command/pr_create.go index 4053c031c..15406a27c 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) @@ -304,8 +304,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } headRemote = &context.Remote{ Remote: gitRemote, - Owner: headRepo.RepoOwner(), - Repo: headRepo.RepoName(), + Repo: headRepo, } } @@ -438,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/pr_create_test.go b/command/pr_create_test.go index 408a45256..9f2a1ebdc 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/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/command/pr_test.go b/command/pr_test.go index 9828836e9..961f4b6d5 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -967,28 +967,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(`{}`)) @@ -1017,6 +1014,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(` @@ -1032,18 +1030,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() @@ -1065,6 +1051,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`), @@ -1075,15 +1062,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(`{}`)) @@ -1108,6 +1086,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`), @@ -1118,15 +1097,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(`{}`)) @@ -1149,6 +1119,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`), @@ -1159,15 +1130,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(`{}`)) @@ -1196,28 +1158,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(`{}`)) @@ -1245,28 +1204,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(`{}`)) @@ -1284,16 +1240,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`), @@ -1325,6 +1279,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`), @@ -1341,15 +1296,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/command/repo.go b/command/repo.go index 7c15945aa..9d4a1dc6b 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) @@ -588,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) diff --git a/command/root.go b/command/root.go index 3693fd6f0..b87cf83c0 100644 --- a/command/root.go +++ b/command/root.go @@ -6,7 +6,10 @@ import ( "io" "net/http" "os" + "os/exec" + "path/filepath" "regexp" + "runtime" "runtime/debug" "strings" @@ -15,6 +18,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" @@ -330,23 +334,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) { @@ -363,25 +366,85 @@ func determineEditor(cmd *cobra.Command) (string, error) { return editorCommand, nil } -func ExpandAlias(args []string) ([]string, error) { - empty := []string{} +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) + + return preparedCmd.Run() +} + +var findSh = func() (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. +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 empty, nil + return } ctx := initContext() cfg, err := ctx.Config() if err != nil { - return empty, err + return } aliases, err := cfg.Aliases() if err != nil { - return empty, err + return } expansion, ok := aliases.Get(args[1]) if ok { + if strings.HasPrefix(expansion, "!") { + isShell = true + 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:]...) + } + + return + } + extraArgs := []string{} for i, a := range args[2:] { if !strings.Contains(expansion, "$") { @@ -392,18 +455,24 @@ 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) + 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 } - newArgs = append(newArgs, extraArgs...) - - return newArgs, nil + expanded = append(newArgs, extraArgs...) + return } - return args[1:], nil + expanded = args[1:] + return +} + +func connectedToTerminal(cmd *cobra.Command) bool { + return utils.IsTerminal(cmd.InOrStdin()) && utils.IsTerminal(cmd.OutOrStdout()) } 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") } 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/context/blank_context.go b/context/blank_context.go index 80a2cd298..3035b4d21 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/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/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/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/`. 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 diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 37fecbe8d..fbe748204 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 @@ -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()) @@ -39,32 +48,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()) + } }) } } 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) 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) 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) +}