Fix wrong main view content in rare edge case situations (#6089)

See the individual commit messages for what exactly is fixed here.

pkg/gui/gui_driver.go

@@ -266,12 +266,12 @@ func (self *GuiDriver) TopViewInWindow(windowName string) *gocui.View {
 }
 
 func (self *GuiDriver) SetCaption(caption string) {
-	self.gui.setCaption(caption)
+	self.OnUIThreadAndWait(func() { self.gui.setCaption(caption) })
 	self.waitTillIdle()
 }
 
 func (self *GuiDriver) SetCaptionPrefix(prefix string) {
-	self.gui.setCaptionPrefix(prefix)
+	self.OnUIThreadAndWait(func() { self.gui.setCaptionPrefix(prefix) })
 	self.waitTillIdle()
 }
 

pkg/gui/main_panels.go

@@ -355,9 +355,12 @@ func (gui *Gui) clampDiffSelectionToContent(view *gocui.View) {
 // claiming the render it was showing: whatever it is given next is content the user
 // hasn't seen there, and is shown from the top like any other.
 //
-// A position waiting to be put back goes too: this pane is getting no render for it
-// to ride, and whoever is waiting for the view to be back where it belongs has to
-// hear that it never will be.
+// The pane is emptied right away, and it is also given an empty render. The render
+// takes its place among the view's tasks, so that a render asked for before it can't
+// fill the pane again, whether that render's task is still to be created or is still
+// reading. Like any render that isn't a re-render of what the pane was showing, it
+// drops a position waiting to be put back: whoever is waiting for the view to be back
+// where it belongs has to hear that it never will be.
 func (gui *Gui) clearMainView(mainContext types.Context) {
 	view := mainContext.GetView()
 	view.Clear()
@@ -369,7 +372,9 @@ func (gui *Gui) clearMainView(mainContext types.Context) {
 	gui.State.ContextMgr.UpdateSelectionHighlights()
 	if manager := gui.getViewBufferManagerForView(view); manager != nil {
 		manager.ForgetRenderedContent()
-		manager.DropRestoreForNextTask()
+		if err := gui.newStringTask(view, ""); err != nil {
+			gui.c.Log.Error(err)
+		}
 	}
 }
 

pkg/gui/main_view_render.go

@@ -56,11 +56,16 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er
 	// it before anything else can touch the command's arguments.
 	cmdStr := strings.Join(cmd.Args, " ")
 
+	manager := gui.getManager(view)
+	// The task takes its place among the view's tasks now, although it is only
+	// created after the layout (see the matching call in newCmdTask).
+	reservation := manager.ReserveTask()
+
 	// Mark the view as loading synchronously now, before the layout pass: the
 	// actual task is created in afterLayout (below), which runs after layout, so
 	// without this the next layout pass would clamp the scroll position to the
 	// not-yet-loaded content.
-	gui.getManager(view).StartLoading()
+	manager.StartLoading()
 	// Hold the scrollbar at its current height while the re-render loads, so the
 	// thumb doesn't shrink and snap back when the first partial paint swaps in
 	// (see the matching call in newCmdTask).
@@ -68,6 +73,10 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er
 
 	// Run the render after layout so that it gets the correct size
 	gui.afterLayout(func() error {
+		if manager.IsSuperseded(reservation) {
+			return nil
+		}
+
 		// The layout may have changed the size of the view, so only now is the
 		// width to render at known, and with it the renderer command.
 		width := gui.renderWidth(view)
@@ -113,7 +122,7 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er
 		if rendersThroughAPipe() {
 			run = gui.pipedRender
 		}
-		return gui.newTaskForRender(spec, prefix, cmdStr, run)
+		return gui.newTaskForRender(reservation, spec, prefix, cmdStr, run)
 	})
 
 	return nil
@@ -144,16 +153,17 @@ type (
 type runRender func(spec renderSpec) (startRender, onCloseRender)
 
 // newTaskForRender creates the task that reads the render's output into its
-// view, running the command the given way. key names what is rendered, so that
+// view, running the command the given way. The task takes the place that the
+// reservation holds among the view's tasks. key names what is rendered, so that
 // a re-render of the same content can be told from a render of other content.
-func (gui *Gui) newTaskForRender(spec renderSpec, prefix string, key string, run runRender) error {
+func (gui *Gui) newTaskForRender(reservation tasks.TaskReservation, spec renderSpec, prefix string, key string, run runRender) error {
 	setColumnsEnvVar(spec.cmd, spec.width)
 
 	start, onClose := run(spec)
 
 	manager := gui.getManager(spec.view)
 	linesToRead := gui.linesToReadFromCmdTask(spec.view)
-	return manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), key)
+	return manager.NewReservedTask(reservation, manager.NewCmdTask(start, prefix, linesToRead, onClose), key)
 }
 
 // renderWithoutPtyEnvVar makes a render take the piped path on a platform that

pkg/gui/tasks_adapter.go

@@ -18,10 +18,16 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 		cmdStr,
 	).Debug("RunCommand")
 
+	manager := gui.getManager(view)
+	// The task is only created after the layout (see below), but it has to take
+	// its place among the view's tasks now. Otherwise a task asked for after this
+	// one, but created before the layout, would be replaced by it.
+	reservation := manager.ReserveTask()
+
 	// Mark the view as loading synchronously (before the task's goroutine runs
 	// and before the next layout pass) so the layout doesn't clamp the scroll
 	// position to the not-yet-loaded content.
-	gui.getManager(view).StartLoading()
+	manager.StartLoading()
 	// Hold the scrollbar at the height the view has now (the previous render),
 	// while it still shows that render: once the re-render swaps in its first
 	// partial paint the displayed buffer is briefly short, and we don't want the
@@ -34,8 +40,12 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 	// keeps the task goroutine from reading the view's live dimensions while it
 	// streams output.
 	gui.afterLayout(func() error {
+		if manager.IsSuperseded(reservation) {
+			return nil
+		}
+
 		spec := renderSpec{view: view, cmd: cmd, width: gui.renderWidth(view)}
-		return gui.newTaskForRender(spec, prefix, cmdStr, gui.plainRender)
+		return gui.newTaskForRender(reservation, spec, prefix, cmdStr, gui.plainRender)
 	})
 
 	return nil

pkg/integration/tests/main_view/keep_an_emptied_pane_empty.go

@@ -0,0 +1,43 @@
+package main_view
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+// Both selection changes are handled before the next layout. Selecting file_a asks
+// for its staged changes in the lower pane, whose task is only created after the
+// layout. Selecting file_b again leaves that pane with nothing to show, and empties it
+// right away. The pane is hidden then, but when it is shown again, it shows what it
+// holds until its next render replaces it.
+var KeepAnEmptiedPaneEmpty = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Selecting a file and moving back off it in rapid succession leaves the pane that the file filled empty",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file_a", "one\n")
+		shell.CreateFileAndAdd("file_b", "one\n")
+		shell.Commit("one")
+
+		shell.UpdateFileAndAdd("file_a", "STAGED\n")
+		shell.UpdateFile("file_a", "UNSTAGED\n")
+		shell.UpdateFile("file_b", "two\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Files().
+			IsFocused().
+			NavigateToLine(Contains("file_b"))
+
+		t.Views().Secondary().
+			IsInvisible()
+
+		t.Views().Files().
+			PressRapidly(keys.Universal.PrevItem, keys.Universal.NextItem).
+			SelectedLine(Contains("file_b"))
+
+		t.Views().Secondary().
+			IsInvisible().
+			Content(Equals(""))
+	},
+})

