Fix Windows crash when switching to fullscreen mode with a custom pager (#5838)

Fix a crash when typing `+` twice to go to full screen mode, when a
custom pager such as delta or difftastic is configured.

Fixes #5837.

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)

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()
+	}
+}

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()
 		}

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)

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)
+}