Improve performance of moving rebase todos (#5978)

In larger repos, moving a rebase todo in an interactive rebase was
slower than necessary, especially when moving it up/down all the way
using auto-repeat.

pkg/commands/git_commands/commit_loader.go

@@ -174,7 +174,7 @@ func (self *CommitLoader) MergeRebasingCommits(hashPool *utils.StringPool, commi
 	}
 
 	if workingTreeState.Rebasing {
-		rebasingCommits, err := self.getHydratedRebasingCommits(hashPool, addConflictedRebasingCommit)
+		rebasingCommits, err := self.getHydratedRebasingCommits(hashPool, commits, addConflictedRebasingCommit)
 		if err != nil {
 			return nil, err
 		}
@@ -251,8 +251,8 @@ func (self *CommitLoader) extractCommitFromLine(hashPool *utils.StringPool, line
 	})
 }
 
-func (self *CommitLoader) getHydratedRebasingCommits(hashPool *utils.StringPool, addConflictingCommit bool) ([]*models.Commit, error) {
-	return self.getHydratedTodoCommits(hashPool, self.getRebasingCommits(hashPool, addConflictingCommit), false)
+func (self *CommitLoader) getHydratedRebasingCommits(hashPool *utils.StringPool, existingCommits []*models.Commit, addConflictingCommit bool) ([]*models.Commit, error) {
+	return self.getHydratedTodoCommits(hashPool, self.getRebasingCommits(hashPool, addConflictingCommit), existingCommits, false)
 }
 
 func (self *CommitLoader) getHydratedSequencerCommits(hashPool *utils.StringPool, workingTreeState models.WorkingTreeState) ([]*models.Commit, error) {
@@ -271,39 +271,56 @@ func (self *CommitLoader) getHydratedSequencerCommits(hashPool *utils.StringPool
 		}
 	}
 
-	return self.getHydratedTodoCommits(hashPool, commits, true)
+	return self.getHydratedTodoCommits(hashPool, commits, nil, true)
 }
 