pkg/integration/tests/main_view/show_the_tab_switched_to_last.go

@@ -0,0 +1,39 @@
+package main_view
+
+import (
+	"github.com/jesseduffield/lazygit/pkg/config"
+	. "github.com/jesseduffield/lazygit/pkg/integration/components"
+)
+
+// Both tab switches are handled before the next layout. Switching to the Files tab
+// asks for the file's diff, whose task is only created after the layout; switching
+// back to the Worktrees tab asks for the worktree's details, whose task is created
+// right away. The main view has to show what was asked for last.
+var ShowTheTabSwitchedToLast = NewIntegrationTest(NewIntegrationTestArgs{
+	Description:  "Switching tabs twice in rapid succession leaves the main view showing the second tab's content",
+	ExtraCmdArgs: []string{},
+	Skip:         false,
+	SetupConfig:  func(config *config.AppConfig) {},
+	SetupRepo: func(shell *Shell) {
+		shell.CreateFileAndAdd("file1", "one\n")
+		shell.Commit("one")
+		shell.UpdateFile("file1", "ONE\n")
+	},
+	Run: func(t *TestDriver, keys config.KeybindingConfig) {
+		t.Views().Worktrees().
+			Focus().
+			Lines(
+				Contains("(main worktree)").IsSelected(),
+			)
+
+		t.Views().Main().
+			Content(Contains("Path:"))
+
+		t.Views().Worktrees().
+			PressRapidly(keys.Universal.PrevTab, keys.Universal.NextTab).
+			IsFocused()
+
+		t.Views().Main().
+			Content(Contains("Path:"))
+	},
+})

