From b0b90afa872ccaceedeac8610f788af896e53dd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Feb 2021 17:06:29 +0100 Subject: [PATCH 01/11] issue/pr create: exit with nonzero status code when "Cancel" was chosen This is to indicate that the command had not finished successfully. --- pkg/cmd/issue/create/create.go | 1 + pkg/cmd/pr/create/create.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 0b2a93cd4..3d6b5b25c 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -249,6 +249,7 @@ func createRun(opts *CreateOptions) (err error) { if action == prShared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") + err = cmdutil.SilentError return } } else { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 52539b8f5..18b8c1e99 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -299,7 +299,8 @@ func createRun(opts *CreateOptions) (err error) { if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - return nil + err = cmdutil.SilentError + return } err = handlePush(*opts, *ctx) From fff051468eb793e89e946f4eac6ed1f5ab1efd20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Feb 2021 19:42:41 +0100 Subject: [PATCH 02/11] Avoid triggering recovery mechanism when cancelling `issue/pr create` --- pkg/cmd/issue/create/create.go | 4 ++-- pkg/cmd/pr/create/create.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 3d6b5b25c..298f3bf97 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -249,8 +249,8 @@ func createRun(opts *CreateOptions) (err error) { if action == prShared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = cmdutil.SilentError - return + err = nil // avoid triggering PreserveInput + return cmdutil.SilentError } } else { if tb.Title == "" { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 18b8c1e99..a56410726 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -299,8 +299,8 @@ func createRun(opts *CreateOptions) (err error) { if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = cmdutil.SilentError - return + err = nil // avoid triggering PreserveInput + return cmdutil.SilentError } err = handlePush(*opts, *ctx) From 3efa76430576eb11ac33aaadc4719cfc14db4ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Feb 2021 20:09:03 +0100 Subject: [PATCH 03/11] Avoid the issue/pr recovery mechanism handling Ctrl-C keypress in prompts Either InterruptErr or SilentErr will be present when the user has chosen "Cancel" or pressed Ctrl-C in prompts. We don't want the recovery mechanism to kick in these cases because the cancellation was likely willingly initiated by the user. --- pkg/cmd/issue/create/create.go | 4 ++-- pkg/cmd/pr/create/create.go | 4 ++-- pkg/cmd/pr/shared/preserve.go | 8 ++++++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 298f3bf97..3d6b5b25c 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -249,8 +249,8 @@ func createRun(opts *CreateOptions) (err error) { if action == prShared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = nil // avoid triggering PreserveInput - return cmdutil.SilentError + err = cmdutil.SilentError + return } } else { if tb.Title == "" { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index a56410726..18b8c1e99 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -299,8 +299,8 @@ func createRun(opts *CreateOptions) (err error) { if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = nil // avoid triggering PreserveInput - return cmdutil.SilentError + err = cmdutil.SilentError + return } err = handlePush(*opts, *ctx) diff --git a/pkg/cmd/pr/shared/preserve.go b/pkg/cmd/pr/shared/preserve.go index 4105823cd..44b8fa183 100644 --- a/pkg/cmd/pr/shared/preserve.go +++ b/pkg/cmd/pr/shared/preserve.go @@ -2,9 +2,12 @@ package shared import ( "encoding/json" + "errors" "fmt" "os" + "github.com/AlecAivazis/survey/v2/terminal" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" ) @@ -18,6 +21,11 @@ func PreserveInput(io *iostreams.IOStreams, state *IssueMetadataState, createErr return } + if errors.Is(*createErr, cmdutil.SilentError) || errors.Is(*createErr, terminal.InterruptErr) { + // these errors are user-initiated cancellations + return + } + out := io.ErrOut // this extra newline guards against appending to the end of a survey line From 2ebdde1ddd0b0887b92ba3f3fc4b59082bb7498c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 2 Mar 2021 13:48:44 +0100 Subject: [PATCH 04/11] Exit with status code "2" on user cancellation errors This also stops printing "interrupt" after Ctrl-C is pressed. --- cmd/gh/main.go | 10 ++++++---- pkg/cmd/gist/edit/edit.go | 2 +- pkg/cmd/issue/create/create.go | 2 +- pkg/cmd/pr/create/create.go | 4 ++-- pkg/cmd/pr/merge/merge.go | 2 +- pkg/cmd/pr/shared/preserve.go | 4 +--- pkg/cmd/release/create/create.go | 2 +- pkg/cmd/release/delete/delete.go | 2 +- pkg/cmdutil/errors.go | 13 ++++++++++++- 9 files changed, 26 insertions(+), 15 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index a0eba09a5..adfa28f90 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -148,6 +148,12 @@ func main() { rootCmd.SetArgs(expandedArgs) if cmd, err := rootCmd.ExecuteC(); err != nil { + if err == cmdutil.SilentError { + os.Exit(1) + } else if cmdutil.IsUserCancellation(err) { + os.Exit(2) + } + printError(stderr, err, cmd, hasDebug) var httpErr api.HTTPError @@ -177,10 +183,6 @@ func main() { } func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { - if err == cmdutil.SilentError { - return - } - var dnsError *net.DNSError if errors.As(err, &dnsError) { fmt.Fprintf(out, "error connecting to %s\n", dnsError.Name) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 7cba14800..3c16f1a16 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -177,7 +177,7 @@ func editRun(opts *EditOptions) error { case "Submit": stop = true case "Cancel": - return cmdutil.SilentError + return cmdutil.CancelError } if stop { diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 3d6b5b25c..0bd8dabc9 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -249,7 +249,7 @@ func createRun(opts *CreateOptions) (err error) { if action == prShared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = cmdutil.SilentError + err = cmdutil.CancelError return } } else { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 18b8c1e99..2dc7045ff 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -299,7 +299,7 @@ func createRun(opts *CreateOptions) (err error) { if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Discarding.") - err = cmdutil.SilentError + err = cmdutil.CancelError return } @@ -542,7 +542,7 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { } else if pushOptions[selectedOption] == "Skip pushing the branch" { isPushEnabled = false } else if pushOptions[selectedOption] == "Cancel" { - return nil, cmdutil.SilentError + return nil, cmdutil.CancelError } else { // "Create a fork of ..." if baseRepo.IsPrivate { diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 67498b726..5981b330f 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -228,7 +228,7 @@ func mergeRun(opts *MergeOptions) error { } if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Cancelled.") - return cmdutil.SilentError + return cmdutil.CancelError } } diff --git a/pkg/cmd/pr/shared/preserve.go b/pkg/cmd/pr/shared/preserve.go index 44b8fa183..6d3c32965 100644 --- a/pkg/cmd/pr/shared/preserve.go +++ b/pkg/cmd/pr/shared/preserve.go @@ -2,11 +2,9 @@ package shared import ( "encoding/json" - "errors" "fmt" "os" - "github.com/AlecAivazis/survey/v2/terminal" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" ) @@ -21,7 +19,7 @@ func PreserveInput(io *iostreams.IOStreams, state *IssueMetadataState, createErr return } - if errors.Is(*createErr, cmdutil.SilentError) || errors.Is(*createErr, terminal.InterruptErr) { + if cmdutil.IsUserCancellation(*createErr) { // these errors are user-initiated cancellations return } diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 9b87b8845..fd94a511a 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -262,7 +262,7 @@ func createRun(opts *CreateOptions) error { case "Save as draft": opts.Draft = true case "Cancel": - return cmdutil.SilentError + return cmdutil.CancelError default: return fmt.Errorf("invalid action: %v", opts.SubmitAction) } diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index fde3fbcee..c880f9d58 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -78,7 +78,7 @@ func deleteRun(opts *DeleteOptions) error { } if !confirmed { - return cmdutil.SilentError + return cmdutil.CancelError } } diff --git a/pkg/cmdutil/errors.go b/pkg/cmdutil/errors.go index 77ca58340..aa33e98ef 100644 --- a/pkg/cmdutil/errors.go +++ b/pkg/cmdutil/errors.go @@ -1,6 +1,10 @@ package cmdutil -import "errors" +import ( + "errors" + + "github.com/AlecAivazis/survey/v2/terminal" +) // FlagError is the kind of error raised in flag processing type FlagError struct { @@ -17,3 +21,10 @@ func (fe FlagError) Unwrap() error { // SilentError is an error that triggers exit code 1 without any error messaging var SilentError = errors.New("SilentError") + +// CancelError signals user-initiated cancellation +var CancelError = errors.New("CancelError") + +func IsUserCancellation(err error) bool { + return errors.Is(err, CancelError) || errors.Is(err, terminal.InterruptErr) +} From 98f1f5ec0d570a1b89cfe249cc6fff37dc6d7ab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Mar 2021 16:20:21 +0100 Subject: [PATCH 05/11] Use absolute path when configuring gh as git credential This keeps git operations working even when PATH is modified, e.g. `brew update` will work even though Homebrew runs the command explicitly without `/usr/local/bin` in PATH. Additionally, this inserts a blank value for `credential.*.helper` to instruct git to ignore previously configured credential helpers, i.e. those that might have been set up in system configuration files. We do this because otherwise, git will store the credential obtained from gh in every other credential helper in the chain, which we want to avoid. Before: git config --global credential.https://github.com.helper '!gh auth git-credential' After: git config --global credential.https://github.com.helper '' git config --global --add credential.https://github.com.helper '!/path/to/gh auth git-credential' --- pkg/cmd/auth/login/login.go | 5 ++++ pkg/cmd/auth/refresh/refresh.go | 3 +++ pkg/cmd/auth/shared/git_credential.go | 30 +++++++++++++++++++--- pkg/cmd/auth/shared/git_credential_test.go | 29 ++++++++++++++++++--- pkg/cmd/auth/shared/login_flow.go | 3 ++- pkg/cmd/factory/default.go | 6 +++++ pkg/cmdutil/factory.go | 3 +++ 7 files changed, 70 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 64baae10f..995405021 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -23,6 +23,8 @@ type LoginOptions struct { Config func() (config.Config, error) HttpClient func() (*http.Client, error) + MainExecutable string + Interactive bool Hostname string @@ -36,6 +38,8 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + + MainExecutable: f.Executable, } var tokenStdin bool @@ -189,6 +193,7 @@ func loginRun(opts *LoginOptions) error { Interactive: opts.Interactive, Web: opts.Web, Scopes: opts.Scopes, + Executable: opts.MainExecutable, }) } diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 32f236ca9..58267182c 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -19,6 +19,8 @@ type RefreshOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) + MainExecutable string + Hostname string Scopes []string AuthFlow func(config.Config, *iostreams.IOStreams, string, []string) error @@ -34,6 +36,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. _, err := authflow.AuthFlowWithConfig(cfg, io, hostname, "", scopes) return err }, + MainExecutable: f.Executable, } cmd := &cobra.Command{ diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index e67d95a6e..d95004469 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -15,6 +15,8 @@ import ( ) type GitCredentialFlow struct { + Executable string + shouldSetup bool helper string scopes []string @@ -50,13 +52,26 @@ func (flow *GitCredentialFlow) ShouldSetup() bool { } func (flow *GitCredentialFlow) Setup(hostname, username, authToken string) error { - return GitCredentialSetup(hostname, username, authToken, flow.helper) + return flow.gitCredentialSetup(hostname, username, authToken) } -func GitCredentialSetup(hostname, username, password, helper string) error { - if helper == "" { +func (flow *GitCredentialFlow) gitCredentialSetup(hostname, username, password string) error { + if flow.helper == "" { + // first use a blank value to indicate to git we want to sever the chain of credential helpers + preConfigureCmd, err := git.GitCommand("config", "--global", gitCredentialHelperKey(hostname), "") + if err != nil { + return err + } + if err = run.PrepareCmd(preConfigureCmd).Run(); err != nil { + return err + } + // use GitHub CLI as a credential helper (for this host only) - configureCmd, err := git.GitCommand("config", "--global", gitCredentialHelperKey(hostname), "!gh auth git-credential") + configureCmd, err := git.GitCommand( + "config", "--global", "--add", + gitCredentialHelperKey(hostname), + fmt.Sprintf("!%s auth git-credential", shellQuote(flow.Executable)), + ) if err != nil { return err } @@ -124,3 +139,10 @@ func isOurCredentialHelper(cmd string) bool { return strings.TrimSuffix(filepath.Base(args[0]), ".exe") == "gh" } + +func shellQuote(s string) string { + if strings.ContainsAny(s, " $") { + return "'" + s + "'" + } + return s +} diff --git a/pkg/cmd/auth/shared/git_credential_test.go b/pkg/cmd/auth/shared/git_credential_test.go index debcf2d12..5a0b8b10b 100644 --- a/pkg/cmd/auth/shared/git_credential_test.go +++ b/pkg/cmd/auth/shared/git_credential_test.go @@ -12,7 +12,12 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { cs.Register(`git credential reject`, 0, "") cs.Register(`git credential approve`, 0, "") - if err := GitCredentialSetup("example.com", "monalisa", "PASSWD", "osxkeychain"); err != nil { + f := GitCredentialFlow{ + Executable: "gh", + helper: "osxkeychain", + } + + if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { t.Errorf("GitCredentialSetup() error = %v", err) } } @@ -20,13 +25,29 @@ func TestGitCredentialSetup_configureExisting(t *testing.T) { func TestGitCredentialSetup_setOurs(t *testing.T) { cs, restoreRun := run.Stub() defer restoreRun(t) - cs.Register(`git config --global credential\.https://example\.com\.helper`, 0, "", func(args []string) { - if val := args[len(args)-1]; val != "!gh auth git-credential" { + cs.Register(`git config --global credential\.`, 0, "", func(args []string) { + if key := args[len(args)-2]; key != "credential.https://example.com.helper" { + t.Errorf("git config key was %q", key) + } + if val := args[len(args)-1]; val != "" { + t.Errorf("global credential helper configured to %q", val) + } + }) + cs.Register(`git config --global --add credential\.`, 0, "", func(args []string) { + if key := args[len(args)-2]; key != "credential.https://example.com.helper" { + t.Errorf("git config key was %q", key) + } + if val := args[len(args)-1]; val != "!/path/to/gh auth git-credential" { t.Errorf("global credential helper configured to %q", val) } }) - if err := GitCredentialSetup("example.com", "monalisa", "PASSWD", ""); err != nil { + f := GitCredentialFlow{ + Executable: "/path/to/gh", + helper: "", + } + + if err := f.gitCredentialSetup("example.com", "monalisa", "PASSWD"); err != nil { t.Errorf("GitCredentialSetup() error = %v", err) } } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 6f75a784f..42369c466 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -28,6 +28,7 @@ type LoginOptions struct { Interactive bool Web bool Scopes []string + Executable string sshContext sshContext } @@ -56,7 +57,7 @@ func Login(opts *LoginOptions) error { var additionalScopes []string - credentialFlow := &GitCredentialFlow{} + credentialFlow := &GitCredentialFlow{Executable: opts.Executable} if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 2c70ab5cd..ff8ca8ac9 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -44,6 +44,11 @@ func New(appVersion string) *cmdutil.Factory { } remotesFunc := rr.Resolver(hostOverride) + ghExecutable := "gh" + if exe, err := os.Executable(); err == nil { + ghExecutable = exe + } + return &cmdutil.Factory{ IOStreams: io, Config: configFunc, @@ -70,5 +75,6 @@ func New(appVersion string) *cmdutil.Factory { } return currentBranch, nil }, + Executable: ghExecutable, } } diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 62ed5d802..eb9546b52 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -16,4 +16,7 @@ type Factory struct { Remotes func() (context.Remotes, error) Config func() (config.Config, error) Branch func() (string, error) + + // Executable is the path to the currently invoked gh binary + Executable string } From 440b59f8c393d9aceeae1bc6e43742ebca494f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Mar 2021 20:12:51 +0100 Subject: [PATCH 06/11] Add the `api --preview` flag to opt into GitHub API previews This was previously available manually via the `-H` flag, but it was verbose, especially when opting into multiple previews. --- pkg/cmd/api/api.go | 19 ++++++++++++++++++- pkg/cmd/api/api_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 8273b3635..7427d05b6 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -37,6 +37,7 @@ type ApiOptions struct { MagicFields []string RawFields []string RequestHeaders []string + Previews []string ShowResponseHeaders bool Paginate bool Silent bool @@ -106,7 +107,10 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command $ gh api -X GET search/issues -f q='repo:cli/cli is:open remote' # set a custom HTTP header - $ gh api -H 'Accept: application/vnd.github.XYZ-preview+json' ... + $ gh api -H 'Accept: application/vnd.github.v3.raw+json' ... + + # opt into GitHub API previews + $ gh api --preview baptiste,nebula ... # list releases with GraphQL $ gh api graphql -F owner=':owner' -F name=':repo' -f query=' @@ -175,6 +179,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command cmd.Flags().StringArrayVarP(&opts.MagicFields, "field", "F", nil, "Add a typed parameter in `key=value` format") cmd.Flags().StringArrayVarP(&opts.RawFields, "raw-field", "f", nil, "Add a string parameter in `key=value` format") cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add a HTTP request header in `key:value` format") + cmd.Flags().StringSliceVarP(&opts.Previews, "preview", "p", nil, "Opt into GitHub API previews") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The `file` to use as body for the HTTP request") @@ -219,6 +224,10 @@ func apiRun(opts *ApiOptions) error { } } + if len(opts.Previews) > 0 { + requestHeaders = append(requestHeaders, "Accept: "+previewNamesToMIMETypes(opts.Previews)) + } + httpClient, err := opts.HttpClient() if err != nil { return err @@ -513,3 +522,11 @@ func parseErrorResponse(r io.Reader, statusCode int) (io.Reader, string, error) return bodyCopy, "", nil } + +func previewNamesToMIMETypes(names []string) string { + types := []string{fmt.Sprintf("application/vnd.github.%s-preview+json", names[0])} + for _, p := range names[1:] { + types = append(types, fmt.Sprintf("application/vnd.github.%s-preview", p)) + } + return strings.Join(types, ", ") +} diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 69ffa100d..44582cac8 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -910,3 +910,29 @@ func Test_fillPlaceholders(t *testing.T) { }) } } + +func Test_previewNamesToMIMETypes(t *testing.T) { + tests := []struct { + name string + previews []string + want string + }{ + { + name: "single", + previews: []string{"nebula"}, + want: "application/vnd.github.nebula-preview+json", + }, + { + name: "multiple", + previews: []string{"nebula", "baptiste", "squirrel-girl"}, + want: "application/vnd.github.nebula-preview+json, application/vnd.github.baptiste-preview, application/vnd.github.squirrel-girl-preview", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := previewNamesToMIMETypes(tt.previews); got != tt.want { + t.Errorf("previewNamesToMIMETypes() = %q, want %q", got, tt.want) + } + }) + } +} From 9234163679f265dacdd3c87877eeee21560bc401 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 13:22:11 +0100 Subject: [PATCH 07/11] Formalize `gh` process exit codes Here are the statuses: - 0: success - 1: misc. error - 2: user interrupt/cancellation - 4: authentication needed These old exit codes are now changed to "1": - we used to return "2" for config file errors; - we used to return "2" for alias expansion errors; - we used to return "3" for alias runtime errors. I do not believe that there is a need to distinguish these specific cases via exit status, and converting them to "1" frees codes "2" and "3" for more practical use. --- cmd/gh/main.go | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 3619eca9f..7da84d9fc 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -13,6 +13,7 @@ import ( "time" surveyCore "github.com/AlecAivazis/survey/v2/core" + "github.com/AlecAivazis/survey/v2/terminal" "github.com/cli/cli/api" "github.com/cli/cli/internal/build" "github.com/cli/cli/internal/config" @@ -32,7 +33,21 @@ import ( var updaterEnabled = "" +type exitCode int + +const ( + exitOK exitCode = 0 + exitError exitCode = 1 + exitCancel exitCode = 2 + exitAuth exitCode = 4 +) + func main() { + code := mainRun() + os.Exit(int(code)) +} + +func mainRun() exitCode { buildDate := build.Date buildVersion := build.Version @@ -78,7 +93,7 @@ func main() { cfg, err := cmdFactory.Config() if err != nil { fmt.Fprintf(stderr, "failed to read configuration: %s\n", err) - os.Exit(2) + return exitError } if prompt, _ := cfg.Get("", "prompt"); prompt == "disabled" { @@ -102,7 +117,7 @@ func main() { expandedArgs, isShell, err = expand.ExpandAlias(cfg, os.Args, nil) if err != nil { fmt.Fprintf(stderr, "failed to process aliases: %s\n", err) - os.Exit(2) + return exitError } if hasDebug { @@ -113,7 +128,7 @@ func main() { exe, err := safeexec.LookPath(expandedArgs[0]) if err != nil { fmt.Fprintf(stderr, "failed to run external command: %s", err) - os.Exit(3) + return exitError } externalCmd := exec.Command(exe, expandedArgs[1:]...) @@ -125,14 +140,14 @@ func main() { err = preparedCmd.Run() if err != nil { if ee, ok := err.(*exec.ExitError); ok { - os.Exit(ee.ExitCode()) + return exitCode(ee.ExitCode()) } fmt.Fprintf(stderr, "failed to run external command: %s", err) - os.Exit(3) + return exitError } - os.Exit(0) + return exitOK } } @@ -142,29 +157,33 @@ func main() { fmt.Fprintln(stderr, cs.Bold("Welcome to GitHub CLI!")) fmt.Fprintln(stderr) fmt.Fprintln(stderr, "To authenticate, please run `gh auth login`.") - os.Exit(4) + return exitAuth } rootCmd.SetArgs(expandedArgs) if cmd, err := rootCmd.ExecuteC(); err != nil { if err == cmdutil.SilentError { - os.Exit(1) + return exitError } else if cmdutil.IsUserCancellation(err) { - os.Exit(2) + if errors.Is(err, terminal.InterruptErr) { + // ensure the next shell prompt will start on its own line + fmt.Fprint(stderr, "\n") + } + return exitCancel } printError(stderr, err, cmd, hasDebug) var httpErr api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 401 { - fmt.Println("hint: try authenticating with `gh auth login`") + fmt.Fprintln(stderr, "hint: try authenticating with `gh auth login`") } - os.Exit(1) + return exitError } if root.HasFailed() { - os.Exit(1) + return exitError } newRelease := <-updateMessageChan @@ -175,7 +194,7 @@ func main() { } if isHomebrew && isRecentRelease(newRelease.PublishedAt) { // do not notify Homebrew users before the version bump had a chance to get merged into homebrew-core - return + return exitOK } fmt.Fprintf(stderr, "\n\n%s %s → %s\n", ansi.Color("A new release of gh is available:", "yellow"), @@ -187,6 +206,8 @@ func main() { fmt.Fprintf(stderr, "%s\n\n", ansi.Color(newRelease.URL, "yellow")) } + + return exitOK } func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { From cfbfb578f07e329623d1bd5c0010a89bbf613e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 13:41:50 +0100 Subject: [PATCH 08/11] Read `Executable` from factory instead of from stdlib --- cmd/gh/main.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index e20452808..0934863fc 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -163,10 +163,7 @@ func main() { newRelease := <-updateMessageChan if newRelease != nil { - isHomebrew := false - if ghExe, err := os.Executable(); err == nil { - isHomebrew = isUnderHomebrew(ghExe) - } + isHomebrew := isUnderHomebrew(cmdFactory.Executable) if isHomebrew && isRecentRelease(newRelease.PublishedAt) { // do not notify Homebrew users before the version bump had a chance to get merged into homebrew-core return From 0f27084f570995936fb63671ec247e70d4768808 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 15:01:59 +0100 Subject: [PATCH 09/11] Add flag parsing test for `api --template` --- pkg/cmd/api/api_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 16df36c20..75132d7b4 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -47,6 +47,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -66,6 +67,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -85,6 +87,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -104,6 +107,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -123,6 +127,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: true, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -142,6 +147,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: true, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -166,6 +172,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: true, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -190,6 +197,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -214,6 +222,7 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: 0, + Template: "", }, wantsErr: false, }, @@ -233,6 +242,27 @@ func Test_NewCmdApi(t *testing.T) { Paginate: false, Silent: false, CacheTTL: time.Minute * 5, + Template: "", + }, + wantsErr: false, + }, + { + name: "with template", + cli: "user -t 'hello {{.name}}'", + wants: ApiOptions{ + Hostname: "", + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "user", + RequestInputFile: "", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + Paginate: false, + Silent: false, + CacheTTL: 0, + Template: "hello {{.name}}", }, wantsErr: false, }, @@ -270,6 +300,7 @@ func Test_NewCmdApi(t *testing.T) { assert.Equal(t, tt.wants.Paginate, opts.Paginate) assert.Equal(t, tt.wants.Silent, opts.Silent) assert.Equal(t, tt.wants.CacheTTL, opts.CacheTTL) + assert.Equal(t, tt.wants.Template, opts.Template) }) } } From f53ad7161ac956f94f1c4a6770f081472a0f8971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 16:35:08 +0100 Subject: [PATCH 10/11] Add more `api --template` tests --- pkg/cmd/api/api.go | 21 +------ pkg/cmd/api/template.go | 43 +++++++++++++- pkg/cmd/api/template_test.go | 105 ++++++++++++++++++++++++++++++++++- 3 files changed, 147 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 106a564d2..72e1edb19 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -14,7 +14,6 @@ import ( "strconv" "strings" "syscall" - "text/template" "time" "github.com/MakeNowJust/heredoc" @@ -335,25 +334,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream if opts.Template != "" { // TODO: reuse parsed template across pagination invocations - var t *template.Template - t, err = parseTemplate(opts.Template, opts.IO.ColorEnabled()) - if err != nil { - return - } - - var jsonData []byte - jsonData, err = ioutil.ReadAll(responseBody) - if err != nil { - return - } - - var m interface{} - err = json.Unmarshal(jsonData, &m) - if err != nil { - return - } - - err = t.Execute(opts.IO.Out, m) + err = executeTemplate(opts.IO.Out, responseBody, opts.Template, opts.IO.ColorEnabled()) if err != nil { return } diff --git a/pkg/cmd/api/template.go b/pkg/cmd/api/template.go index 823852e51..78c7a96c8 100644 --- a/pkg/cmd/api/template.go +++ b/pkg/cmd/api/template.go @@ -1,7 +1,10 @@ package api import ( + "encoding/json" "fmt" + "io" + "io/ioutil" "math" "strconv" "strings" @@ -31,7 +34,7 @@ func parseTemplate(tpl string, colorEnabled bool) (*template.Template, error) { if err != nil { return "", err } - return utils.FuzzyAgoAbbr(now, t), nil + return timeAgo(now.Sub(t)), nil }, "pluck": templatePluck, @@ -47,6 +50,25 @@ func parseTemplate(tpl string, colorEnabled bool) (*template.Template, error) { return template.New("").Funcs(templateFuncs).Parse(tpl) } +func executeTemplate(w io.Writer, input io.Reader, templateStr string, colorEnabled bool) error { + t, err := parseTemplate(templateStr, colorEnabled) + if err != nil { + return err + } + + jsonData, err := ioutil.ReadAll(input) + if err != nil { + return err + } + + var data interface{} + if err := json.Unmarshal(jsonData, &data); err != nil { + return err + } + + return t.Execute(w, data) +} + func jsonScalarToString(input interface{}) (string, error) { switch tt := input.(type) { case string: @@ -94,3 +116,22 @@ func templateJoin(sep string, input []interface{}) (string, error) { } return strings.Join(results, sep), nil } + +func timeAgo(ago time.Duration) string { + if ago < time.Minute { + return "just now" + } + if ago < time.Hour { + return utils.Pluralize(int(ago.Minutes()), "minute") + " ago" + } + if ago < 24*time.Hour { + return utils.Pluralize(int(ago.Hours()), "hour") + " ago" + } + if ago < 30*24*time.Hour { + return utils.Pluralize(int(ago.Hours())/24, "day") + " ago" + } + if ago < 365*24*time.Hour { + return utils.Pluralize(int(ago.Hours())/24/30, "month") + " ago" + } + return utils.Pluralize(int(ago.Hours()/24/365), "year") + " ago" +} diff --git a/pkg/cmd/api/template_test.go b/pkg/cmd/api/template_test.go index ff03d9a98..067b87fcc 100644 --- a/pkg/cmd/api/template_test.go +++ b/pkg/cmd/api/template_test.go @@ -1,6 +1,15 @@ package api -import "testing" +import ( + "bytes" + "fmt" + "io" + "strings" + "testing" + "time" + + "github.com/MakeNowJust/heredoc" +) func Test_jsonScalarToString(t *testing.T) { tests := []struct { @@ -58,3 +67,97 @@ func Test_jsonScalarToString(t *testing.T) { }) } } + +func Test_executeTemplate(t *testing.T) { + type args struct { + json io.Reader + template string + colorize bool + } + tests := []struct { + name string + args args + wantW string + wantErr bool + }{ + { + name: "color", + args: args{ + json: strings.NewReader(`{}`), + template: `{{color "blue+h" "songs are like tattoos"}}`, + colorize: false, + }, + wantW: "\x1b[0;94msongs are like tattoos\x1b[0m", + }, + { + name: "autocolor enabled", + args: args{ + json: strings.NewReader(`{}`), + template: `{{autocolor "red" "stop"}}`, + colorize: true, + }, + wantW: "\x1b[0;31mstop\x1b[0m", + }, + { + name: "autocolor disabled", + args: args{ + json: strings.NewReader(`{}`), + template: `{{autocolor "red" "go"}}`, + colorize: false, + }, + wantW: "go", + }, + { + name: "timefmt", + args: args{ + json: strings.NewReader(`{"created_at":"2008-02-25T20:18:33Z"}`), + template: `{{.created_at | timefmt "Mon Jan 2, 2006"}}`, + colorize: false, + }, + wantW: "Mon Feb 25, 2008", + }, + { + name: "timeago", + args: args{ + json: strings.NewReader(fmt.Sprintf(`{"created_at":"%s"}`, time.Now().Add(-5*time.Minute).Format(time.RFC3339))), + template: `{{.created_at | timeago}}`, + colorize: false, + }, + wantW: "5 minutes ago", + }, + { + name: "pluck", + args: args{ + json: strings.NewReader(heredoc.Doc(`[ + {"name": "bug"}, + {"name": "feature request"}, + {"name": "chore"} + ]`)), + template: `{{range(pluck "name" .)}}{{. | printf "%s\n"}}{{end}}`, + colorize: false, + }, + wantW: "bug\nfeature request\nchore\n", + }, + { + name: "join", + args: args{ + json: strings.NewReader(`[ "bug", "feature request", "chore" ]`), + template: `{{join "\t" .}}`, + colorize: false, + }, + wantW: "bug\tfeature request\tchore", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := &bytes.Buffer{} + if err := executeTemplate(w, tt.args.json, tt.args.template, tt.args.colorize); (err != nil) != tt.wantErr { + t.Errorf("executeTemplate() error = %v, wantErr %v", err, tt.wantErr) + return + } + if gotW := w.String(); gotW != tt.wantW { + t.Errorf("executeTemplate() = %q, want %q", gotW, tt.wantW) + } + }) + } +} From eb08774370a0240f07a6235edceb53dd29aa2aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Mar 2021 16:48:06 +0100 Subject: [PATCH 11/11] Assert that `executeTemplate` is invoked --- pkg/cmd/api/api_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 75132d7b4..3b2acb6cf 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -426,6 +426,20 @@ func Test_apiRun(t *testing.T) { stdout: "HTTP/1.1 200 Okey-dokey\nContent-Type: text/plain\r\n\r\n", stderr: ``, }, + { + name: "output template", + options: ApiOptions{ + Template: `{{.status}}`, + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"status":"not a cat"}`)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + err: nil, + stdout: "not a cat", + stderr: ``, + }, } for _, tt := range tests {