Skip to content

Commit e46dc1e

Browse files
committed
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.
1 parent 2051733 commit e46dc1e

File tree

2 files changed

+66
-88
lines changed

2 files changed

+66
-88
lines changed

pkg/commands/git_commands/commit_loader.go

Lines changed: 37 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212
"sync"
1313

14+
"github.com/jesseduffield/generics/set"
1415
"github.com/jesseduffield/lazygit/pkg/commands/models"
1516
"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
1617
"github.com/jesseduffield/lazygit/pkg/common"
@@ -98,23 +99,25 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
9899
}
99100
})
100101

101-
var ancestor string
102-
var remoteAncestor string
102+
var unmergedCommitHashes *set.Set[string]
103+
var remoteUnmergedCommitHashes *set.Set[string]
104+
mainBranches := opts.MainBranches.Get()
105+
103106
go utils.Safe(func() {
104107
defer wg.Done()
105108

106-
ancestor = opts.MainBranches.GetMergeBase(opts.RefName)
107-
if opts.RefToShowDivergenceFrom != "" {
108-
remoteAncestor = opts.MainBranches.GetMergeBase(opts.RefToShowDivergenceFrom)
109+
if len(mainBranches) > 0 {
110+
unmergedCommitHashes = self.getReachableHashes(opts.RefName, mainBranches)
111+
if opts.RefToShowDivergenceFrom != "" {
112+
remoteUnmergedCommitHashes = self.getReachableHashes(opts.RefToShowDivergenceFrom, mainBranches)
113+
}
109114
}
110115
})
111116

112-
passedFirstPushedCommit := false
113-
// I can get this before
114-
firstPushedCommit, err := self.getFirstPushedCommit(opts.RefForPushedStatus)
115-
if err != nil || firstPushedCommit == "" {
116-
// must have no upstream branch so we'll consider everything as pushed
117-
passedFirstPushedCommit = true
117+
var unpushedCommitHashes *set.Set[string]
118+
if opts.RefForPushedStatus != nil {
119+
unpushedCommitHashes = self.getReachableHashes(opts.RefForPushedStatus.FullRefName(),
120+
append([]string{opts.RefForPushedStatus.RefName() + "@{u}"}, mainBranches...))
118121
}
119122

120123
wg.Wait()
@@ -123,19 +126,6 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
123126
return nil, logErr
124127
}
125128

126-
for _, commit := range commits {
127-
if commit.Hash() == firstPushedCommit {
128-
passedFirstPushedCommit = true
129-
}
130-
if !commit.IsTODO() {
131-
if passedFirstPushedCommit {
132-
commit.Status = models.StatusPushed
133-
} else {
134-
commit.Status = models.StatusUnpushed
135-
}
136-
}
137-
}
138-
139129
if len(commits) == 0 {
140130
return commits, nil
141131
}
@@ -153,10 +143,10 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit,
153143
localSectionStart = len(commits)
154144
}
155145

156-
setCommitMergedStatuses(remoteAncestor, commits[:localSectionStart])
157-
setCommitMergedStatuses(ancestor, commits[localSectionStart:])
146+
setCommitStatuses(unpushedCommitHashes, remoteUnmergedCommitHashes, commits[:localSectionStart])
147+
setCommitStatuses(unpushedCommitHashes, unmergedCommitHashes, commits[localSectionStart:])
158148
} else {
159-
setCommitMergedStatuses(ancestor, commits)
149+
setCommitStatuses(unpushedCommitHashes, unmergedCommitHashes, commits)
160150
}
161151

162152
return commits, nil
@@ -549,56 +539,40 @@ func (self *CommitLoader) getConflictedSequencerCommit(hashPool *utils.StringPoo
549539
})
550540
}
551541