pkg/integration/tests/test_list.go

@@ -410,6 +410,7 @@ var tests = []*components.IntegrationTest{
 	main_view.JumpToAFileOfTheDiff,
 	main_view.JumpToAFileOnlyOverADiff,
 	main_view.KeepAWrappedLineCoveredAcrossARerender,
+	main_view.KeepAnEmptiedPaneEmpty,
 	main_view.KeepBothHalvesOfAChangeSelected,
 	main_view.KeepPositionByTheVisibleEndOfASelection,
 	main_view.KeepPositionInBothPanesWhenChangingContextSize,
@@ -480,6 +481,7 @@ var tests = []*components.IntegrationTest{
 	main_view.SelectionCommandTooltipsFollowTheDiff,
 	main_view.SelectionCommandsOnlyWhereTheyApply,
 	main_view.SelectionOverTheCustomPatch,
+	main_view.ShowTheTabSwitchedToLast,
 	main_view.StageDeletedFile,
 	main_view.StageDiffLines,
 	main_view.StageDiffLinesOfAPathWithASpace,

pkg/tasks/tasks.go

@@ -64,9 +64,10 @@ type ViewBufferManager struct {
 
 	waitingMutex deadlock.Mutex
 	// Guards newTaskID and taskKey, which identify the most recently requested
-	// task. Both are written on the goroutine NewTask spawns, and taskKey is
-	// read from the UI thread (GetTaskKey), so neither may be touched without
-	// holding this.
+	// task. newTaskID is written wherever a task is asked for (ReserveTask) and
+	// read on the goroutine NewReservedTask spawns. taskKey is written on that
+	// goroutine and read from the UI thread (GetTaskKey). So neither may be
+	// touched without holding this.
 	taskIDMutex deadlock.Mutex
 	Log         *logrus.Entry
 	newTaskID   int
@@ -116,10 +117,12 @@ type ViewBufferManager struct {
 	// Guarded by taskIDMutex, like the task key.
 	keepScrollForNextTask bool
 
-	// Whether a command task is currently reading content into the view. While
-	// this is true the content is still growing, so callers (e.g. the layout)
-	// must not clamp the view's scroll position to the amount loaded so far.
-	loading atomic.Bool
+	// The command task whose output the view is loading: the one that was asked
+	// for last when StartLoading was called. It is cleared when that task has
+	// read all of its input. The view counts as loading only while this task is
+	// still the one asked for last; a task asked for after it takes the view
+	// over, whatever it shows. Guarded by taskIDMutex, like the task key.
+	loadingTaskID int
 
 	// beforeStart is the function that is called before starting a new task
 	beforeStart  func()
@@ -343,19 +346,36 @@ func (self *ViewBufferManager) ReadLines(totalLines int) {
 	self.readRequests.enqueue(LinesToRead{Total: totalLines, InitialRefreshAfter: -1})
 }
 
-// IsLoading reports whether a command task is currently reading content into the
-// view, meaning the content is still growing.
+// IsLoading reports whether the task asked for last is a command task that is
+// still reading content into the view, meaning the content is still growing.
 func (self *ViewBufferManager) IsLoading() bool {
-	return self.loading.Load()
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	return self.loadingTaskID != 0 && self.loadingTaskID == self.newTaskID
 }
 
-// StartLoading marks the view as loading content. It must be called
-// synchronously when a command/pty task is started, before the task's goroutine
-// runs, so that a layout pass happening in between doesn't clamp the scroll
-// position to the not-yet-loaded content. It is cleared when the task reaches
-// the end of its input.
+// StartLoading marks the view as loading the output of the task asked for last,
+// which must be a command task. Call it right when that task is asked for, before
+// its goroutine runs and before the next layout pass, so that the layout doesn't
+// clamp the scroll position to the not-yet-loaded content. The view stops loading
+// when the task reaches the end of its input, or when another task is asked for.
 func (self *ViewBufferManager) StartLoading() {
-	self.loading.Store(true)
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	self.loadingTaskID = self.newTaskID
+}
+
+// finishLoading records that the given task has read all of its input. If the
+// view is loading another task's output, that task is still loading.
+func (self *ViewBufferManager) finishLoading(taskID int) {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	if self.loadingTaskID == taskID {
+		self.loadingTaskID = 0
+	}
 }
 
 func (self *ViewBufferManager) ReadToEnd(then func()) {
@@ -700,10 +720,8 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix
 								self.onEndOfInput()
 							})
 							// The content is fully loaded now, so it's safe again for the
-							// layout to clamp the scroll position to it. We deliberately
-							// don't clear this when stopped (rather than EOF'd), because that
-							// means a newer task is taking over and is still loading.
-							self.loading.Store(false)
+							// layout to clamp the scroll position to it.
+							self.finishLoading(opts.taskID)
 							callThen()
 							break outer
 						}
@@ -815,9 +833,48 @@ type TaskOpts struct {
 	// We use this to keep track of when a user's action is complete (i.e. all views
 	// have been refreshed to display the results of their action)
 	InitialContentLoaded func()
+
+	// The task's place in the order of the view's tasks (see ReserveTask).
+	taskID int
 }
 
+// A TaskReservation holds a task's place in the order of the view's tasks, from
+// when the task is asked for until it is created. See ReserveTask.
+type TaskReservation struct {
+	taskID int
+}
+
+// ReserveTask gives a task its place in the order of the view's tasks, for a task
+// that is only created later, with NewReservedTask. The view shows the task that
+// was asked for last. If another task is asked for after the reservation, that
+// task replaces the reserved one, even when it is created first.
+func (self *ViewBufferManager) ReserveTask() TaskReservation {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	self.newTaskID++
+	return TaskReservation{taskID: self.newTaskID}
+}
+
+// IsSuperseded reports whether another task has been asked for since the
+// reservation was made. The reserved task would then stop as soon as it was
+// created, so there is no point in creating it.
+func (self *ViewBufferManager) IsSuperseded(reservation TaskReservation) bool {
+	self.taskIDMutex.Lock()
+	defer self.taskIDMutex.Unlock()
+
+	return reservation.taskID < self.newTaskID
+}
+
+// NewTask creates a task that takes its place in the order of the view's tasks
+// right away.
 func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error {
+	return self.NewReservedTask(self.ReserveTask(), f, key)
+}
+
+// NewReservedTask creates the task that the reservation was made for. It doesn't
+// run if another task has been asked for since the reservation was made.
+func (self *ViewBufferManager) NewReservedTask(reservation TaskReservation, f func(TaskOpts) error, key string) error {
 	gocuiTask := self.newGocuiTask()
 
 	var completeTaskOnce sync.Once
@@ -828,15 +885,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error
 		})
 	}
 
