From e46dc1ead637857c0b4fe60dc72a8fe24bebe092 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 30 Jul 2025 15:07:15 +0200 Subject: [PATCH] Use a better approach for determining pushed and merge statuses Previously we would call git merge-base with the upstream branch to determine where unpushed commits end and pushed commits start, and also git merge-base with the main branch(es) to see where the merged commits start. This worked ok in normal cases, but it had two problems: - when filtering by path or by author, those merge-base commits would usually not be part of the commit list, so we would miss the point where we should switch from unpushed to pushed, or from pushed to merged. The consequence was that in filtering mode, all commit hashes were always yellow. - when main was merged into a feature branch, we would color all commits from that merge on down in green, even ones that are only part of the feature branch but not main. To fix these problems, we switch our approach to one where we call git rev-list with the branch in question, with negative refspecs for the upstream branch and the main branches, respectively; this gives us the complete picture of which commits are pushed/unpushed/merged, so it also works in the cases described above. And funnily, even though intuitively it feels more expensive, it actually performs better than the merge-base calls (for normal usage scenarios at least), so the commit-loading part of refresh is faster now in general. We are talking about differences like 300ms before, 140ms after, in some unscientific measurements I took (depends a lot on repo sizes, branch length, etc.). An exception are degenerate cases like feature branches with hundreds of thousands of commits, which are slower now; but I don't think we need to worry about those too much. --- pkg/commands/git_commands/commit_loader.go | 100 +++++++----------- .../git_commands/commit_loader_test.go | 54 +++++----- 2 files changed, 66 insertions(+), 88 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 1d4314c5b..72adf1b69 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -11,6 +11,7 @@ import ( "strings" "sync" + "github.com/jesseduffield/generics/set" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/common" @@ -98,23 +99,25 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, } }) - var ancestor string - var remoteAncestor string + var unmergedCommitHashes *set.Set[string] + var remoteUnmergedCommitHashes *set.Set[string] + mainBranches := opts.MainBranches.Get() + go utils.Safe(func() { defer wg.Done() - ancestor = opts.MainBranches.GetMergeBase(opts.RefName) - if opts.RefToShowDivergenceFrom != "" { - remoteAncestor = opts.MainBranches.GetMergeBase(opts.RefToShowDivergenceFrom) + if len(mainBranches) > 0 { + unmergedCommitHashes = self.getReachableHashes(opts.RefName, mainBranches) + if opts.RefToShowDivergenceFrom != "" { + remoteUnmergedCommitHashes = self.getReachableHashes(opts.RefToShowDivergenceFrom, mainBranches) + } } }) - passedFirstPushedCommit := false - // I can get this before - firstPushedCommit, err := self.getFirstPushedCommit(opts.RefForPushedStatus) - if err != nil || firstPushedCommit == "" { - // must have no upstream branch so we'll consider everything as pushed - passedFirstPushedCommit = true + var unpushedCommitHashes *set.Set[string] + if opts.RefForPushedStatus != nil { + unpushedCommitHashes = self.getReachableHashes(opts.RefForPushedStatus.FullRefName(), + append([]string{opts.RefForPushedStatus.RefName() + "@{u}"}, mainBranches...)) } wg.Wait() @@ -123,19 +126,6 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, return nil, logErr } - for _, commit := range commits { - if commit.Hash() == firstPushedCommit { - passedFirstPushedCommit = true - } - if !commit.IsTODO() { - if passedFirstPushedCommit { - commit.Status = models.StatusPushed - } else { - commit.Status = models.StatusUnpushed - } - } - } - if len(commits) == 0 { return commits, nil } @@ -153,10 +143,10 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, localSectionStart = len(commits) } - setCommitMergedStatuses(remoteAncestor, commits[:localSectionStart]) - setCommitMergedStatuses(ancestor, commits[localSectionStart:]) + setCommitStatuses(unpushedCommitHashes, remoteUnmergedCommitHashes, commits[:localSectionStart]) + setCommitStatuses(unpushedCommitHashes, unmergedCommitHashes, commits[localSectionStart:]) } else { - setCommitMergedStatuses(ancestor, commits) + setCommitStatuses(unpushedCommitHashes, unmergedCommitHashes, commits) } return commits, nil @@ -549,56 +539,40 @@ func (self *CommitLoader) getConflictedSequencerCommit(hashPool *utils.StringPoo }) } -func setCommitMergedStatuses(ancestor string, commits []*models.Commit) { - if ancestor == "" { - return - } - - passedAncestor := false +func setCommitStatuses(unpushedCommitHashes *set.Set[string], unmergedCommitHashes *set.Set[string], commits []*models.Commit) { for i, commit := range commits { - // some commits aren't really commits and don't have hashes, such as the update-ref todo - if commit.Hash() != "" && strings.HasPrefix(ancestor, commit.Hash()) { - passedAncestor = true - } - if commit.Status != models.StatusPushed && commit.Status != models.StatusUnpushed { + if commit.IsTODO() { continue } - if passedAncestor { + + if unmergedCommitHashes == nil || unmergedCommitHashes.Includes(commit.Hash()) { + if unpushedCommitHashes != nil && unpushedCommitHashes.Includes(commit.Hash()) { + commits[i].Status = models.StatusUnpushed + } else { + commits[i].Status = models.StatusPushed + } + } else { commits[i].Status = models.StatusMerged } } } -func ignoringWarnings(commandOutput string) string { - trimmedOutput := strings.TrimSpace(commandOutput) - split := strings.Split(trimmedOutput, "\n") - // need to get last line in case the first line is a warning about how the error is ambiguous. - // At some point we should find a way to make it unambiguous - lastLine := split[len(split)-1] - - return lastLine -} - -// getFirstPushedCommit returns the first commit hash which has been pushed to the ref's upstream. -// all commits above this are deemed unpushed and marked as such. -func (self *CommitLoader) getFirstPushedCommit(ref models.Ref) (string, error) { - if ref == nil { - return "", nil - } - - output, err := self.cmd.New( - NewGitCmd("merge-base"). - Arg(ref.FullRefName()). - Arg(ref.RefName() + "@{u}"). +func (self *CommitLoader) getReachableHashes(refName string, notRefNames []string) *set.Set[string] { + output, _, err := self.cmd.New( + NewGitCmd("rev-list"). + Arg(refName). + Arg(lo.Map(notRefNames, func(name string, _ int) string { + return "^" + name + })...). ToArgv(), ). DontLog(). - RunWithOutput() + RunWithOutputs() if err != nil { - return "", err + return set.New[string]() } - return ignoringWarnings(output), nil + return set.NewFromSlice(utils.SplitLines(output)) } // getLog gets the git log. diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index cbf170e5b..7f9873b0b 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/go-errors/errors" + "github.com/jesseduffield/generics/set" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/common" @@ -43,7 +44,7 @@ func TestGetCommits(t *testing.T) { logOrder: "topo-order", opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil). ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, @@ -54,7 +55,7 @@ func TestGetCommits(t *testing.T) { logOrder: "topo-order", opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil). ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, @@ -67,7 +68,7 @@ func TestGetCommits(t *testing.T) { mainBranches: []string{"master", "main", "develop"}, runner: oscommands.NewFakeRunner(t). // here it's seeing which commits are yet to be pushed - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}", "^refs/remotes/origin/master", "^refs/remotes/origin/main"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil). // here it's actually getting all the commits in a formatted form, one per line ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). // here it's testing which of the configured main branches have an upstream @@ -77,8 +78,9 @@ func TestGetCommits(t *testing.T) { ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/develop"}, "", errors.New("error")). // doesn't exist there, either, so it checks for a local branch ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/develop"}, "", errors.New("error")). // no local branch either - // here it's seeing where our branch diverged from the master branch so that we can mark that commit and parent commits as 'merged' - ExpectGitArgs([]string{"merge-base", "HEAD", "refs/remotes/origin/master", "refs/remotes/origin/main"}, "26c07b1ab33860a1a7591a0638f9925ccf497ffa", nil), + // here it's seeing which of our commits are not on any of the main branches yet + ExpectGitArgs([]string{"rev-list", "HEAD", "^refs/remotes/origin/master", "^refs/remotes/origin/main"}, + "0eea75e8c631fba6b58135697835d58ba4c18dbc\nb21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164\ne94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c\nd8084cd558925eb7c9c38afeed5725c21653ab90\n65f910ebd85283b5cce9bf67d03d3f1a9ea3813a\n", nil), expectedCommitOpts: []models.NewCommitOpts{ { @@ -197,13 +199,13 @@ func TestGetCommits(t *testing.T) { expectedError: nil, }, { - testName: "should not call merge-base for mainBranches if none exist", + testName: "should not call rev-list for mainBranches if none exist", logOrder: "topo-order", opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false}, mainBranches: []string{"master", "main"}, runner: oscommands.NewFakeRunner(t). // here it's seeing which commits are yet to be pushed - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil). // here it's actually getting all the commits in a formatted form, one per line ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist; neither does @@ -233,13 +235,13 @@ func TestGetCommits(t *testing.T) { expectedError: nil, }, { - testName: "should call merge-base for all main branches that exist", + testName: "should call rev-list with all main branches that exist", logOrder: "topo-order", opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false}, mainBranches: []string{"master", "main", "develop", "1.0-hotfixes"}, runner: oscommands.NewFakeRunner(t). // here it's seeing which commits are yet to be pushed - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}", "^refs/remotes/origin/master", "^refs/remotes/origin/develop", "^refs/remotes/origin/1.0-hotfixes"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil). // here it's actually getting all the commits in a formatted form, one per line ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist @@ -249,8 +251,8 @@ func TestGetCommits(t *testing.T) { ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/main"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "refs/remotes/origin/develop", nil). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "1.0-hotfixes@{u}"}, "refs/remotes/origin/1.0-hotfixes", nil). - // here it's seeing where our branch diverged from the master branch so that we can mark that commit and parent commits as 'merged' - ExpectGitArgs([]string{"merge-base", "HEAD", "refs/remotes/origin/master", "refs/remotes/origin/develop", "refs/remotes/origin/1.0-hotfixes"}, "26c07b1ab33860a1a7591a0638f9925ccf497ffa", nil), + // here it's seeing which of our commits are not on any of the main branches yet + ExpectGitArgs([]string{"rev-list", "HEAD", "^refs/remotes/origin/master", "^refs/remotes/origin/develop", "^refs/remotes/origin/1.0-hotfixes"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil), expectedCommitOpts: []models.NewCommitOpts{ { @@ -275,7 +277,7 @@ func TestGetCommits(t *testing.T) { logOrder: "default", opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil). ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, @@ -286,7 +288,7 @@ func TestGetCommits(t *testing.T) { logOrder: "default", opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, FilterPath: "src"}, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). + ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil). ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%P%x00%m%x00%D%x00%s", "--abbrev=40", "--follow", "--name-status", "--no-show-signature", "--", "src"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, @@ -536,11 +538,12 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { } } -func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { +func TestCommitLoader_setCommitStatuses(t *testing.T) { type scenario struct { testName string commitOpts []models.NewCommitOpts - ancestor string + unmergedCommits []string + unpushedCommits []string expectedCommitOpts []models.NewCommitOpts } @@ -548,13 +551,13 @@ func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { { testName: "basic", commitOpts: []models.NewCommitOpts{ - {Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, - {Hash: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusPushed}, - {Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed}, + {Hash: "12345", Name: "1", Action: models.ActionNone}, + {Hash: "67890", Name: "2", Action: models.ActionNone}, + {Hash: "abcde", Name: "3", Action: models.ActionNone}, }, - ancestor: "67890", + unmergedCommits: []string{"12345"}, expectedCommitOpts: []models.NewCommitOpts{ - {Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, + {Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusPushed}, {Hash: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusMerged}, {Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusMerged}, }, @@ -562,11 +565,12 @@ func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { { testName: "with update-ref", commitOpts: []models.NewCommitOpts{ - {Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, - {Hash: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone}, - {Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed}, + {Hash: "12345", Name: "1", Action: models.ActionNone}, + {Hash: "", Name: "", Action: todo.UpdateRef}, + {Hash: "abcde", Name: "3", Action: models.ActionNone}, }, - ancestor: "deadbeef", + unmergedCommits: []string{"12345", "abcde"}, + unpushedCommits: []string{"12345"}, expectedCommitOpts: []models.NewCommitOpts{ {Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed}, {Hash: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone}, @@ -581,7 +585,7 @@ func TestCommitLoader_setCommitMergedStatuses(t *testing.T) { commits := lo.Map(scenario.commitOpts, func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) }) - setCommitMergedStatuses(scenario.ancestor, commits) + setCommitStatuses(set.NewFromSlice(scenario.unpushedCommits), set.NewFromSlice(scenario.unmergedCommits), commits) expectedCommits := lo.Map(scenario.expectedCommitOpts, func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) }) assert.Equal(t, expectedCommits, commits)