552-
func setCommitMergedStatuses(ancestor string, commits []*models.Commit) {
553-
if ancestor == "" {
554-
return
555-
}
556-
557-
passedAncestor := false
542+
func setCommitStatuses(unpushedCommitHashes *set.Set[string], unmergedCommitHashes *set.Set[string], commits []*models.Commit) {
558543
for i, commit := range commits {
559-
// some commits aren't really commits and don't have hashes, such as the update-ref todo
560-
if commit.Hash() != "" && strings.HasPrefix(ancestor, commit.Hash()) {
561-
passedAncestor = true
562-
}
563-
if commit.Status != models.StatusPushed && commit.Status != models.StatusUnpushed {
544+
if commit.IsTODO() {
564545
continue
565546
}
566-
if passedAncestor {
547+
548+
if unmergedCommitHashes == nil || unmergedCommitHashes.Includes(commit.Hash()) {
549+
if unpushedCommitHashes != nil && unpushedCommitHashes.Includes(commit.Hash()) {
550+
commits[i].Status = models.StatusUnpushed
551+
} else {
552+
commits[i].Status = models.StatusPushed
553+
}
554+
} else {
567555
commits[i].Status = models.StatusMerged
568556
}
569557
}
570558
}
571559

572-
func ignoringWarnings(commandOutput string) string {
573-
trimmedOutput := strings.TrimSpace(commandOutput)
574-
split := strings.Split(trimmedOutput, "\n")
575-
// need to get last line in case the first line is a warning about how the error is ambiguous.
576-
// At some point we should find a way to make it unambiguous
577-
lastLine := split[len(split)-1]
578-
579-
return lastLine
580-
}
581-
582-
// getFirstPushedCommit returns the first commit hash which has been pushed to the ref's upstream.
583-
// all commits above this are deemed unpushed and marked as such.
584-
func (self *CommitLoader) getFirstPushedCommit(ref models.Ref) (string, error) {
585-
if ref == nil {
586-
return "", nil
587-
}
588-
589-
output, err := self.cmd.New(
590-
NewGitCmd("merge-base").
591-
Arg(ref.FullRefName()).
592-
Arg(ref.RefName() + "@{u}").
560+
func (self *CommitLoader) getReachableHashes(refName string, notRefNames []string) *set.Set[string] {
561+
output, _, err := self.cmd.New(
562+
NewGitCmd("rev-list").
563+
Arg(refName).
564+
Arg(lo.Map(notRefNames, func(name string, _ int) string {
565+
return "^" + name
566+
})...).
593567
ToArgv(),
594568
).
595569
DontLog().
596-
RunWithOutput()
570+
RunWithOutputs()
597571
if err != nil {
598-
return "", err
572+
return set.New[string]()
599573
}
600574

601-
return ignoringWarnings(output), nil
575+
return set.NewFromSlice(utils.SplitLines(output))
602576
}
603577

604578
// getLog gets the git log.

pkg/commands/git_commands/commit_loader_test.go

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/go-errors/errors"
9+
"github.com/jesseduffield/generics/set"
910
"github.com/jesseduffield/lazygit/pkg/commands/models"
1011
"github.com/jesseduffield/lazygit/pkg/commands/oscommands"
1112
"github.com/jesseduffield/lazygit/pkg/common"
@@ -43,7 +44,7 @@ func TestGetCommits(t *testing.T) {
4344
logOrder: "topo-order",
4445
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
4546
runner: oscommands.NewFakeRunner(t).
46-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
47+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil).
4748
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),
4849

4950
expectedCommitOpts: []models.NewCommitOpts{},
@@ -54,7 +55,7 @@ func TestGetCommits(t *testing.T) {
5455
logOrder: "topo-order",
5556
opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
5657
runner: oscommands.NewFakeRunner(t).
57-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
58+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil).
5859
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),
5960

6061
expectedCommitOpts: []models.NewCommitOpts{},
@@ -67,7 +68,7 @@ func TestGetCommits(t *testing.T) {
6768
mainBranches: []string{"master", "main", "develop"},
6869
runner: oscommands.NewFakeRunner(t).
6970
// here it's seeing which commits are yet to be pushed
70-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
71+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}", "^refs/remotes/origin/master", "^refs/remotes/origin/main"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil).
7172
// here it's actually getting all the commits in a formatted form, one per line
7273
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).
7374
// here it's testing which of the configured main branches have an upstream
@@ -77,8 +78,9 @@ func TestGetCommits(t *testing.T) {
7778
ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead
7879
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
7980
ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/develop"}, "", errors.New("error")). // no local branch either
80-
// here it's seeing where our branch diverged from the master branch so that we can mark that commit and parent commits as 'merged'
81-
ExpectGitArgs([]string{"merge-base", "HEAD", "refs/remotes/origin/master", "refs/remotes/origin/main"}, "26c07b1ab33860a1a7591a0638f9925ccf497ffa", nil),
81+
// here it's seeing which of our commits are not on any of the main branches yet
82+
ExpectGitArgs([]string{"rev-list", "HEAD", "^refs/remotes/origin/master", "^refs/remotes/origin/main"},
83+
"0eea75e8c631fba6b58135697835d58ba4c18dbc\nb21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164\ne94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c\nd8084cd558925eb7c9c38afeed5725c21653ab90\n65f910ebd85283b5cce9bf67d03d3f1a9ea3813a\n", nil),
8284

8385
expectedCommitOpts: []models.NewCommitOpts{
8486
{
@@ -197,13 +199,13 @@ func TestGetCommits(t *testing.T) {
197199
expectedError: nil,
198200
},
199201
{
200-
testName: "should not call merge-base for mainBranches if none exist",
202+
testName: "should not call rev-list for mainBranches if none exist",
201203
logOrder: "topo-order",
202204
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
203205
mainBranches: []string{"master", "main"},
204206
runner: oscommands.NewFakeRunner(t).
205207
// here it's seeing which commits are yet to be pushed
206-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
208+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil).
207209
// here it's actually getting all the commits in a formatted form, one per line
208210
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).
209211
// here it's testing which of the configured main branches exist; neither does
@@ -233,13 +235,13 @@ func TestGetCommits(t *testing.T) {
233235
expectedError: nil,
234236
},
235237
{
236-
testName: "should call merge-base for all main branches that exist",
238+
testName: "should call rev-list with all main branches that exist",
237239
logOrder: "topo-order",
238240
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
239241
mainBranches: []string{"master", "main", "develop", "1.0-hotfixes"},
240242
runner: oscommands.NewFakeRunner(t).
241243
// here it's seeing which commits are yet to be pushed
242-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
244+
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).
243245
// here it's actually getting all the commits in a formatted form, one per line
244246
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).
245247
// here it's testing which of the configured main branches exist
@@ -249,8 +251,8 @@ func TestGetCommits(t *testing.T) {
249251
ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/heads/main"}, "", errors.New("error")).
250252
ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "develop@{u}"}, "refs/remotes/origin/develop", nil).
251253
ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "1.0-hotfixes@{u}"}, "refs/remotes/origin/1.0-hotfixes", nil).
252-
// here it's seeing where our branch diverged from the master branch so that we can mark that commit and parent commits as 'merged'
253-
ExpectGitArgs([]string{"merge-base", "HEAD", "refs/remotes/origin/master", "refs/remotes/origin/develop", "refs/remotes/origin/1.0-hotfixes"}, "26c07b1ab33860a1a7591a0638f9925ccf497ffa", nil),
254+
// here it's seeing which of our commits are not on any of the main branches yet
255+
ExpectGitArgs([]string{"rev-list", "HEAD", "^refs/remotes/origin/master", "^refs/remotes/origin/develop", "^refs/remotes/origin/1.0-hotfixes"}, "0eea75e8c631fba6b58135697835d58ba4c18dbc\n", nil),
254256