-	// Assign the taskID synchronously so it reflects NewTask call order
-	// rather than the order in which the spawned goroutines happen to be
-	// scheduled. Otherwise two NewTask calls in quick succession can have
-	// their goroutines race, with the later-called task ending up with the
-	// lower taskID and losing the staleness check below.
-	self.taskIDMutex.Lock()
-	self.newTaskID++
-	taskID := self.newTaskID
-	self.taskIDMutex.Unlock()
+	taskID := reservation.taskID
 
 	go utils.Safe(func() {
 		defer completeGocuiTask()
@@ -906,7 +955,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error
 
 		self.waitingMutex.Unlock()
 
-		if err := f(TaskOpts{Stop: stop, InitialContentLoaded: completeGocuiTask}); err != nil {
+		if err := f(TaskOpts{Stop: stop, InitialContentLoaded: completeGocuiTask, taskID: taskID}); err != nil {
 			self.Log.Error(err) // might need an onError callback
 		}
 

pkg/tasks/tasks_test.go

@@ -813,3 +813,142 @@ func TestReadToEndHoldsItsTaskUntilThenReturns(t *testing.T) {
 	assert.Equal(t, gocui.TaskStatusBusy, statusDuringThen)
 	assert.Equal(t, gocui.TaskStatusDone, task.Status())
 }
+
+// doneSignallingTask lets a test wait for the tasks of a view to finish, including
+// those that never get to run.
+type doneSignallingTask struct {
+	*gocui.FakeTask
+	done func()
+}
+
+func (self *doneSignallingTask) Done() {
+	self.FakeTask.Done()
+	self.done()
+}
+
+// A render whose task is only created after the layout takes its place among the
+// view's tasks when it is asked for. A message asked for after it, before the
+// layout, is created first, and the render's task mustn't replace it.
+func TestReservedTaskDoesntReplaceATaskAskedForLater(t *testing.T) {
+	var tasksDone sync.WaitGroup
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		bytes.NewBuffer(nil),
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() gocui.Task {
+			tasksDone.Add(1)
+			return &doneSignallingTask{FakeTask: gocui.NewFakeTask(), done: tasksDone.Done}
+		},
+		// no UI thread in the test; run the view mutations inline
+		func(f func()) error { f(); return nil },
+	)
+
+	var mutex sync.Mutex
+	var tasksRun []string
+	task := func(name string) func(TaskOpts) error {
+		return func(TaskOpts) error {
+			mutex.Lock()
+			defer mutex.Unlock()
+			tasksRun = append(tasksRun, name)
+			return nil
+		}
+	}
+
+	reservation := manager.ReserveTask()
+	assert.False(t, manager.IsSuperseded(reservation))
+
+	_ = manager.NewTask(task("message"), "message")
+	assert.True(t, manager.IsSuperseded(reservation))
+
+	_ = manager.NewReservedTask(reservation, task("render"), "render")
+	tasksDone.Wait()
+
+	assert.Equal(t, []string{"message"}, tasksRun)
+}
+
+// A message that replaces a command task while the task is still reading its
+// output leaves nothing loading content into the view.
+func TestMessageEndsTheLoadingOfTheTaskItReplaces(t *testing.T) {
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		bytes.NewBuffer(nil),
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() gocui.Task { return gocui.NewFakeTask() },
+		// no UI thread in the test; run the view mutations inline
+		func(f func()) error { f(); return nil },
+	)
+
+	stalled := BlockingLineReader{
+		linesToYield: 3,
+		blocked:      make(chan struct{}),
+		unblock:      make(chan struct{}),
+	}
+	defer close(stalled.unblock)
+	start := func() (Cmd, io.Reader) {
+		// not actually starting this because it's not necessary
+		return ExecCmd{Cmd: exec.Command("blah")}, &stalled
+	}
+
+	reservation := manager.ReserveTask()
+	manager.StartLoading()
+	_ = manager.NewReservedTask(reservation, manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, nil), "cmd")
+	<-stalled.blocked
+	assert.True(t, manager.IsLoading())
+
+	_ = manager.NewTask(func(TaskOpts) error { return nil }, "message")
+	assert.False(t, manager.IsLoading())
+}
+
+// The view is loading the content of the command task asked for last. A task
+// asked for before it reaching the end of its input doesn't end that.
+func TestEarlierTaskEndingLeavesALaterTaskLoading(t *testing.T) {
+	manager := NewViewBufferManager(
+		utils.NewDummyLog(),
+		bytes.NewBuffer(nil),
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() {},
+		func() gocui.Task { return gocui.NewFakeTask() },
+		// no UI thread in the test; run the view mutations inline
+		func(f func()) error { f(); return nil },
+	)
+
+	stalled := BlockingLineReader{
+		linesToYield: 3,
+		blocked:      make(chan struct{}),
+		unblock:      make(chan struct{}),
+	}
+	start := func() (Cmd, io.Reader) {
+		// not actually starting this because it's not necessary
+		return ExecCmd{Cmd: exec.Command("blah")}, &stalled
+	}
+
+	earlierDone := make(chan struct{})
+	reservation := manager.ReserveTask()
+	manager.StartLoading()
+	_ = manager.NewReservedTask(reservation,
+		manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, func() { close(earlierDone) }), "earlier")
+	<-stalled.blocked
+
+	// The later task is asked for, but not created yet, as for a render whose task
+	// is created after the layout.
+	manager.ReserveTask()
+	manager.StartLoading()
+
+	close(stalled.unblock)
+	<-earlierDone
+	assert.True(t, manager.IsLoading())
+}