From eddbbdad236d3978ed9787561229338e2be4d7ba Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 29 Aug 2026 13:44:53 +0200 Subject: [PATCH] Avoid rehydrating unchanged rebase todos Narrow rebase refreshes already have complete metadata for existing todos. Reuse it so repeated todo moves do not spawn git show for the entire list. New hashes still fall back to hydration. --- pkg/commands/git_commands/commit_loader.go | 71 +++++++----- .../git_commands/commit_loader_test.go | 104 ++++++++++++++++++ 2 files changed, 148 insertions(+), 27 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 8b79bd8cd..381dd641e 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/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, diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 7f9873b0b..d26119720 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/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