255257
expectedCommitOpts: []models.NewCommitOpts{
256258
{
@@ -275,7 +277,7 @@ func TestGetCommits(t *testing.T) {
275277
logOrder: "default",
276278
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, IncludeRebaseCommits: false},
277279
runner: oscommands.NewFakeRunner(t).
278-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
280+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil).
279281
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),
280282

281283
expectedCommitOpts: []models.NewCommitOpts{},
@@ -286,7 +288,7 @@ func TestGetCommits(t *testing.T) {
286288
logOrder: "default",
287289
opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: &models.Branch{Name: "mybranch"}, FilterPath: "src"},
288290
runner: oscommands.NewFakeRunner(t).
289-
ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil).
291+
ExpectGitArgs([]string{"rev-list", "refs/heads/mybranch", "^mybranch@{u}"}, "", nil).
290292
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),
291293

292294
expectedCommitOpts: []models.NewCommitOpts{},
@@ -536,37 +538,39 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
536538
}
537539
}
538540

539-
func TestCommitLoader_setCommitMergedStatuses(t *testing.T) {
541+
func TestCommitLoader_setCommitStatuses(t *testing.T) {
540542
type scenario struct {
541543
testName string
542544
commitOpts []models.NewCommitOpts
543-
ancestor string
545+
unmergedCommits []string
546+
unpushedCommits []string
544547
expectedCommitOpts []models.NewCommitOpts
545548
}
546549

547550
scenarios := []scenario{
548551
{
549552
testName: "basic",
550553
commitOpts: []models.NewCommitOpts{
551-
{Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed},
552-
{Hash: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusPushed},
553-
{Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed},
554+
{Hash: "12345", Name: "1", Action: models.ActionNone},
555+
{Hash: "67890", Name: "2", Action: models.ActionNone},
556+
{Hash: "abcde", Name: "3", Action: models.ActionNone},
554557
},
555-
ancestor: "67890",
558+
unmergedCommits: []string{"12345"},
556559
expectedCommitOpts: []models.NewCommitOpts{
557-
{Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed},
560+
{Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusPushed},
558561
{Hash: "67890", Name: "2", Action: models.ActionNone, Status: models.StatusMerged},
559562
{Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusMerged},
560563
},
561564
},
562565
{
563566
testName: "with update-ref",
564567
commitOpts: []models.NewCommitOpts{
565-
{Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed},
566-
{Hash: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone},
567-
{Hash: "abcde", Name: "3", Action: models.ActionNone, Status: models.StatusPushed},
568+
{Hash: "12345", Name: "1", Action: models.ActionNone},
569+
{Hash: "", Name: "", Action: todo.UpdateRef},
570+
{Hash: "abcde", Name: "3", Action: models.ActionNone},
568571
},
569-
ancestor: "deadbeef",
572+
unmergedCommits: []string{"12345", "abcde"},
573+
unpushedCommits: []string{"12345"},
570574
expectedCommitOpts: []models.NewCommitOpts{
571575
{Hash: "12345", Name: "1", Action: models.ActionNone, Status: models.StatusUnpushed},
572576
{Hash: "", Name: "", Action: todo.UpdateRef, Status: models.StatusNone},
@@ -581,7 +585,7 @@ func TestCommitLoader_setCommitMergedStatuses(t *testing.T) {
581585

582586
commits := lo.Map(scenario.commitOpts,
583587
func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) })
584-
setCommitMergedStatuses(scenario.ancestor, commits)
588+
setCommitStatuses(set.NewFromSlice(scenario.unpushedCommits), set.NewFromSlice(scenario.unmergedCommits), commits)
585589
expectedCommits := lo.Map(scenario.expectedCommitOpts,
586590
func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) })
587591
assert.Equal(t, expectedCommits, commits)

0 commit comments

Comments
 (0)