diff --git a/AGENTS.md b/AGENTS.md index d65a623d9..f38a336e0 100644 --- a/AGENTS.md +++ b/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 diff --git a/pkg/gocui/search_test.go b/pkg/gocui/search_test.go new file mode 100644 index 000000000..ba20da9de --- /dev/null +++ b/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() + } +} diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 6e7b35520..0a06d1981 100644 --- a/pkg/gocui/view.go +++ b/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 , 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 diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index a0efcb14c..bc4a4219e 100644 --- a/pkg/gui/main_panels.go +++ b/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() +} diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 5e5295639..3ed141d68 100644 --- a/pkg/gui/tasks_adapter.go +++ b/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) diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 5f3e4c2ab..3d924a566 100644 --- a/pkg/gui/view_helpers.go +++ b/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. - // We might be able to fix the problems with this, but it doesn't seem easy, so for now - // just don't rerender the view while searching, on the assumption that users will probably - // 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() - } + 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 diff --git a/pkg/integration/tests/filter_and_search/rerender_the_searched_main_view.go b/pkg/integration/tests/filter_and_search/rerender_the_searched_main_view.go new file mode 100644 index 000000000..3d631e650 --- /dev/null +++ b/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'")) + }, +}) diff --git a/pkg/integration/tests/filter_and_search/search_a_long_diff.go b/pkg/integration/tests/filter_and_search/search_a_long_diff.go new file mode 100644 index 000000000..7f2ebf156 --- /dev/null +++ b/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")) + }, +}) diff --git a/pkg/integration/tests/filter_and_search/search_status_after_a_rerender.go b/pkg/integration/tests/filter_and_search/search_status_after_a_rerender.go new file mode 100644 index 000000000..8116049b9 --- /dev/null +++ b/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)")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 0d25ddba6..a4e732cf0 100644 --- a/pkg/integration/tests/test_list.go +++ b/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, diff --git a/pkg/tasks/read_request_queue.go b/pkg/tasks/read_request_queue.go new file mode 100644 index 000000000..444e514fd --- /dev/null +++ b/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 +} diff --git a/pkg/tasks/read_request_queue_test.go b/pkg/tasks/read_request_queue_test.go new file mode 100644 index 000000000..423b89d09 --- /dev/null +++ b/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: + } +} diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 17cfabb5f..deaeee042 100644 --- a/pkg/tasks/tasks.go +++ b/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 - // lines (e.g. as the user scrolls). Held in an atomic because it's swapped - // out as tasks come and go while ReadLines/ReadToEnd read it from the UI - // thread; nil when no task is running. - readLines atomic.Pointer[chan LinesToRead] - taskKey string + // The requests by which the currently-running task is told to read more lines + // (e.g. as the user scrolls), and which it answers once it has. The task + // serving them comes and goes; see readRequestQueue. + readRequests *readRequestQueue + 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 { - readLines := *ch - go utils.Safe(func() { - readLines <- LinesToRead{Total: totalLines, InitialRefreshAfter: -1} - }) - } + // A request with no Then needs no answer, so there is nothing to do when no + // task is there to take it. + self.readRequests.enqueue(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 { - readLines := *ch - go utils.Safe(func() { - readLines <- LinesToRead{Total: -1, InitialRefreshAfter: -1, Then: then} - }) - } else if then != nil { - then() + // The reading happens on the task's own goroutine, and the caller hears about + // it through then, so lazygit must not count as idle in between. + task := self.newGocuiTask() + answered := func() { + task.Done() + if then != nil { + 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) - self.readLines.Store(&readLines) + // Begin serving before any goroutine starts, so that the first request below + // 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 { - case <-opts.Stop: - break outer - case linesToRead := <-readLines: + linesToRead, ok := self.readRequests.dequeue() + if !ok { + // Nothing to read yet: wait to be told there is, or to be stopped. + 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{}) diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index b15b48ee4..c50a54cac 100644 --- a/pkg/tasks/tasks_test.go +++ b/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()) +}