Re-render the focused main view if its content changes while it is being searched (#5993)

When searching the focused main view using `/`, any updates to its
content were ignored because back when we introduced the focused main
view feature we couldn't make it work; search mode couldn't cope well
with the view content changing under it.

In this PR we make that work, and remove the limitation. Along the way
we fix a bunch of other related problems; some are only theoretical race
conditions that have been found by reading the code, but never observed
in reality; some are real problems that are too edge-casey to describe
in detail. See the individual commit messages for details.

AGENTS.md

@@ -82,6 +82,17 @@ while still being meaningful and self-contained.
   excuse bundling it in. Before committing, review your diff and split out any
   hunk that is behavior-preserving (an extraction, a rename, a move) into a
   preceding commit, by staging hunks or resetting and recommitting in order.
+- **A preparatory refactor is a new commit only when it prepares something
+  new.** Before adding one, find the commit that introduced the code you are
+  about to restructure. If that commit is on this branch, the refactor is a
+  `fixup!` for it rather than a commit of its own: a branch must never contain
+  a commit whose code a later commit on the same branch tidies up. A prep
+  refactor earns a commit of its own only when the shape it corrects came from
+  before the branch. This holds across a branch stack too — if the commit that
+  introduced the code is in an earlier branch of the stack, the fixup belongs
+  there, and the branches above it get replayed. The one exception is when
+  fixing it there turns out to be unreasonably difficult; ask me what to do
+  rather than deciding to leave the repair at the tip.
 - **Do not use conventional commits** (no `feat:`/`fix:`/`chore:` prefixes).
   Match the plain English imperative style of the existing history.
 - **Wrap message body to 72 characters**. The subject is allowed to go up to 80

pkg/gocui/search_test.go

@@ -0,0 +1,58 @@
+package gocui
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// writeLines writes the given lines to the view, as a task rendering content into it
+// does: one line at a time.
+func writeLines(v *View, lines ...string) {
+	for _, line := range lines {
+		fmt.Fprintf(v, "%s\n", line)
+	}
+}
+
+func TestSearchStatusAfterTheMatchesChange(t *testing.T) {
+	v := NewView("name", 0, 0, 40, 10, OutputNormal)
+	writeLines(v, "match", "other", "match", "other", "match")
+
+	v.Search("match", nil)
+	_ = v.gotoNextMatch()
+	_ = v.gotoNextMatch()
+	index, total := v.GetSearchStatus()
+	assert.Equal(t, 2, index)
+	assert.Equal(t, 3, total)
+
+	// The content is re-rendered with only the first of those matches left in it.
+	v.Clear()
+	writeLines(v, "match", "other", "other")
+
+	index, total = v.GetSearchStatus()
+	assert.Equal(t, 0, index)
+	assert.Equal(t, 1, total)
+}
+
+func TestSearchPositionsFollowStreamedContent(t *testing.T) {
+	v := NewView("name", 0, 0, 40, 10, OutputNormal)
+	v.Search("match", nil)
+
+	// A render arrives a line at a time, and the status describes all of it.
+	writeLines(v, "other", "match", "other", "match")
+
+	_, total := v.GetSearchStatus()
+	assert.Equal(t, 2, total)
+}
+
+func BenchmarkWriteToSearchedView(b *testing.B) {
+	for b.Loop() {
+		v := NewView("name", 0, 0, 100, 40, OutputNormal)
+		v.Search("match", nil)
+		for i := range 2000 {
+			fmt.Fprintf(v, "line %d of a diff, most of which does not match\n", i)
+		}
+		v.GetSearchStatus()
+	}
+}

pkg/gocui/view.go

@@ -271,6 +271,12 @@ type searcher struct {
 	currentSearchIndex int
 	onSelectItem       func(*View, int)
 	renderSearchStatus func(*View, int, int)
+
+	// Whether the content has changed since the positions were worked out, so that
+	// they have to be worked out again before they are read. Working them out walks
+	// the whole view, and content arrives a line at a time, so it happens once per
+	// read rather than once per line written.
+	positionsStale bool
 }
 
 func (v *View) setRenderSearchStatus(renderSearchStatus func(*View, int, int)) {
@@ -287,7 +293,40 @@ func (v *View) renderSearchStatus(index int, itemCount int) {
 	}
 }
 
+// refreshSearchPositions works the search positions out again if the content has
+// changed since they were last worked out. Every read of the positions goes through
+// this, so that no caller has to know whether the view has been drawn since the
+// content it is asking about arrived.
+func (v *View) refreshSearchPositions() {
+	v.writeMutex.Lock()
+	defer v.writeMutex.Unlock()
+
+	v.refreshSearchPositionsIfNeeded()
+}
+
+// refreshSearchPositions for a caller that already holds writeMutex.
+func (v *View) refreshSearchPositionsIfNeeded() {
+	if v.searcher.positionsStale {
+		v.updateSearchPositions()
+	}
+}
+
+// RefreshSearch runs the search again over content the view has just been re-rendered
+// with, and shows the "x of y" status of what it finds. The view stays where it is: the
+// position in the content is the user's, and the search follows it rather than moving
+// it.
+func (v *View) RefreshSearch() {
+	if !v.IsSearching() {
+		return
+	}
+
+	v.UpdateSearchResults(v.searcher.searchString, v.searcher.modelSearchResults)
+	v.renderSearchStatus(v.searcher.currentSearchIndex, len(v.searcher.searchPositions))
+}
+
 func (v *View) gotoNextMatch() error {
+	v.refreshSearchPositions()
+
 	if len(v.searcher.searchPositions) == 0 {
 		return nil
 	}
@@ -307,6 +346,8 @@ func (v *View) gotoNextMatch() error {
 }
 
 func (v *View) gotoPreviousMatch() error {
+	v.refreshSearchPositions()
+
 	if len(v.searcher.searchPositions) == 0 {
 		return nil
 	}
@@ -328,6 +369,8 @@ func (v *View) gotoPreviousMatch() error {
 }
 
 func (v *View) SelectSearchResult(index int) {
+	v.refreshSearchPositions()
+
 	itemCount := len(v.searcher.searchPositions)
 	if itemCount == 0 {
 		return
@@ -347,6 +390,8 @@ func (v *View) SelectSearchResult(index int) {
 
 // Returns <current match index>, <total matches>
 func (v *View) GetSearchStatus() (int, int) {
+	v.refreshSearchPositions()
+
 	return v.searcher.currentSearchIndex, len(v.searcher.searchPositions)
 }
 
@@ -420,6 +465,8 @@ func (v *View) nearestSearchPosition() int {
 }
 
 func (v *View) SetNearestSearchPosition() {
+	v.refreshSearchPositions()
+
 	if len(v.searcher.searchPositions) > 0 {
 		newPos := v.nearestSearchPosition()
 		if newPos != v.searcher.currentSearchIndex {
@@ -902,7 +949,7 @@ func (v *View) write(p []byte) {
 
 	v.buf.write(v, p)
 
-	v.updateSearchPositions()
+	v.searcher.positionsStale = true
 }
 
 // write parses p into cells and appends them to the buffer at its write cursor.
@@ -1353,6 +1400,8 @@ func stringToGraphemes(s string) []string {
 }
 
 func (v *View) updateSearchPositions() {
+	v.searcher.positionsStale = false
+
 	if v.searcher.searchString != "" {
 		var normalizeRune func(s string) string
 		var normalizedSearchStr string
@@ -1431,6 +1480,11 @@ func (v *View) updateSearchPositions() {
 			}
 		}
 	}
+
+	// The content may hold fewer matches than it did, so the current one is brought
+	// back into range: readers index the positions by it.
+	v.searcher.currentSearchIndex = min(v.searcher.currentSearchIndex,
+		max(0, len(v.searcher.searchPositions)-1))
 }
 
 // IsTainted tells us if the view is tainted
@@ -1461,6 +1515,7 @@ func (v *View) draw(isWindowFocused bool) {
 	}
 
 	v.refreshViewLinesIfNeeded()
+	v.refreshSearchPositionsIfNeeded()
 
 	visibleViewLinesHeight := v.viewLineLengthIgnoringTrailingBlankLines()
 	if v.Autoscroll && visibleViewLinesHeight > maxY {
@@ -2070,6 +2125,8 @@ func (v *View) setContentLineCount(lineCount int) {
 // result that is visible in the view, if any, or the first one that is below the view if none is
 // visible.
 func (v *View) selectVisibleSearchResultAfterScrollUp() {
+	v.refreshSearchPositions()
+
 	if !v.Highlight && len(v.searcher.searchPositions) != 0 {
 		windowBottom := v.oy + v.InnerHeight()
 		if v.searcher.searchPositions[v.searcher.currentSearchIndex].Y >= windowBottom {
@@ -2093,6 +2150,8 @@ func (v *View) selectVisibleSearchResultAfterScrollUp() {
 // result that is visible in the view, if any, or the last one that is above the view if none is
 // visible.
 func (v *View) selectVisibleSearchResultAfterScrollDown() {
+	v.refreshSearchPositions()
+
 	if !v.Highlight && len(v.searcher.searchPositions) != 0 {
 		if v.searcher.searchPositions[v.searcher.currentSearchIndex].Y < v.oy {
 			newSearchIndex := v.searcher.currentSearchIndex

pkg/gui/main_panels.go

@@ -2,6 +2,7 @@ package gui
 
 import (
 	"github.com/jesseduffield/lazygit/pkg/gocui"
+	"github.com/jesseduffield/lazygit/pkg/gui/context"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
 )
 
@@ -139,3 +140,17 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) {
 func (gui *Gui) splitMainPanel(splitMainPanel bool) {
 	gui.State.SplitMainPanel = splitMainPanel
 }
+
+// reApplySearch runs a search the view holds again over the content a render has just
+// finished putting there, so that the matches highlighted and the "x of y" status
+// describe what the view shows now rather than what it showed when the search was
+// typed. Call it once the content is final.
+func (gui *Gui) reApplySearch(view *gocui.View) {
+	// While the prompt is open, the search view holds what the user is typing, and the
+	// status would be written over it.
+	if gui.State.ContextMgr.Current().GetKey() == context.SEARCH_CONTEXT_KEY {
+		return
+	}
+
+	view.RefreshSearch()
+}

pkg/gui/tasks_adapter.go

@@ -91,6 +91,7 @@ func (gui *Gui) newStringTaskWithoutScroll(view *gocui.View, str string) error {
 	f := func(tasks.TaskOpts) error {
 		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.SetViewContent(view, str)
+			gui.reApplySearch(view)
 		})
 	}
 
@@ -108,6 +109,7 @@ func (gui *Gui) newStringTaskWithScroll(view *gocui.View, str string, originX in
 		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.SetViewContent(view, str)
 			view.SetOrigin(originX, originY)
+			gui.reApplySearch(view)
 		})
 	}
 
@@ -125,6 +127,7 @@ func (gui *Gui) newStringTaskWithKey(view *gocui.View, str string, key string) e
 		return gui.g.OnUIThreadAndWaitBackground(func() {
 			gui.c.ResetViewOrigin(view)
 			gui.c.SetViewContent(view, str)
+			gui.reApplySearch(view)
 		})
 	}
 
@@ -170,6 +173,8 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager {
 
 					view.SetOrigin(0, newOriginY)
 				}
+
+				gui.reApplySearch(view)
 			},
 			func() {
 				view.SetOrigin(0, 0)

pkg/gui/view_helpers.go

@@ -25,6 +25,18 @@ func (gui *Gui) linesToReadFromCmdTask(v *gocui.View) tasks.LinesToRead {
 
 	linesForFirstRefresh := height + oy + 10
 
+	// A search counts the matches in everything the view holds, so a re-render of a
+	// view that is being searched is read all the way to the end (as opening the
+	// search prompt reads it, see MainViewController.openSearch). Lines left unread
+	// hold matches the search doesn't know about, and would add themselves to the
+	// "x of y" as the user scrolled far enough to load them.
+	if v.IsSearching() {
+		return tasks.LinesToRead{
+			Total:               -1,
+			InitialRefreshAfter: linesForFirstRefresh,
+		}
+	}
+
 	// We want to read as many lines initially as necessary to let the
 	// scrollbar go to its minimum height, so that the scrollbar thumb doesn't
 	// change size as you scroll down.
@@ -159,15 +171,9 @@ func (gui *Gui) postRefreshUpdate(c types.Context, opts types.OnFocusOpts) {
 
 		currentCtx := gui.State.ContextMgr.Current()
 		if currentCtx.GetKey() == context.NORMAL_MAIN_CONTEXT_KEY || currentCtx.GetKey() == context.NORMAL_SECONDARY_CONTEXT_KEY {
-			// Searching can't cope well with the view being updated while it is being searched.
+			sidePanelContext := gui.State.ContextMgr.NextInStack(currentCtx)
-			// We might be able to fix the problems with this, but it doesn't seem easy, so for now
+			if sidePanelContext != nil && sidePanelContext.GetKey() == c.GetKey() {
-			// just don't rerender the view while searching, on the assumption that users will probably
+				sidePanelContext.HandleRenderToMain()
-			// either search or change their data, but not both at the same time.
-			if !currentCtx.GetView().IsSearching() {
-				sidePanelContext := gui.State.ContextMgr.NextInStack(currentCtx)
-				if sidePanelContext != nil && sidePanelContext.GetKey() == c.GetKey() {
-					sidePanelContext.HandleRenderToMain()
-				}
 			}
 		} else if c.GetKey() == gui.State.ContextMgr.CurrentStatic().GetKey() {
 			// If our view is not the current one, but it is the current static context, then this

pkg/integration/tests/filter_and_search/rerender_the_searched_main_view.go

@@ -0,0 +1,37 @@
+package filter_and_search
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var RerenderTheSearchedMainView = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "A refresh renders the focused main view again even while it is being searched",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "one\ntwo\nthree\n")
+		shell.Commit("one")
+
+		shell.UpdateFile("file1", "one\nNEEDLE\nthree\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().
+			IsFocused().
+			FilterOrSearch("NEEDLE").
+			Content(Contains("+NEEDLE")).
+			Tap(func() {
+				t.Shell().UpdateFile("file1", "one\nOTHER\nthree\n")
+			}).
+			Press(keys.Universal.Refresh).
+			Content(Contains("+OTHER")).
+			Content(DoesNotContain("+NEEDLE"))
+
+		t.Views().Search().Content(Contains("No matches for 'NEEDLE'"))
+	},
+})

pkg/integration/tests/filter_and_search/search_a_long_diff.go

@@ -0,0 +1,66 @@
+package filter_and_search
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+// longFileWithThreeMatches is long enough that a render of its diff stops well short
+// of the end, with two of the three matches for the search below the point it stops at.
+func longFileWithThreeMatches() string {
+	lines := make([]string, 0, 2000)
+	for i := range 2000 {
+		switch i {
+		case 100:
+			lines = append(lines, "NEEDLE first")
+		case 1000:
+			lines = append(lines, "NEEDLE middle")
+		case 1900:
+			lines = append(lines, "NEEDLE last")
+		default:
+			lines = append(lines, fmt.Sprintf("line %d", i))
+		}
+	}
+	return strings.Join(lines, "\n") + "\n"
+}
+
+var SearchALongDiff = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Search a diff that is longer than a single render of it reads",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	// A small window, so that a render stops well short of 2000 lines.
+	Width:       120,
+	Height:      30,
+	SetupConfig: func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "")
+		shell.Commit("one")
+
+		shell.UpdateFile("file1", longFileWithThreeMatches())
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Press(keys.Universal.FocusMainView)
+
+		// All three matches are counted: opening the prompt reads the whole diff
+		// first, however much of it the render had got to.
+		t.Views().Main().
+			IsFocused().
+			FilterOrSearch("NEEDLE")
+
+		t.Views().Search().Content(Contains("matches for 'NEEDLE' (1 of 3)"))
+
+		// Rendering the diff again reads it from the start, and it is read all the
+		// way down to the matches the search already knows about.
+		t.Views().Main().
+			Press(keys.Universal.IncreaseContextInDiffView).
+			Tap(func() {
+				t.ExpectToast(Equals("Changed diff context size to 4"))
+			}).
+			Content(Contains("+NEEDLE last"))
+	},
+})

pkg/integration/tests/filter_and_search/search_status_after_a_rerender.go

@@ -0,0 +1,45 @@
+package filter_and_search
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+var SearchStatusAfterARerender = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "The search status counts the matches in a diff that has been rendered again",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1",
+			"line 1\nline 2\nline 3\nline 4\nline 5\nline 6\nline 7\nline 8\nline 9\nNEEDLE\nline 11\nline 12\nline 13\nline 14\n")
+		shell.Commit("one")
+
+		// Four lines above NEEDLE, so that it is context at a context size of 4 but
+		// not at 3.
+		shell.UpdateFile("file1",
+			"line 1\nline 2\nline 3\nline 4\nline 5\nchanged\nline 7\nline 8\nline 9\nNEEDLE\nline 11\nline 12\nline 13\nline 14\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			Press(keys.Universal.FocusMainView)
+
+		t.Views().Main().
+			IsFocused().
+			Content(DoesNotContain("NEEDLE")).
+			FilterOrSearch("NEEDLE")
+
+		t.Views().Search().Content(Contains("No matches for 'NEEDLE'"))
+
+		// A wider context brings NEEDLE into the diff, and the search counts it.
+		t.Views().Main().
+			Press(keys.Universal.IncreaseContextInDiffView).
+			Tap(func() {
+				t.ExpectToast(Equals("Changed diff context size to 4"))
+			}).
+			Content(Contains("NEEDLE"))
+
+		t.Views().Search().Content(Contains("matches for 'NEEDLE' (1 of 1)"))
+	},
+})

pkg/integration/tests/test_list.go

@@ -277,6 +277,9 @@ var tests = []*components.IntegrationTest{
 	filter_and_search.NestedFilter,
 	filter_and_search.NestedFilterTransient,
 	filter_and_search.NewSearch,
+	filter_and_search.RerenderTheSearchedMainView,
+	filter_and_search.SearchALongDiff,
+	filter_and_search.SearchStatusAfterARerender,
 	filter_and_search.StageAllStagesOnlyTrackedFilesInTrackedOnlyFilter,
 	filter_and_search.StagingFolderStagesOnlyTrackedFilesInTrackedOnlyFilter,
 	filter_by_author.SelectAuthor,

pkg/tasks/read_request_queue.go

@@ -0,0 +1,101 @@
+package tasks
+
+import "sync"
+
+// readRequestQueue is an unbounded, order-preserving FIFO of the read requests a
+// view's running command task serves (see LinesToRead), with a reader that comes
+// and goes.
+//
+// It's unbounded, rather than a fixed-size channel, for the same reasons as the
+// user-event queue in gocui. Requests are handed over from the UI thread, where a
+// blocking send would deadlock against the task that is waiting to be let go, and
+// a fixed channel that fills up leaves only bad choices: blocking, dropping,
+// reordering, or panicking on overflow. Appending to a slice does none of those.
+//
+// The reader coming and going is the other half of what it's for. A request is
+// how a caller asks for content to be read and hears, through the request's Then,
+// that it has been; a request nobody answers leaves that caller waiting for good.
+// So asking whether a task is there and handing it the request are one step, and
+// so are taking the task away and handing back what it never answered. A request
+// made in between finds no task and goes back to its caller to answer.
+//
+// enqueue appends under the mutex and rings the doorbell; the task selects on the
+// doorbell to wake, then takes requests until there are none left. The doorbell is
+// buffered(1) and rung with a non-blocking send, so it's a coalescing "work
+// pending" flag rather than a per-request signal: a burst of appends leaves at
+// most one token, and the task takes everything the token stands for on a single
+// wake. A token left over after the queue empties causes one harmless empty wake.
+type readRequestQueue struct {
+	mutex    sync.Mutex
+	requests []LinesToRead
+	doorbell chan struct{}
+
+	// Whether a task is there to serve the requests. False before the first task
+	// starts, and between one task ending and the next starting.
+	serving bool
+}
+
+func newReadRequestQueue() *readRequestQueue {
+	return &readRequestQueue{doorbell: make(chan struct{}, 1)}
+}
+
+// beginServing says that a task is now there to serve the queue, and returns the
+// doorbell that tells it when there is something to serve.
+func (self *readRequestQueue) beginServing() <-chan struct{} {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	self.serving = true
+	return self.doorbell
+}
+
+// stopServing takes the task away and hands back the requests it never answered,
+// for the caller to answer in its place.
+func (self *readRequestQueue) stopServing() []LinesToRead {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	self.serving = false
+	unanswered := self.requests
+	self.requests = nil
+	return unanswered
+}
+
+// enqueue gives a request to the task serving the queue, and reports whether
+// there was one to give it to. When there wasn't, the request is the caller's to
+// answer.
+func (self *readRequestQueue) enqueue(request LinesToRead) bool {
+	self.mutex.Lock()
+	if !self.serving {
+		self.mutex.Unlock()
+		return false
+	}
+	self.requests = append(self.requests, request)
+	self.mutex.Unlock()
+
+	select {
+	case self.doorbell <- struct{}{}:
+	default:
+	}
+	return true
+}
+
+// dequeue takes the oldest request, reporting false when there are none.
+func (self *readRequestQueue) dequeue() (LinesToRead, bool) {
+	self.mutex.Lock()
+	defer self.mutex.Unlock()
+
+	if len(self.requests) == 0 {
+		return LinesToRead{}, false
+	}
+	request := self.requests[0]
+	if len(self.requests) == 1 {
+		// Release the backing array whenever the queue drains, so a one-off burst
+		// doesn't pin its peak size for the rest of the session.
+		self.requests = nil
+	} else {
+		self.requests[0] = LinesToRead{}
+		self.requests = self.requests[1:]
+	}
+	return request, true
+}

pkg/tasks/read_request_queue_test.go

@@ -0,0 +1,58 @@
+package tasks
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReadRequestQueueHandsBackWhatNoTaskWillServe(t *testing.T) {
+	queue := newReadRequestQueue()
+
+	// Nothing has begun serving, so the request comes straight back to its caller.
+	assert.False(t, queue.enqueue(LinesToRead{Total: 1}))
+
+	queue.beginServing()
+	assert.True(t, queue.enqueue(LinesToRead{Total: 1}))
+	assert.True(t, queue.enqueue(LinesToRead{Total: 2}))
+
+	request, ok := queue.dequeue()
+	assert.True(t, ok)
+	assert.Equal(t, 1, request.Total)
+
+	// What the task never got to comes back when it stops, and nothing is taken
+	// from a caller after that.
+	unanswered := queue.stopServing()
+	assert.Len(t, unanswered, 1)
+	assert.Equal(t, 2, unanswered[0].Total)
+
+	assert.False(t, queue.enqueue(LinesToRead{Total: 3}))
+	_, ok = queue.dequeue()
+	assert.False(t, ok)
+}
+
+func TestReadRequestQueueRingsTheDoorbell(t *testing.T) {
+	queue := newReadRequestQueue()
+	doorbell := queue.beginServing()
+
+	select {
+	case <-doorbell:
+		assert.Fail(t, "the doorbell rang before anything was queued")
+	default:
+	}
+
+	// A burst leaves one token, which stands for everything queued.
+	queue.enqueue(LinesToRead{Total: 1})
+	queue.enqueue(LinesToRead{Total: 2})
+
+	select {
+	case <-doorbell:
+	default:
+		assert.Fail(t, "the doorbell didn't ring")
+	}
+	select {
+	case <-doorbell:
+		assert.Fail(t, "the doorbell rang twice for one wake")
+	default:
+	}
+}

pkg/tasks/tasks.go

@@ -70,12 +70,11 @@ type ViewBufferManager struct {
 	taskIDMutex deadlock.Mutex
 	Log         *logrus.Entry
 	newTaskID   int
-	// The channel by which the currently-running task is told to read more
+	// The requests by which the currently-running task is told to read more lines
-	// lines (e.g. as the user scrolls). Held in an atomic because it's swapped
+	// (e.g. as the user scrolls), and which it answers once it has. The task
-	// out as tasks come and go while ReadLines/ReadToEnd read it from the UI
+	// serving them comes and goes; see readRequestQueue.
-	// thread; nil when no task is running.
+	readRequests *readRequestQueue
-	readLines atomic.Pointer[chan LinesToRead]
+	taskKey      string
-	taskKey   string
 
 	// Resets the view's scroll position to the top. A render whose content is
 	// different from what the view last showed (a different command key) calls
@@ -173,6 +172,7 @@ func NewViewBufferManager(
 	onUIThread func(f func()) error,
 ) *ViewBufferManager {
 	return &ViewBufferManager{
+		readRequests: newReadRequestQueue(),
 		Log:          log,
 		writer:       writer,
 		beforeStart:  beforeStart,
@@ -191,12 +191,9 @@ func NewViewBufferManager(
 // (e.g. as the user scrolls down, back up, and down again) don't re-read lines
 // that have already been read: the task only ever reads the shortfall.
 func (self *ViewBufferManager) ReadLines(totalLines int) {
-	if ch := self.readLines.Load(); ch != nil {
+	// A request with no Then needs no answer, so there is nothing to do when no
-		readLines := *ch
+	// task is there to take it.
-		go utils.Safe(func() {
+	self.readRequests.enqueue(LinesToRead{Total: totalLines, InitialRefreshAfter: -1})
-			readLines <- LinesToRead{Total: totalLines, InitialRefreshAfter: -1}
-		})
-	}
 }
 
 // IsLoading reports whether a command task is currently reading content into the
@@ -215,13 +212,31 @@ func (self *ViewBufferManager) StartLoading() {
 }
 
 func (self *ViewBufferManager) ReadToEnd(then func()) {
-	if ch := self.readLines.Load(); ch != nil {
+	// The reading happens on the task's own goroutine, and the caller hears about
-		readLines := *ch
+	// it through then, so lazygit must not count as idle in between.
-		go utils.Safe(func() {
+	task := self.newGocuiTask()
-			readLines <- LinesToRead{Total: -1, InitialRefreshAfter: -1, Then: then}
+	answered := func() {
-		})
+		task.Done()
-	} else if then != nil {
+		if then != nil {
-		then()
+			then()
+		}
+	}
+
+	request := LinesToRead{Total: -1, InitialRefreshAfter: -1, Then: answered}
+	if !self.readRequests.enqueue(request) {
+		// With no task reading, everything there is to read has been read.
+		answered()
+	}
+}
+
+// stopServingReadRequests takes the task away from the read-request queue and
+// answers whatever it never got to, so that nobody is left waiting for a callback
+// that isn't coming.
+func (self *ViewBufferManager) stopServingReadRequests() {
+	for _, request := range self.readRequests.stopServing() {
+		if request.Then != nil {
+			request.Then()
+		}
 	}
 }
 
@@ -289,8 +304,9 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 
 		loadingMutex := deadlock.Mutex{}
 
-		readLines := make(chan LinesToRead, 1024)
+		// Begin serving before any goroutine starts, so that the first request below
-		self.readLines.Store(&readLines)
+		// can't arrive before there is a task to take it.
+		readRequests := self.readRequests.beginServing()
 
 		scanner := bufio.NewScanner(r)
 		scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize))
@@ -423,10 +439,17 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 				if stopped() {
 					break outer
 				}
-				select {
+				linesToRead, ok := self.readRequests.dequeue()
-				case <-opts.Stop:
+				if !ok {
-					break outer
+					// Nothing to read yet: wait to be told there is, or to be stopped.
-				case linesToRead := <-readLines:
+					select {
+					case <-opts.Stop:
+						break outer
+					case <-readRequests:
+					}
+					continue
+				}
+				{
 					callThen := func() {
 						if linesToRead.Then != nil {
 							linesToRead.Then()
@@ -493,21 +516,6 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 							// means a newer task is taking over and is still loading.
 							self.loading.Store(false)
 							callThen()
-							// Any read requests that were queued while we were reading are
-							// now trivially satisfied, since we've read everything. Fire
-							// their callbacks instead of dropping them when we break out of
-							// the loop below (and nil out readLines).
-						drain:
-							for {
-								select {
-								case queued := <-readLines:
-									if queued.Then != nil {
-										queued.Then()
-									}
-								default:
-									break drain
-								}
-							}
 							break outer
 						}
 						writeToView(append(line, '\n'))
@@ -532,7 +540,11 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 				}
 			}
 
-			self.readLines.Store(nil)
+			// Whoever made a request the loop never got to is waiting to hear that the
+			// content it asked for has been read, and there is nothing here to read it
+			// any more: at end of input it has all been read already, and a task that
+			// was stopped is handing the view over to the one replacing it.
+			self.stopServingReadRequests()
 
 			refreshViewIfStale()
 
@@ -556,7 +568,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 			close(lineWrittenChan)
 		})
 
-		readLines <- linesToRead
+		self.readRequests.enqueue(linesToRead)
 
 		<-done
 
@@ -670,7 +682,8 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error
 			self.stopCurrentTask()
 		}
 
-		self.readLines.Store(nil)
+		// Nothing serves read requests between one task and the next.
+		self.stopServingReadRequests()
 
 		stop := make(chan struct{})
 		notifyStopped := make(chan struct{})

pkg/tasks/tasks_test.go

@@ -253,12 +253,9 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) {
 	})
 
 	<-reader.blocked
+	// The request is queued by the time this returns, so it is outstanding when we
+	// let the task reach EOF below.
 	manager.ReadToEnd(func() { thenCalled = true })
-	// ReadToEnd queues its request from a goroutine; wait for it to land so that
-	// it is definitely outstanding by the time we let the task reach EOF.
-	for len(*manager.readLines.Load()) == 0 {
-		time.Sleep(time.Millisecond)
-	}
 	close(reader.unblock)
 
 	wg.Wait()
@@ -486,3 +483,42 @@ func TestNewCmdTaskRefresh(t *testing.T) {
 		}
 	}
 }
+
+// A read request is answered by the task that serves it calling the request's Then.
+// This checks that requests still waiting when the task is stopped are answered too,
+// which is what happens when a re-render replaces the task.
+func TestQueuedReadRequestsAreAnsweredWhenTheTaskStops(t *testing.T) {
+	noop := func() {}
+	task := gocui.NewFakeTask()
+
+	// A pipe the task blocks on, so that the requests are still waiting when it stops.
+	pipeReader, pipeWriter := io.Pipe()
+	defer pipeWriter.Close()
+
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(), bytes.NewBuffer(nil), noop, noop, noop, noop, noop, noop,
+		func() gocui.Task { return task },
+		func(f func()) error { f(); return nil },
+	)
+
+	stop := make(chan struct{})
+	fn := manager.NewCmdTask(
+		func() (Cmd, io.Reader) { return ExecCmd{Cmd: exec.Command("true")}, pipeReader },
+		"", LinesToRead{Total: 1, InitialRefreshAfter: -1}, noop)
+	go func() { _, _ = pipeWriter.Write([]byte("first line\n")) }()
+	go func() { _ = fn(TaskOpts{Stop: stop, InitialContentLoaded: noop}) }()
+	// Let the task start and read the line it was asked for, so that the requests
+	// below are handed to a task that is waiting for them.
+	time.Sleep(50 * time.Millisecond)
+
+	answered := atomic.Int32{}
+	manager.ReadToEnd(func() { answered.Add(1) })
+	manager.ReadToEnd(func() { answered.Add(1) })
+
+	// Let the first request be picked up and block on the pipe, then stop the task.
+	time.Sleep(50 * time.Millisecond)
+	close(stop)
+	time.Sleep(50 * time.Millisecond)
+
+	assert.EqualValues(t, 2, answered.Load())
+}