diff --git a/pkg/cmd/cmdutil.go b/pkg/cmd/cmdutil.go index aee3b10..285df03 100644 --- a/pkg/cmd/cmdutil.go +++ b/pkg/cmd/cmdutil.go @@ -130,6 +130,24 @@ func streamOutput(label string, generateOutput func(w *os.File) error) error { return streamOutputOSSpecific(label, generateOutput) } +// pagerCommand splits PAGER into an executable and its arguments. $PAGER is written +// as a command line by the tools this CLI is used beside (git, gh, man), so +// "less -R" has to reach less rather than be resolved as a single executable name. +// When the first word does not resolve, the whole value is kept, so a configuration +// that works today keeps working, including an executable path that contains spaces. +func pagerCommand() []string { + pager := strings.TrimSpace(os.Getenv("PAGER")) + if pager == "" { + return []string{"less"} + } + if command := strings.Fields(pager); len(command) > 1 { + if _, err := exec.LookPath(command[0]); err == nil { + return command + } + } + return []string{pager} +} + func streamToPagerWithPipe(label string, generateOutput func(w *os.File) error) error { r, w, err := os.Pipe() if err != nil { @@ -138,16 +156,12 @@ func streamToPagerWithPipe(label string, generateOutput func(w *os.File) error) defer r.Close() defer w.Close() - pagerProgram := os.Getenv("PAGER") - if pagerProgram == "" { - pagerProgram = "less" - } - - if _, err := exec.LookPath(pagerProgram); err != nil { + command := pagerCommand() + if _, err := exec.LookPath(command[0]); err != nil { return err } - cmd := exec.Command(pagerProgram) + cmd := exec.Command(command[0], command[1:]...) cmd.Stdin = r cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/pkg/cmd/cmdutil_pager_unix_test.go b/pkg/cmd/cmdutil_pager_unix_test.go new file mode 100644 index 0000000..614c423 --- /dev/null +++ b/pkg/cmd/cmdutil_pager_unix_test.go @@ -0,0 +1,83 @@ +//go:build !windows + +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// pagerCommand must split a command line while keeping the configurations that work +// today: an unset PAGER, a bare program, and an executable path containing spaces. +func TestPagerCommand(t *testing.T) { + tempDir := t.TempDir() + pagerPath := filepath.Join(tempDir, "pager") + require.NoError(t, os.WriteFile(pagerPath, []byte("#!/bin/sh\n"), 0700)) + + spacedDir := filepath.Join(tempDir, "bin dir") + require.NoError(t, os.Mkdir(spacedDir, 0700)) + spacedPath := filepath.Join(spacedDir, "pager") + require.NoError(t, os.WriteFile(spacedPath, []byte("#!/bin/sh\n"), 0700)) + + missingPath := filepath.Join(tempDir, "missing") + + for _, tc := range []struct { + name string + pager string + want []string + }{ + {name: "unset falls back to less", pager: "", want: []string{"less"}}, + {name: "blank falls back to less", pager: " \t ", want: []string{"less"}}, + {name: "bare program", pager: pagerPath, want: []string{pagerPath}}, + {name: "bare program is trimmed", pager: " " + pagerPath + " ", want: []string{pagerPath}}, + {name: "arguments are split off", pager: pagerPath + " -R --no-init", want: []string{pagerPath, "-R", "--no-init"}}, + {name: "trimmed value with arguments", pager: " " + pagerPath + " -R", want: []string{pagerPath, "-R"}}, + {name: "path with spaces stays one program", pager: spacedPath, want: []string{spacedPath}}, + {name: "unresolvable first word keeps the whole value", pager: missingPath + " -R", want: []string{missingPath + " -R"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("PAGER", tc.pager) + require.Equal(t, tc.want, pagerCommand()) + }) + } +} + +// PAGER is written as a command line by the tools this CLI is used beside +// (PAGER="less -R" is common), so both pager implementations have to pass the +// arguments through instead of resolving the whole value as one executable name. +func TestStreamToPagerPassesPagerArguments(t *testing.T) { + paths := map[string]func(string, func(*os.File) error) error{ + "pipe": streamToPagerWithPipe, + "socket": streamOutputOSSpecific, + } + for name, stream := range paths { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + argPath, outputPath := filepath.Join(dir, "arg"), filepath.Join(dir, "output") + pager := filepath.Join(dir, "pager") + require.NoError(t, os.WriteFile(pager, []byte("#!/bin/sh\nprintf '%s\\n' \"$1\" > \"$OPENAI_TEST_PAGER_ARG\"\ncat > \"$OPENAI_TEST_PAGER_OUTPUT\"\n"), 0700)) + t.Setenv("PAGER", pager+" --no-init") + t.Setenv("OPENAI_TEST_PAGER_ARG", argPath) + t.Setenv("OPENAI_TEST_PAGER_OUTPUT", outputPath) + + require.NoError(t, stream("pager arguments", func(w *os.File) error { + if name == "socket" { + require.Equal(t, "parent-socket", w.Name(), "socket pager must not fall back to the pipe") + } + _, err := w.WriteString("payload\n") + return err + })) + + arg, err := os.ReadFile(argPath) + require.NoError(t, err) + require.Equal(t, "--no-init\n", string(arg)) + + output, err := os.ReadFile(outputPath) + require.NoError(t, err) + require.Equal(t, "payload\n", string(output)) + }) + } +} diff --git a/pkg/cmd/cmdutil_unix.go b/pkg/cmd/cmdutil_unix.go index 4902563..26a0fbb 100644 --- a/pkg/cmd/cmdutil_unix.go +++ b/pkg/cmd/cmdutil_unix.go @@ -90,12 +90,9 @@ func openSocketPairPager(label string) (*os.File, int, error) { parentConn := os.NewFile(uintptr(parentFd), "parent-socket") - pagerProgram := os.Getenv("PAGER") - if pagerProgram == "" { - pagerProgram = "less" - } + command := pagerCommand() - pagerPath, err := exec.LookPath(pagerProgram) + pagerPath, err := exec.LookPath(command[0]) if err != nil { unix.Close(parentFd) return nil, 0, err @@ -115,7 +112,7 @@ func openSocketPairPager(label string) (*os.File, int, error) { }, } - pid, err := syscall.ForkExec(pagerPath, []string{pagerProgram}, procAttr) + pid, err := syscall.ForkExec(pagerPath, command, procAttr) if err != nil { unix.Close(parentFd) return nil, 0, err