diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index 72ade5110..eaa762ed6 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/pkg/commands/oscommands/pty_windows.go @@ -36,7 +36,16 @@ func (p *winPty) Resize(cols, rows uint16) error { // there is nothing left to resize. return nil } - return windows.ResizePseudoConsole(p.hpc, windows.Coord{X: int16(cols), Y: int16(rows)}) + return windows.ResizePseudoConsole(p.hpc, clampPtySize(cols, rows)) +} + +// clampPtySize clamps a requested pty size to the minimum that ConPTY +// accepts: CreatePseudoConsole and ResizePseudoConsole reject zero +// dimensions with E_INVALIDARG, but callers legitimately request them — the +// pty is sized after the main view, which is zero-sized while hidden, e.g. +// in full-screen mode with a side panel focused. +func clampPtySize(cols, rows uint16) windows.Coord { + return windows.Coord{X: int16(max(cols, 1)), Y: int16(max(rows, 1))} } // closeHpc closes the pseudoconsole exactly once. Safe to call from multiple @@ -140,7 +149,7 @@ func StartPty(cmd *exec.Cmd, cols, rows uint16) (sp StartedPty, err error) { // CreatePseudoConsole dupes the handles it needs internally; we release // our references to the child-side ends immediately after. var hpc windows.Handle - size := windows.Coord{X: int16(cols), Y: int16(rows)} + size := clampPtySize(cols, rows) if err = windows.CreatePseudoConsole(size, inRead, outWrite, 0, &hpc); err != nil { _ = windows.CloseHandle(inRead) _ = windows.CloseHandle(outWrite) diff --git a/pkg/commands/oscommands/pty_windows_test.go b/pkg/commands/oscommands/pty_windows_test.go new file mode 100644 index 000000000..0b4173561 --- /dev/null +++ b/pkg/commands/oscommands/pty_windows_test.go @@ -0,0 +1,25 @@ +package oscommands + +import ( + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" +) + +// The requested size can legitimately be zero: the pty inherits the main +// view's dimensions, and that view is zero-sized while hidden, e.g. in +// full-screen mode with a side panel focused. +func TestStartPtyWithZeroSize(t *testing.T) { + // The command deliberately produces no output: go test runs with + // redirected std handles, which CreateProcess duplicates into the child + // in place of handles to the attached pseudoconsole, so command output + // would bypass the pty and pollute the test log. + sp, err := StartPty(exec.Command("cmd", "/c", "exit 0"), 0, 0) + assert.NoError(t, err) + + if err == nil { + _ = sp.Wait() + _ = sp.Pty.Close() + } +} diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index dbf968048..e10ea8ec7 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -100,6 +100,7 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cols, rows := gui.desiredPtySize(view) var p oscommands.Pty + var fallbackPipe io.ReadCloser start := func() (tasks.Cmd, io.Reader) { // The pty (and pager) wrap to this width; apply it here, on the // task's goroutine once the previous task has stopped, so it doesn't @@ -109,7 +110,11 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error sp, err := oscommands.StartPty(cmd, cols, rows) if err != nil { gui.c.Log.Error(err) - return tasks.ExecCmd{Cmd: cmd}, nil + // Fall back to running the command without a pty: the pager is + // lost, but the command's output still renders. + execCmd, pipe := startCmdWithPipe(cmd, gui.c.Log) + fallbackPipe = pipe + return execCmd, pipe } p = sp.Pty @@ -125,6 +130,10 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error if p != nil { p.Close() } + if fallbackPipe != nil { + fallbackPipe.Close() + fallbackPipe = nil + } delete(gui.viewPtmxMap, view.Name()) gui.Mutexes.PtyMutex.Unlock() } diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 27aacf58b..3dce93874 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -7,6 +7,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/tasks" + "github.com/sirupsen/logrus" ) func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error { @@ -29,19 +30,9 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error start := func() (tasks.Cmd, io.Reader) { view.SetContentWidth(contentWidth) - var err error - r, err = cmd.StdoutPipe() - if err != nil { - gui.c.Log.Error(err) - r = nil - } - cmd.Stderr = cmd.Stdout - - if err := cmd.Start(); err != nil { - gui.c.Log.Error(err) - } - - return tasks.ExecCmd{Cmd: cmd}, r + execCmd, pipe := startCmdWithPipe(cmd, gui.c.Log) + r = pipe + return execCmd, pipe } onClose := func() { @@ -59,6 +50,27 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error return nil } +// startCmdWithPipe starts cmd with its stdout and stderr going to a single +// pipe, and returns the command along with the pipe's read end, in the shape +// that NewCmdTask expects from its start func. It never returns a nil reader, +// because NewCmdTask's scanner panics on one: when the pipe can't be created +// the command isn't started at all, and an empty reader is returned so that +// the task shuts down cleanly with the error in the log. +func startCmdWithPipe(cmd *exec.Cmd, log *logrus.Entry) (tasks.Cmd, io.ReadCloser) { + r, err := cmd.StdoutPipe() + if err != nil { + log.Error(err) + return tasks.ExecCmd{Cmd: cmd}, io.NopCloser(strings.NewReader("")) + } + cmd.Stderr = cmd.Stdout + + if err := cmd.Start(); err != nil { + log.Error(err) + } + + return tasks.ExecCmd{Cmd: cmd}, r +} + func (gui *Gui) newStringTask(view *gocui.View, str string) error { // using str so that if rendering the exact same thing we don't reset the origin return gui.newStringTaskWithKey(view, str, str) diff --git a/pkg/gui/tasks_adapter_test.go b/pkg/gui/tasks_adapter_test.go new file mode 100644 index 000000000..48c1bb45f --- /dev/null +++ b/pkg/gui/tasks_adapter_test.go @@ -0,0 +1,24 @@ +package gui + +import ( + "bytes" + "os/exec" + "testing" + + "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stretchr/testify/assert" +) + +func TestStartCmdWithPipeWhenPipeCannotBeCreated(t *testing.T) { + cmd := exec.Command("non-existent-command") + // Assigning stdout up front makes cmd.StdoutPipe fail. This happens in + // practice on the Unix pty fallback path: a failed pty start can leave + // the tty assigned to the command's stdout. + cmd.Stdout = &bytes.Buffer{} + + _, r := startCmdWithPipe(cmd, utils.NewDummyLog()) + + // NewCmdTask's scanner panics on a nil reader, so startCmdWithPipe must + // not return one even when it can't create the pipe. + assert.NotNil(t, r) +}