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