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 1/5] 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 2/5] 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 3/5] 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 4/5] 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 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 5/5] 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) {