-func (self *CommitLoader) getHydratedTodoCommits(hashPool *utils.StringPool, todoCommits []*models.Commit, todoFileHasShortHashes bool) ([]*models.Commit, error) {
+func (self *CommitLoader) getHydratedTodoCommits(
+	hashPool *utils.StringPool,
+	todoCommits []*models.Commit,
+	existingCommits []*models.Commit,
+	todoFileHasShortHashes bool,
+) ([]*models.Commit, error) {
 	if len(todoCommits) == 0 {
 		return nil, nil
 	}
 
-	commitHashes := lo.FilterMap(todoCommits, func(commit *models.Commit, _ int) (string, bool) {
-		return commit.Hash(), commit.Hash() != ""
-	})
-
-	// note that we're not filtering these as we do non-rebasing commits just because
-	// I suspect that will cause some damage
-	cmdObj := self.cmd.New(
-		NewGitCmd("show").
-			Config("log.showSignature=false").
-			Arg("--no-patch", "--oneline", "--abbrev=20", prettyFormat).
-			Arg(commitHashes...).
-			ToArgv(),
-	).DontLog()
-
+	// A refresh of only the rebasing todos should reuse the already loaded todos to avoid
+	// unnecessary git show calls.
 	fullCommits := map[string]*models.Commit{}
-	err := cmdObj.RunAndProcessLines(func(line string) (bool, error) {
-		if line == "" || line[0] != '+' {
-			return false, nil
+	for _, commit := range existingCommits {
+		if commit.IsTODO() && commit.Hash() != "" {
+			// Make a copy of the commit; that's necessary to avoid mutating the original commit
+			// when we later reuse it in the loop at the end of this function.
+			fullCommits[commit.Hash()] = lo.ToPtr(*commit)
 		}
-		commit := self.extractCommitFromLine(hashPool, line[1:], false)
-		fullCommits[commit.Hash()] = commit
-		return false, nil
+	}
+
+	commitHashesToFetch := lo.FilterMap(todoCommits, func(commit *models.Commit, _ int) (string, bool) {
+		return commit.Hash(), commit.Hash() != "" && fullCommits[commit.Hash()] == nil
 	})
-	if err != nil {
-		return nil, err
+
+	if len(commitHashesToFetch) > 0 {
+		// note that we're not filtering these as we do non-rebasing commits just because
+		// I suspect that will cause some damage
+		cmdObj := self.cmd.New(
+			NewGitCmd("show").
+				Config("log.showSignature=false").
+				Arg("--no-patch", "--oneline", "--abbrev=20", prettyFormat).
+				Arg(commitHashesToFetch...).
+				ToArgv(),
+		).DontLog()
+
+		err := cmdObj.RunAndProcessLines(func(line string) (bool, error) {
+			if line == "" || line[0] != '+' {
+				return false, nil
+			}
+			commit := self.extractCommitFromLine(hashPool, line[1:], false)
+			fullCommits[commit.Hash()] = commit
+			return false, nil
+		})
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	findFullCommit := lo.Ternary(todoFileHasShortHashes,

pkg/commands/git_commands/commit_loader_test.go

@@ -538,6 +538,110 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
 	}
 }
 
+func TestCommitLoaderGetHydratedTodoCommitsReusesExistingCommit(t *testing.T) {
+	hashPool := &utils.StringPool{}
+	runner := oscommands.NewFakeRunner(t)
+	loader := &CommitLoader{
+		cmd: oscommands.NewDummyCmdObjBuilder(runner),
+	}
+	existingCommit := models.NewCommit(hashPool, models.NewCommitOpts{
+		Hash:          "0123456789012345678901234567890123456789",
+		Name:          "hydrated subject",
+		AuthorName:    "Jane Doe",
+		AuthorEmail:   "jane@example.com",
+		UnixTimestamp: 1234,
+		Parents:       []string{"1123456789012345678901234567890123456789"},
+		Status:        models.StatusRebasing,
+		Action:        todo.Pick,
+	})
+	refreshedTodo := models.NewCommit(hashPool, models.NewCommitOpts{
+		Hash:       existingCommit.Hash(),
+		Name:       "subject from the todo file",
+		Status:     models.StatusConflicted,
+		Action:     todo.Fixup,
+		ActionFlag: "-C",
+	})
+
+	commits, err := loader.getHydratedTodoCommits(
+		hashPool,
+		[]*models.Commit{refreshedTodo},
+		[]*models.Commit{existingCommit},
+		false,
+	)
+
+	assert.NoError(t, err)
+	assert.Equal(t, []*models.Commit{
+		models.NewCommit(hashPool, models.NewCommitOpts{
+			Hash:          existingCommit.Hash(),
+			Name:          "hydrated subject",
+			AuthorName:    "Jane Doe",
+			AuthorEmail:   "jane@example.com",
+			UnixTimestamp: 1234,
+			Parents:       []string{"1123456789012345678901234567890123456789"},
+			Status:        models.StatusConflicted,
+			Action:        todo.Fixup,
+			ActionFlag:    "-C",
+		}),
+	}, commits)
+	assert.Equal(t, todo.Pick, existingCommit.Action)
+	assert.Equal(t, models.StatusRebasing, existingCommit.Status)
+	runner.CheckForMissingCalls()
+}
+
+func TestCommitLoaderGetHydratedTodoCommitsLoadsMissingCommit(t *testing.T) {
+	hashPool := &utils.StringPool{}
+	existingHash := "0123456789012345678901234567890123456789"
+	missingHash := "2123456789012345678901234567890123456789"
+	missingCommitOutput := strings.ReplaceAll(
+		`+2123456789012345678901234567890123456789|1235|John Doe|john@example.com||>|tag: new|new subject`,
+		"|",
+		"\x00",
+	)
+	runner := oscommands.NewFakeRunner(t).ExpectGitArgs(
+		[]string{
+			"-c", "log.showSignature=false", "show", "--no-patch", "--oneline", "--abbrev=20",
+			prettyFormat, missingHash,
+		},
+		missingCommitOutput,
+		nil,
+	)
+	loader := &CommitLoader{
+		cmd: oscommands.NewDummyCmdObjBuilder(runner),
+	}
+	existingCommit := models.NewCommit(hashPool, models.NewCommitOpts{
+		Hash:   existingHash,
+		Name:   "existing subject",
+		Status: models.StatusRebasing,
+		Action: todo.Pick,
+	})
+	refreshedTodos := []*models.Commit{
+		models.NewCommit(hashPool, models.NewCommitOpts{
+			Hash:   existingHash,
+			Status: models.StatusRebasing,
+			Action: todo.Pick,
+		}),
+		models.NewCommit(hashPool, models.NewCommitOpts{
+			Hash:   missingHash,
+			Status: models.StatusRebasing,
+			Action: todo.Edit,
+		}),
+	}
+
+	commits, err := loader.getHydratedTodoCommits(
+		hashPool,
+		refreshedTodos,
+		[]*models.Commit{existingCommit},
+		false,
+	)
+
+	assert.NoError(t, err)
+	assert.Len(t, commits, 2)
+	assert.Equal(t, "existing subject", commits[0].Name)
+	assert.Equal(t, "new subject", commits[1].Name)
+	assert.Equal(t, todo.Edit, commits[1].Action)
+	runner.CheckForMissingCalls()
+}
+
 func TestCommitLoader_setCommitStatuses(t *testing.T) {
 	type scenario struct {
 		testName           string

pkg/gui/context/simple_context.go

@@ -41,7 +41,7 @@ func (self *SimpleContext) HandleFocus(opts types.OnFocusOpts) {
 		fn(opts)
 	}
 
-	if self.onRenderToMainFn != nil {
+	if self.onRenderToMainFn != nil && !opts.SkipMainViewUpdate {
 		self.onRenderToMainFn()
 	}
 }

pkg/gui/controllers/helpers/refresh_helper.go

@@ -119,6 +119,9 @@ type refreshEnv struct {
 	// reload state (see RefreshOptions.DontBlockRepoSwitch).
 	keepScrollPosition bool
 
+	// Whether refreshing a side context should leave the main view unchanged.
+	skipMainViewUpdate bool
+
 	// the repo generation captured when the refresh started
 	generation int
 
@@ -231,6 +234,7 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr
 		background:         options.Background || options.DontBlockRepoSwitch,
 		backgroundRoutine:  options.Background,
 		keepScrollPosition: options.Background || options.DontBlockRepoSwitch,
+		skipMainViewUpdate: options.SkipMainViewUpdate,
 	}
 	if !self.captureOnUIThread(calledFromWorker, env.background, func() {
 		env.generation = self.c.State().GetRepoGeneration()
@@ -1670,11 +1674,10 @@ func (self *RefreshHelper) refreshView(context types.Context, env refreshEnv) {
 		// the filtered list model is up to date for rendering.
 		self.searchHelper.ReApplyFilter(context)
 
-		if env.keepScrollPosition {
-			self.c.PostRefreshUpdateKeepingScrollPosition(context)
-		} else {
-			self.c.PostRefreshUpdate(context)
-		}
+		self.c.PostRefreshUpdateWithOptions(context, types.OnFocusOpts{
+			KeepScrollPosition: env.keepScrollPosition,
+			SkipMainViewUpdate: env.skipMainViewUpdate,
+		})
 
 		self.c.AfterLayout(func() error {
 			// Re-applying the search must be done after re-rendering the view though,
@@ -1853,7 +1856,8 @@ func (self *RefreshHelper) setGithubPullRequests(baseInfo *githubRemoteInfo, bra
 		// This lands whenever the network call happens to return, and only
 		// changes how the branches are rendered, not which one is selected, so
 		// it has no business moving the viewport.
-		self.c.PostRefreshUpdateKeepingScrollPosition(self.c.Contexts().Branches)
+		self.c.PostRefreshUpdateWithOptions(self.c.Contexts().Branches,
+			types.OnFocusOpts{KeepScrollPosition: true})
 	})
 }
 

pkg/gui/controllers/local_commits_controller.go

@@ -191,7 +191,8 @@ func (self *LocalCommitsController) handleCommitDrag(opts gocui.ViewMouseBinding
 
 	self.commitDrag.hasMoved = true
 	if self.updateCommitDragInsertion(opts.Y) {
-		self.c.PostRefreshUpdateKeepingScrollPosition(self.context())
+		self.c.PostRefreshUpdateWithOptions(self.context(),
+			types.OnFocusOpts{KeepScrollPosition: true})
 	}
 	originY := self.context().GetView().OriginY()
 	self.dragAutoscroller.Update(opts.Y - originY)
@@ -344,7 +345,9 @@ func (self *LocalCommitsController) startMovingCommitsIndicator(insertionIndex i
 func (self *LocalCommitsController) stopMovingCommitsIndicator() {
 	self.stopMovingCommitsIndicatorTicker()
 	self.context().ClearDropInsertionIndex()
-	self.c.PostRefreshUpdate(self.context())
+	self.c.PostRefreshUpdateWithOptions(
+		self.context(), types.OnFocusOpts{SkipMainViewUpdate: true},
+	)
 }
 
 func (self *LocalCommitsController) stopMovingCommitsIndicatorTicker() {
@@ -1170,16 +1173,21 @@ func (self *LocalCommitsController) move(
 		if err := self.c.Git().Rebase.MoveTodos(selectedCommits, offset); err != nil {
 			return err
 		}
-		self.context().MoveSelection(offset)
-		self.context().HandleFocus(types.OnFocusOpts{})
 
 		// Block input until the refresh has landed: a quick second press must
 		// read the moved todo from the refreshed model, not grab whatever the
 		// advanced selection index points at in the stale one.
 		self.c.RefreshBlockingInput(types.RefreshOptions{
-			Scope:           []types.RefreshableView{types.REBASE_COMMITS},
-			CommitSelection: types.KeepCommitSelectionIndex,
-			Then:            onComplete,
+			Scope:              []types.RefreshableView{types.REBASE_COMMITS},
+			SkipMainViewUpdate: true,
+			Then: func() error {
+				self.context().MoveSelection(offset)
+				self.context().FocusLine(true)
+				if onComplete != nil {
+					return onComplete()
+				}
+				return nil
+			},
 		})
 		return nil
 	}

pkg/gui/gui_common.go

@@ -39,11 +39,11 @@ func (self *guiCommon) RefreshFromWorker(opts types.RefreshOptions) {
 }
 
 func (self *guiCommon) PostRefreshUpdate(context types.Context) {
-	self.gui.postRefreshUpdate(context, false)
+	self.gui.postRefreshUpdate(context, types.OnFocusOpts{})
 }
 
-func (self *guiCommon) PostRefreshUpdateKeepingScrollPosition(context types.Context) {
-	self.gui.postRefreshUpdate(context, true)
+func (self *guiCommon) PostRefreshUpdateWithOptions(context types.Context, opts types.OnFocusOpts) {
+	self.gui.postRefreshUpdate(context, opts)
 }
 
 func (self *guiCommon) RunSubprocessAndRefresh(cmdObj *oscommands.CmdObj) error {

pkg/gui/types/common.go

@@ -51,10 +51,9 @@ type IGuiCommon interface {
 	// case would be overkill, although refresh will internally call 'PostRefreshUpdate'.
 	// It re-focuses the context's selection, which scrolls it into view.
 	PostRefreshUpdate(Context)
-	// Like PostRefreshUpdate, but leaves the view scrolled where it is. For
-	// refreshes that no user action is behind: those must not move the viewport
-	// away from wherever the user last put it.
-	PostRefreshUpdateKeepingScrollPosition(Context)
+	// Like PostRefreshUpdate, with control over scrolling and whether to update
+	// the main view.
+	PostRefreshUpdateWithOptions(Context, OnFocusOpts)
 
 	// renders string to a view without resetting its origin
 	SetViewContent(view *gocui.View, content string)

pkg/gui/types/context.go

@@ -234,6 +234,10 @@ type OnFocusOpts struct {
 	// the view's scroll position alone instead; only for callers that maintain
 	// it themselves, e.g. by keeping the selection at the edge of the viewport.
 	KeepScrollPosition bool
+
+	// Set this when the focused item hasn't changed and the main view's current
+	// content is still valid.
+	SkipMainViewUpdate bool
 }
 
 type OnFocusLostOpts struct {

pkg/gui/types/refresh.go

@@ -72,6 +72,10 @@ type RefreshOptions struct {
 	// letting each scope update the UI as soon as it's done.
 	BatchUIUpdates bool
 
+	// Set this when the refresh doesn't invalidate the main view's current
+	// content, so refreshing the side context needn't render it again.
+	SkipMainViewUpdate bool
+
 	// Controls which local branch is selected after the refresh. Defaults to
 	// KeepBranchSelectionByName.
 	BranchSelection BranchSelectionBehavior

pkg/gui/view_helpers.go

@@ -132,7 +132,7 @@ func (gui *Gui) renderContentOnly() {
 // postRefreshUpdate is to be called on a context after the state that it depends on has been refreshed
 // if the context's view is set to another context we do nothing.
 // if the context's view is the current view we trigger a focus; re-selecting the current item.
-func (gui *Gui) postRefreshUpdate(c types.Context, keepScrollPosition bool) {
+func (gui *Gui) postRefreshUpdate(c types.Context, opts types.OnFocusOpts) {
 	t := time.Now()
 	defer func() {
 		gui.Log.Infof("postRefreshUpdate for %s took %s", c.GetKey(), time.Since(t))
@@ -141,14 +141,17 @@ func (gui *Gui) postRefreshUpdate(c types.Context, keepScrollPosition bool) {
 	c.HandleRender()
 
 	if gui.currentViewName() == c.GetViewName() {
-		c.HandleFocus(types.OnFocusOpts{KeepScrollPosition: keepScrollPosition})
+		c.HandleFocus(opts)
 	} else {
 		// The FocusLine call is included in the HandleFocus method which we
 		// call for focused views above; but we need to call it here for
 		// non-focused views to ensure that an inactive selection is painted
 		// correctly, and that integration tests see the up to date selection
 		// state.
-		c.FocusLine(!keepScrollPosition)
+		c.FocusLine(!opts.KeepScrollPosition)
+		if opts.SkipMainViewUpdate {
+			return
+		}
 
 		currentCtx := gui.State.ContextMgr.Current()
 		if currentCtx.GetKey() == context.NORMAL_MAIN_CONTEXT_KEY || currentCtx.GetKey() == context.NORMAL_SECONDARY_CONTEXT_KEY {