Merge pull request cli#1630 from cli/pager-for-everyone

mislav · web-flow · commit 3049db646fe7 · 2020-09-16T16:22:58.000+02:00
Extend PAGER support to all commands producing significant output
diff --git 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_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.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
@@ -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,19 @@ 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
+		# 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
+	`)
 	assert.Equal(t, expected, mainBuf.String())
 	assert.Equal(t, "", hostsBuf.String())
 
diff --git 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
 		}
 	}
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -88,10 +88,16 @@ func viewRun(opts *ViewOptions) error {
 		}
 		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/diff/diff.go b/pkg/cmd/pr/diff/diff.go
@@ -6,17 +6,15 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"os"
-	"os/exec"
 	"strings"
+	"syscall"
 
 	"github.com/cli/cli/api"
 	"github.com/cli/cli/context"
 	"github.com/cli/cli/internal/ghrepo"
 	"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,15 +91,18 @@ func diffRun(opts *DiffOptions) error {
 	}
 	defer diff.Close()
 
-	if opts.UseColor == "never" {
-		_, err = io.Copy(opts.IO.Out, diff)
+	err = opts.IO.StartPager()
+	if err != nil {
 		return err
 	}
+	defer opts.IO.StopPager()
 
-	if opts.IO.IsStdoutTTY() {
-		if pager := os.Getenv("PAGER"); pager != "" {
-			return runPager(pager, diff, opts.IO.Out)
+	if opts.UseColor == "never" {
+		_, err = io.Copy(opts.IO.Out, diff)
+		if errors.Is(err, syscall.EPIPE) {
+			return nil
 		}
+		return err
 	}
 
 	diffLines := bufio.NewScanner(diff)
@@ -148,14 +149,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
@@ -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/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
diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go
diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go
diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go
diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go
diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go

Original file line number	Diff line number	Diff line change
`@@ -110,14 +110,14 @@ github.com:`
`110`	`110`	`_, err := ParseConfig("config.yml")`
`111`	`111`	`assert.Nil(t, err)`
`112`	`112`
`113`		`- 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"`
`114`	`113`	expectedHosts := `github.com:
`115`	`114`	`user: keiyuri`
`116`	`115`	`oauth_token: "123456"`
`117`	`116`	`
`118`	`117`
`119`		`- assert.Equal(t, expectedMain, mainBuf.String())`
`120`	`118`	`assert.Equal(t, expectedHosts, hostsBuf.String())`
	`119`	`+ assert.NotContains(t, mainBuf.String(), "github.com")`
	`120`	`+ assert.NotContains(t, mainBuf.String(), "oauth_token")`
`121`	`121`	`}`
`122`	`122`
`123`	`123`	`func Test_parseConfigFile(t *testing.T) {`
Original file line number	Diff line number	Diff line change
`@@ -88,10 +88,16 @@ func viewRun(opts *ViewOptions) error {`
`88`	`88`	`}`
`89`	`89`	`return utils.OpenInBrowser(openURL)`
`90`	`90`	`}`
	`91`	`+`
	`92`	`+ err = opts.IO.StartPager()`
	`93`	`+ if err != nil {`
	`94`	`+ return err`
	`95`	`+ }`
	`96`	`+ defer opts.IO.StopPager()`
	`97`	`+`
`91`	`98`	`if opts.IO.IsStdoutTTY() {`
`92`	`99`	`return printHumanIssuePreview(opts.IO.Out, issue)`
`93`	`100`	`}`
`94`		`-`
`95`	`101`	`return printRawIssuePreview(opts.IO.Out, issue)`
`96`	`102`	`}`
`97`	`103`