From d01355e24bc881797c1b0a61d203c5d90d35dcf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 4 Sep 2020 23:07:17 +0200 Subject: [PATCH 1/9] Add global PAGER support Extract the ad-hoc PAGER behavior from `pr diff` command and make it available opt-in to any command through IOStreams. --- pkg/cmd/pr/diff/diff.go | 26 +++++-------------- pkg/cmd/pr/diff/diff_test.go | 41 +---------------------------- pkg/iostreams/iostreams.go | 50 ++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 60 deletions(-) diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index c6902e5b5..3c55e7db8 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -6,8 +6,6 @@ import ( "fmt" "io" "net/http" - "os" - "os/exec" "strings" "github.com/cli/cli/api" @@ -16,7 +14,6 @@ import ( "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" - "github.com/google/shlex" "github.com/spf13/cobra" ) @@ -93,17 +90,17 @@ func diffRun(opts *DiffOptions) error { } defer diff.Close() + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + if opts.UseColor == "never" { _, err = io.Copy(opts.IO.Out, diff) return err } - if opts.IO.IsStdoutTTY() { - if pager := os.Getenv("PAGER"); pager != "" { - return runPager(pager, diff, opts.IO.Out) - } - } - diffLines := bufio.NewScanner(diff) for diffLines.Scan() { diffLine := diffLines.Text() @@ -148,14 +145,3 @@ func isRemovalLine(dl string) bool { func validColorFlag(c string) bool { return c == "auto" || c == "always" || c == "never" } - -var runPager = func(pager string, diff io.Reader, out io.Writer) error { - args, err := shlex.Split(pager) - if err != nil { - return err - } - pagerCmd := exec.Command(args[0], args[1:]...) - pagerCmd.Stdin = diff - pagerCmd.Stdout = out - return pagerCmd.Run() -} diff --git a/pkg/cmd/pr/diff/diff_test.go b/pkg/cmd/pr/diff/diff_test.go index dc93970b6..5e362ac4d 100644 --- a/pkg/cmd/pr/diff/diff_test.go +++ b/pkg/cmd/pr/diff/diff_test.go @@ -2,10 +2,8 @@ package diff import ( "bytes" - "io" "io/ioutil" "net/http" - "os" "testing" "github.com/cli/cli/context" @@ -214,13 +212,8 @@ func TestPRDiff_notty(t *testing.T) { } func TestPRDiff_tty(t *testing.T) { - pager := os.Getenv("PAGER") http := &httpmock.Registry{} - defer func() { - os.Setenv("PAGER", pager) - http.Verify(t) - }() - os.Setenv("PAGER", "") + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -237,38 +230,6 @@ func TestPRDiff_tty(t *testing.T) { assert.Contains(t, output.String(), "\x1b[32m+site: bin/gh\x1b[m") } -func TestPRDiff_pager(t *testing.T) { - realRunPager := runPager - pager := os.Getenv("PAGER") - http := &httpmock.Registry{} - defer func() { - runPager = realRunPager - os.Setenv("PAGER", pager) - http.Verify(t) - }() - runPager = func(pager string, diff io.Reader, out io.Writer) error { - _, err := io.Copy(out, diff) - return err - } - os.Setenv("PAGER", "fakepager") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`)) - http.StubResponse(200, bytes.NewBufferString(testDiff)) - output, err := runCommand(http, nil, true, "") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if diff := cmp.Diff(testDiff, output.String()); diff != "" { - t.Errorf("command output did not match:\n%s", diff) - } -} - const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 73974448..b7fc0154 100644 --- a/.github/workflows/releases.yml diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index e9b9c2220..d3f7e3c11 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" + "github.com/google/shlex" "github.com/mattn/go-colorable" "github.com/mattn/go-isatty" "golang.org/x/crypto/ssh/terminal" @@ -28,6 +29,9 @@ type IOStreams struct { stdoutIsTTY bool stderrTTYOverride bool stderrIsTTY bool + + pagerCommand string + pagerProcess *os.Process } func (s *IOStreams) ColorEnabled() bool { @@ -79,6 +83,51 @@ func (s *IOStreams) IsStderrTTY() bool { return false } +func (s *IOStreams) StartPager() error { + if s.pagerCommand == "" || !s.IsStdoutTTY() { + return nil + } + + pagerArgs, err := shlex.Split(s.pagerCommand) + if err != nil { + return err + } + + pagerEnv := os.Environ() + if _, ok := os.LookupEnv("LESS"); !ok { + pagerEnv = append(pagerEnv, "LESS=FRX") + } + if _, ok := os.LookupEnv("LV"); !ok { + pagerEnv = append(pagerEnv, "LV=-c") + } + + pagerCmd := exec.Command(pagerArgs[0], pagerArgs[1:]...) + pagerCmd.Env = pagerEnv + pagerCmd.Stdout = s.Out + pagerCmd.Stderr = s.ErrOut + pagedOut, err := pagerCmd.StdinPipe() + if err != nil { + return err + } + s.Out = pagedOut + err = pagerCmd.Start() + if err != nil { + return err + } + s.pagerProcess = pagerCmd.Process + return nil +} + +func (s *IOStreams) StopPager() { + if s.pagerProcess == nil { + return + } + + s.Out.(io.ReadCloser).Close() + _, _ = s.pagerProcess.Wait() + s.pagerProcess = nil +} + func (s *IOStreams) TerminalWidth() int { defaultWidth := 80 if s.stdoutTTYOverride { @@ -111,6 +160,7 @@ func System() *IOStreams { Out: colorable.NewColorable(os.Stdout), ErrOut: colorable.NewColorable(os.Stderr), colorEnabled: os.Getenv("NO_COLOR") == "" && stdoutIsTTY, + pagerCommand: os.Getenv("PAGER"), } // prevent duplicate isTerminal queries now that we know the answer From ccfe38de260f06bd399eaf6daa53d60980cf4397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 4 Sep 2020 23:08:09 +0200 Subject: [PATCH 2/9] Add PAGER support to commands that produce significant output --- pkg/cmd/issue/list/fixtures/issueList.json | 3 ++ pkg/cmd/issue/list/list.go | 8 +++- pkg/cmd/issue/list/list_test.go | 24 ++++++----- pkg/cmd/issue/status/status.go | 6 +++ pkg/cmd/issue/view/view.go | 8 +++- pkg/cmd/pr/list/list.go | 8 +++- pkg/cmd/pr/list/list_test.go | 47 ++++++++-------------- pkg/cmd/pr/status/status.go | 6 +++ pkg/cmd/pr/view/view.go | 6 +++ pkg/cmd/repo/view/view.go | 14 ++++++- 10 files changed, 86 insertions(+), 44 deletions(-) diff --git a/pkg/cmd/issue/list/fixtures/issueList.json b/pkg/cmd/issue/list/fixtures/issueList.json index 4b19f3930..1f878f7a5 100644 --- a/pkg/cmd/issue/list/fixtures/issueList.json +++ b/pkg/cmd/issue/list/fixtures/issueList.json @@ -9,6 +9,7 @@ "number": 1, "title": "number won", "url": "https://wow.com", + "updatedAt": "2011-01-26T19:01:12Z", "labels": { "nodes": [ { @@ -22,6 +23,7 @@ "number": 2, "title": "number too", "url": "https://wow.com", + "updatedAt": "2011-01-26T19:01:12Z", "labels": { "nodes": [ { @@ -35,6 +37,7 @@ "number": 4, "title": "number fore", "url": "https://wow.com", + "updatedAt": "2011-01-26T19:01:12Z", "labels": { "nodes": [ { diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index 1065e61be..017556af6 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -116,10 +116,16 @@ func listRun(opts *ListOptions) error { return err } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + if isTerminal { hasFilters := opts.State != "open" || len(opts.Labels) > 0 || opts.Assignee != "" || opts.Author != "" || opts.Mention != "" || opts.Milestone != "" title := prShared.ListHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, hasFilters) - fmt.Fprintf(opts.IO.ErrOut, "\n%s\n\n", title) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } issueShared.PrintIssues(opts.IO, "", len(listResult.Issues), listResult.Issues) diff --git a/pkg/cmd/issue/list/list_test.go b/pkg/cmd/issue/list/list_test.go index 6646a10f7..ff202b6f3 100644 --- a/pkg/cmd/issue/list/list_test.go +++ b/pkg/cmd/issue/list/list_test.go @@ -7,8 +7,10 @@ import ( "net/http" "os/exec" "reflect" + "regexp" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" @@ -97,15 +99,19 @@ func TestIssueList_tty(t *testing.T) { t.Errorf("error running command `issue list`: %v", err) } - eq(t, output.Stderr(), ` -Showing 3 of 3 open issues in OWNER/REPO + out := output.String() + timeRE := regexp.MustCompile(`\d+ years`) + out = timeRE.ReplaceAllString(out, "X years") -`) + assert.Equal(t, heredoc.Doc(` - test.ExpectLines(t, output.String(), - "number won", - "number too", - "number fore") + Showing 3 of 3 open issues in OWNER/REPO + + #1 number won (label) about X years ago + #2 number too (label) about X years ago + #4 number fore (label) about X years ago + `), out) + assert.Equal(t, ``, output.Stderr()) } func TestIssueList_tty_withFlags(t *testing.T) { @@ -141,8 +147,8 @@ func TestIssueList_tty_withFlags(t *testing.T) { t.Errorf("error running command `issue list`: %v", err) } - eq(t, output.String(), "") - eq(t, output.Stderr(), ` + eq(t, output.Stderr(), "") + eq(t, output.String(), ` No issues match your search in OWNER/REPO `) diff --git a/pkg/cmd/issue/status/status.go b/pkg/cmd/issue/status/status.go index d1b68bc0d..42ba91823 100644 --- a/pkg/cmd/issue/status/status.go +++ b/pkg/cmd/issue/status/status.go @@ -68,6 +68,12 @@ func statusRun(opts *StatusOptions) error { return err } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + out := opts.IO.Out fmt.Fprintln(out, "") diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 6d2b0eeaa..7ff7a0e43 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -86,10 +86,16 @@ func viewRun(opts *ViewOptions) error { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } + + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + if opts.IO.IsStdoutTTY() { return printHumanIssuePreview(opts.IO.Out, issue) } - return printRawIssuePreview(opts.IO.Out, issue) } diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index fd1ebb2b9..e4fe318c8 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -133,10 +133,16 @@ func listRun(opts *ListOptions) error { return err } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + if opts.IO.IsStdoutTTY() { hasFilters := opts.State != "open" || len(opts.Labels) > 0 || opts.BaseBranch != "" || opts.Assignee != "" title := shared.ListHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, hasFilters) - fmt.Fprintf(opts.IO.ErrOut, "\n%s\n\n", title) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } table := utils.NewTablePrinter(opts.IO) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 5faf42b57..ab99e4aa9 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -6,10 +6,10 @@ import ( "net/http" "os/exec" "reflect" - "regexp" "strings" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/cmdutil" @@ -76,23 +76,15 @@ func TestPRList(t *testing.T) { t.Fatal(err) } - assert.Equal(t, ` -Showing 3 of 3 open pull requests in OWNER/REPO + assert.Equal(t, heredoc.Doc(` -`, output.Stderr()) - - lines := strings.Split(output.String(), "\n") - res := []*regexp.Regexp{ - regexp.MustCompile(`#32.*New feature.*feature`), - regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), - regexp.MustCompile(`#28.*Improve documentation.*docs`), - } - - for i, r := range res { - if !r.MatchString(lines[i]) { - t.Errorf("%s did not match %s", lines[i], r) - } - } + Showing 3 of 3 open pull requests in OWNER/REPO + + #32 New feature feature + #29 Fixed bad bug hubot:bug-fix + #28 Improve documentation docs + `), output.String()) + assert.Equal(t, ``, output.Stderr()) } func TestPRList_nontty(t *testing.T) { @@ -130,8 +122,8 @@ func TestPRList_filtering(t *testing.T) { t.Fatal(err) } - eq(t, output.String(), "") - eq(t, output.Stderr(), ` + eq(t, output.Stderr(), "") + eq(t, output.String(), ` No pull requests match your search in OWNER/REPO `) @@ -150,19 +142,12 @@ func TestPRList_filteringRemoveDuplicate(t *testing.T) { t.Fatal(err) } - lines := strings.Split(output.String(), "\n") - - res := []*regexp.Regexp{ - regexp.MustCompile(`#32.*New feature.*feature`), - regexp.MustCompile(`#29.*Fixed bad bug.*hubot:bug-fix`), - regexp.MustCompile(`#28.*Improve documentation.*docs`), - } - - for i, r := range res { - if !r.MatchString(lines[i]) { - t.Errorf("%s did not match %s", lines[i], r) - } + out := output.String() + idx := strings.Index(out, "New feature") + if idx < 0 { + t.Fatalf("text %q not found in %q", "New feature", out) } + assert.Equal(t, idx, strings.LastIndex(out, "New feature")) } func TestPRList_filteringClosed(t *testing.T) { diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index dffd2b412..2c012d712 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -97,6 +97,12 @@ func statusRun(opts *StatusOptions) error { return err } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + out := opts.IO.Out fmt.Fprintln(out, "") diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index b653e5ffe..e8b5398b2 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -99,6 +99,12 @@ func viewRun(opts *ViewOptions) error { return utils.OpenInBrowser(openURL) } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + if connectedToTerminal { return printHumanPrPreview(opts.IO.Out, pr) } diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index adcf384a4..f5a70f54b 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -1,10 +1,13 @@ package view import ( + "errors" "fmt" "html/template" "net/http" + "os" "strings" + "syscall" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" @@ -107,6 +110,12 @@ func viewRun(opts *ViewOptions) error { return err } + err = opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() + stdout := opts.IO.Out if !opts.IO.IsStdoutTTY() { @@ -167,7 +176,10 @@ func viewRun(opts *ViewOptions) error { err = tmpl.Execute(stdout, repoData) if err != nil { - return err + var pathError *os.PathError + if !errors.As(err, &pathError) || pathError.Op != "write" || pathError.Unwrap() != syscall.EPIPE { + return err + } } return nil From 84b1b3643328af13b4ee1c5b8f97d2cc2e0033b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 7 Sep 2020 12:41:28 +0200 Subject: [PATCH 3/9] Simplify handling the "broken pipe" error --- pkg/cmd/repo/view/view.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index f5a70f54b..58eecbc01 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -5,7 +5,6 @@ import ( "fmt" "html/template" "net/http" - "os" "strings" "syscall" @@ -175,11 +174,8 @@ func viewRun(opts *ViewOptions) error { } err = tmpl.Execute(stdout, repoData) - if err != nil { - var pathError *os.PathError - if !errors.As(err, &pathError) || pathError.Op != "write" || pathError.Unwrap() != syscall.EPIPE { - return err - } + if err != nil && !errors.Is(err, syscall.EPIPE) { + return err } return nil From 6ad6784c4654d661a421ff6323cf641397eb73fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 14 Sep 2020 15:30:10 +0200 Subject: [PATCH 4/9] Ignore EPIPE when forwarding `pr diff` output to pager --- pkg/cmd/pr/diff/diff.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index 3c55e7db8..2bea8072b 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "strings" + "syscall" "github.com/cli/cli/api" "github.com/cli/cli/context" @@ -98,6 +99,9 @@ func diffRun(opts *DiffOptions) error { if opts.UseColor == "never" { _, err = io.Copy(opts.IO.Out, diff) + if errors.Is(err, syscall.EPIPE) { + return nil + } return err } From 76181156ba1fd6ae8eb4b17dc933e7d0b53bcad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 14 Sep 2020 16:49:30 +0200 Subject: [PATCH 5/9] Prevent endless recursive pager Unset PAGER environment variable when executing the command referenced in PAGER in case that command also has support for PAGER and would end up executing itself indefinitely. --- pkg/iostreams/iostreams.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 9de8ab8d7..e3694dfe5 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -103,6 +103,11 @@ func (s *IOStreams) StartPager() error { } pagerEnv := os.Environ() + for i := len(pagerEnv) - 1; i >= 0; i-- { + if strings.HasPrefix(pagerEnv[i], "PAGER=") { + pagerEnv = append(pagerEnv[0:i], pagerEnv[i+1:]...) + } + } if _, ok := os.LookupEnv("LESS"); !ok { pagerEnv = append(pagerEnv, "LESS=FRX") } From 50291aa6de313486cc498f7f6094192c41314703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 15 Sep 2020 11:43:49 +0200 Subject: [PATCH 6/9] Add PAGER support to the `api` command --- pkg/cmd/api/api.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index f051883a8..9ec4de435 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -13,6 +13,7 @@ import ( "sort" "strconv" "strings" + "syscall" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/internal/ghinstance" @@ -196,6 +197,12 @@ func apiRun(opts *ApiOptions) error { headersOutputStream := opts.IO.Out if opts.Silent { opts.IO.Out = ioutil.Discard + } else { + err := opts.IO.StartPager() + if err != nil { + return err + } + defer opts.IO.StopPager() } host := ghinstance.OverridableDefault() @@ -265,12 +272,13 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream if isJSON && opts.IO.ColorEnabled() { err = jsoncolor.Write(opts.IO.Out, responseBody, " ") - if err != nil { - return - } } else { _, err = io.Copy(opts.IO.Out, responseBody) - if err != nil { + } + if err != nil { + if errors.Is(err, syscall.EPIPE) { + err = nil + } else { return } } From c643778701240f7ab56b598cf1916419721b8f0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 16 Sep 2020 16:10:53 +0200 Subject: [PATCH 7/9] Have only one test assert default config rendering --- internal/config/config_file_test.go | 4 ++-- internal/config/config_type_test.go | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 1eb97ab10..f40cb9097 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -110,14 +110,14 @@ github.com: _, err := ParseConfig("config.yml") assert.Nil(t, err) - expectedMain := "# What protocol to use when performing git operations. Supported values: ssh, https\ngit_protocol: https\n# What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment.\neditor:\n# When to interactively prompt. This is a global config that cannot be overriden by hostname. Supported values: enabled, disabled\nprompt: enabled\n# Aliases allow you to create nicknames for gh commands\naliases:\n co: pr checkout\n" expectedHosts := `github.com: user: keiyuri oauth_token: "123456" ` - assert.Equal(t, expectedMain, mainBuf.String()) assert.Equal(t, expectedHosts, hostsBuf.String()) + assert.NotContains(t, mainBuf.String(), "github.com") + assert.NotContains(t, mainBuf.String(), "oauth_token") } func Test_parseConfigFile(t *testing.T) { diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index 6e561f634..1e1122c78 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -4,6 +4,7 @@ import ( "bytes" "testing" + "github.com/MakeNowJust/heredoc" "github.com/stretchr/testify/assert" ) @@ -19,8 +20,8 @@ func Test_fileConfig_Set(t *testing.T) { assert.NoError(t, c.Set("github.com", "user", "hubot")) assert.NoError(t, c.Write()) - expected := "# What protocol to use when performing git operations. Supported values: ssh, https\ngit_protocol: https\n# What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment.\neditor: nano\n# When to interactively prompt. This is a global config that cannot be overriden by hostname. Supported values: enabled, disabled\nprompt: enabled\n# Aliases allow you to create nicknames for gh commands\naliases:\n co: pr checkout\n" - assert.Equal(t, expected, mainBuf.String()) + assert.Contains(t, mainBuf.String(), "editor: nano") + assert.Contains(t, mainBuf.String(), "git_protocol: https") assert.Equal(t, `github.com: git_protocol: ssh user: hubot @@ -37,7 +38,17 @@ func Test_defaultConfig(t *testing.T) { cfg := NewBlankConfig() assert.NoError(t, cfg.Write()) - expected := "# What protocol to use when performing git operations. Supported values: ssh, https\ngit_protocol: https\n# What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment.\neditor:\n# When to interactively prompt. This is a global config that cannot be overriden by hostname. Supported values: enabled, disabled\nprompt: enabled\n# Aliases allow you to create nicknames for gh commands\naliases:\n co: pr checkout\n" + expected := heredoc.Doc(` + # What protocol to use when performing git operations. Supported values: ssh, https + git_protocol: https + # What editor gh should run when creating issues, pull requests, etc. If blank, will refer to environment. + editor: + # When to interactively prompt. This is a global config that cannot be overriden by hostname. Supported values: enabled, disabled + prompt: enabled + # Aliases allow you to create nicknames for gh commands + aliases: + co: pr checkout + `) assert.Equal(t, expected, mainBuf.String()) assert.Equal(t, "", hostsBuf.String()) From f6dd1bcd0af163eff61ca7002965e9f0126b5def Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 16 Sep 2020 16:15:37 +0200 Subject: [PATCH 8/9] Add the `pager` config option --- cmd/gh/main.go | 8 +++++--- internal/config/config_type.go | 9 +++++++++ internal/config/config_type_test.go | 2 ++ pkg/iostreams/iostreams.go | 4 ++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index fdde7acd3..73b153ffa 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -51,12 +51,14 @@ func main() { os.Exit(2) } - prompt, _ := cfg.Get("", "prompt") - - if prompt == config.PromptsDisabled { + if prompt, _ := cfg.Get("", "prompt"); prompt == config.PromptsDisabled { cmdFactory.IOStreams.SetNeverPrompt(true) } + if pager, _ := cfg.Get("", "pager"); pager != "" { + cmdFactory.IOStreams.SetPager(pager) + } + expandedArgs := []string{} if len(os.Args) > 0 { expandedArgs = os.Args[1:] diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 740955539..2ff23e58c 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -180,6 +180,15 @@ func NewBlankRoot() *yaml.Node { Kind: yaml.ScalarNode, Value: PromptsEnabled, }, + { + HeadComment: "A pager program to send command output to. Example value: less", + Kind: yaml.ScalarNode, + Value: "pager", + }, + { + Kind: yaml.ScalarNode, + Value: "", + }, { HeadComment: "Aliases allow you to create nicknames for gh commands", Kind: yaml.ScalarNode, diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index 1e1122c78..50e2b9f5a 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -45,6 +45,8 @@ func Test_defaultConfig(t *testing.T) { editor: # When to interactively prompt. This is a global config that cannot be overriden by hostname. Supported values: enabled, disabled prompt: enabled + # A pager program to send command output to. Example value: less + pager: # Aliases allow you to create nicknames for gh commands aliases: co: pr checkout diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index e3694dfe5..d6c8ae48f 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -92,6 +92,10 @@ func (s *IOStreams) IsStderrTTY() bool { return false } +func (s *IOStreams) SetPager(cmd string) { + s.pagerCommand = cmd +} + func (s *IOStreams) StartPager() error { if s.pagerCommand == "" || !s.IsStdoutTTY() { return nil From fa2513dea6230b6eb244b2c95029db51012a9c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 16 Sep 2020 16:15:57 +0200 Subject: [PATCH 9/9] Document PAGER environment variable --- pkg/cmd/root/help_topic.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 1357825a2..a1acf5a40 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -28,6 +28,8 @@ func NewHelpTopic(topic string) *cobra.Command { DEBUG: set to any value to enable verbose output to standard error. Include values "api" or "oauth" to print detailed information about HTTP requests or authentication flow. + PAGER: a paging program to send standard output to, e.g. "less". + GLAMOUR_STYLE: the style to use for rendering Markdown. See https://github.com/charmbracelet/glamour#styles