From a2a8755a7cba4a20eaf41233055724b84d8eed34 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Fri, 2 Jun 2023 18:05:56 +1000 Subject: [PATCH] Add commit store I want an efficient way to get information about the commits of a repo. So far, our Commit model contains a mix of immutable and mutable fields, and this means we need to throw out commits whenever we refresh, because one of the mutable fields may have changed. The commit store will store a new model, ImmutableCommit which never changes so we can just continue adding commits to the store without worrying about invalidating any of it. One use case for this store is the ability to determine if one commit is an ancestor of another, which will help us colour the commits against each of our branches in the local branches view. Without an in-memory store, we would need to make one git call per commit which would be super slow. If this store proves useful, we could switch to using it as the source of truth for our commits, with mutable stuff handled separately. --- pkg/commands/git.go | 3 + .../git_commands/commit_store_loader.go | 65 +++++++ .../git_commands/commit_store_loader_test.go | 99 +++++++++++ pkg/commands/models/commit.go | 16 +- pkg/commands/models/commit_store.go | 164 ++++++++++++++++++ pkg/commands/models/commit_store_test.go | 88 ++++++++++ pkg/commands/models/immutable_commit.go | 30 ++++ pkg/gui/controllers/helpers/refresh_helper.go | 18 +- pkg/gui/gui.go | 2 + pkg/gui/types/common.go | 2 + 10 files changed, 479 insertions(+), 8 deletions(-) create mode 100644 pkg/commands/git_commands/commit_store_loader.go create mode 100644 pkg/commands/git_commands/commit_store_loader_test.go create mode 100644 pkg/commands/models/commit_store.go create mode 100644 pkg/commands/models/commit_store_test.go create mode 100644 pkg/commands/models/immutable_commit.go diff --git a/pkg/commands/git.go b/pkg/commands/git.go index d09ff88b0..925849d24 100644 --- a/pkg/commands/git.go +++ b/pkg/commands/git.go @@ -45,6 +45,7 @@ type Loaders struct { BranchLoader *git_commands.BranchLoader CommitFileLoader *git_commands.CommitFileLoader CommitLoader *git_commands.CommitLoader + CommitStoreLoader *git_commands.CommitStoreLoader FileLoader *git_commands.FileLoader ReflogCommitLoader *git_commands.ReflogCommitLoader RemoteLoader *git_commands.RemoteLoader @@ -131,6 +132,7 @@ func NewGitCommandAux( branchLoader := git_commands.NewBranchLoader(cmn, cmd, branchCommands.CurrentBranchInfo, configCommands) commitFileLoader := git_commands.NewCommitFileLoader(cmn, cmd) commitLoader := git_commands.NewCommitLoader(cmn, cmd, dotGitDir, statusCommands.RebaseMode) + commitStoreLoader := git_commands.NewCommitStoreLoader(cmn, cmd) reflogCommitLoader := git_commands.NewReflogCommitLoader(cmn, cmd) remoteLoader := git_commands.NewRemoteLoader(cmn, cmd, repo.Remotes) stashLoader := git_commands.NewStashLoader(cmn, cmd) @@ -158,6 +160,7 @@ func NewGitCommandAux( BranchLoader: branchLoader, CommitFileLoader: commitFileLoader, CommitLoader: commitLoader, + CommitStoreLoader: commitStoreLoader, FileLoader: fileLoader, ReflogCommitLoader: reflogCommitLoader, RemoteLoader: remoteLoader, diff --git a/pkg/commands/git_commands/commit_store_loader.go b/pkg/commands/git_commands/commit_store_loader.go new file mode 100644 index 000000000..14f2d6f8d --- /dev/null +++ b/pkg/commands/git_commands/commit_store_loader.go @@ -0,0 +1,65 @@ +package git_commands + +import ( + "strings" + "time" + + "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/jesseduffield/lazygit/pkg/common" +) + +// CommitStoreLoader populates a commit store with commits from the git log. +type CommitStoreLoader struct { + *common.Common + cmd oscommands.ICmdObjBuilder +} + +func NewCommitStoreLoader( + cmn *common.Common, + cmd oscommands.ICmdObjBuilder, +) *CommitStoreLoader { + return &CommitStoreLoader{ + Common: cmn, + cmd: cmd, + } +} + +// mutates the given commit store to add commits from the git log +func (self *CommitStoreLoader) Load(commitStore *models.CommitStore) error { + t := time.Now() + + err := self.getLogCmd().RunAndProcessLines(func(line string) (bool, error) { + commit := self.extractCommitFromLine(line) + commitStore.Add(commit) + return false, nil + }) + + self.Log.Warnf("CommitStoreLoader Load took %s", time.Since(t)) + + return err +} + +// getLog gets the git log. +func (self *CommitStoreLoader) getLogCmd() oscommands.ICmdObj { + cmdArgs := NewGitCmd("log"). + Arg("--all"). + Arg(`--pretty=format:%H%x00%P`). + ToArgv() + + return self.cmd.New(cmdArgs).DontLog() +} + +func (self *CommitStoreLoader) extractCommitFromLine(line string) models.ImmutableCommit { + split := strings.SplitN(line, "\x00", 2) + + sha := split[0] + + parentsStr := split[1] + parents := []string{} + if len(parentsStr) > 0 { + parents = strings.Split(parentsStr, " ") + } + + return models.NewImmutableCommit(sha, parents) +} diff --git a/pkg/commands/git_commands/commit_store_loader_test.go b/pkg/commands/git_commands/commit_store_loader_test.go new file mode 100644 index 000000000..f0697ea8f --- /dev/null +++ b/pkg/commands/git_commands/commit_store_loader_test.go @@ -0,0 +1,99 @@ +package git_commands + +import ( + "strings" + "testing" + + "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stretchr/testify/assert" +) + +func TestCommitStoreLoaderLoad(t *testing.T) { + commitsOutputStore := strings.Replace(`a|b +b|c d +c|e +d|e +e|`, "|", "\x00", -1) + + args := []string{"log", "--all", "--pretty=format:%H%x00%P"} + + type scenario struct { + testName string + runner *oscommands.FakeCmdObjRunner + startingCommits []models.ImmutableCommit + expectedCommits []models.ImmutableCommit + expectedError error + } + + scenarios := []scenario{ + { + testName: "should return no commits if there are none", + runner: oscommands.NewFakeRunner(t). + ExpectGitArgs(args, "", nil), + + startingCommits: []models.ImmutableCommit{}, + expectedCommits: []models.ImmutableCommit{}, + expectedError: nil, + }, + { + testName: "properly processes commits", + runner: oscommands.NewFakeRunner(t). + ExpectGitArgs(args, commitsOutputStore, nil), + + startingCommits: []models.ImmutableCommit{}, + expectedCommits: []models.ImmutableCommit{ + models.NewImmutableCommit("a", []string{"b"}), + models.NewImmutableCommit("b", []string{"c", "d"}), + models.NewImmutableCommit("c", []string{"e"}), + models.NewImmutableCommit("d", []string{"e"}), + models.NewImmutableCommit("e", []string{}), + }, + expectedError: nil, + }, + { + testName: "merges into exising commits", + runner: oscommands.NewFakeRunner(t). + ExpectGitArgs(args, commitsOutputStore, nil), + + startingCommits: []models.ImmutableCommit{ + // this one is present in the command output + models.NewImmutableCommit("a", []string{"b"}), + // this one isn't + models.NewImmutableCommit("f", []string{"g"}), + }, + expectedCommits: []models.ImmutableCommit{ + models.NewImmutableCommit("a", []string{"b"}), + models.NewImmutableCommit("b", []string{"c", "d"}), + models.NewImmutableCommit("c", []string{"e"}), + models.NewImmutableCommit("d", []string{"e"}), + models.NewImmutableCommit("e", []string{}), + models.NewImmutableCommit("f", []string{"g"}), + }, + expectedError: nil, + }, + } + + for _, scenario := range scenarios { + scenario := scenario + t.Run(scenario.testName, func(t *testing.T) { + common := utils.NewDummyCommon() + + builder := &CommitStoreLoader{ + Common: common, + cmd: oscommands.NewDummyCmdObjBuilder(scenario.runner), + } + + commitStore := models.NewCommitStore() + commitStore.AddSlice(scenario.startingCommits) + + err := builder.Load(commitStore) + + assert.EqualValues(t, scenario.expectedCommits, commitStore.Slice()) + assert.Equal(t, scenario.expectedError, err) + + scenario.runner.CheckForMissingCalls() + }) + } +} diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index e0ff875a0..da62a8778 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -18,7 +18,6 @@ const ( StatusPushed StatusMerged StatusRebasing - StatusSelected StatusReflog ) @@ -30,18 +29,21 @@ const ( // Commit : A git commit type Commit struct { - Sha string + // TODO: rename to hash + Sha string + // Commit message subject line Name string - Status CommitStatus - Action todo.TodoCommand - Tags []string - ExtraInfo string // something like 'HEAD -> master, tag: v0.15.2' AuthorName string // something like 'Jesse Duffield' AuthorEmail string // something like 'jessedduffield@gmail.com' UnixTimestamp int64 - // SHAs of parent commits (will be multiple if it's a merge commit) Parents []string + + // mutable fields + Status CommitStatus + Action todo.TodoCommand + Tags []string + ExtraInfo string // something like 'HEAD -> master, tag: v0.15.2' } func (c *Commit) ShortSha() string { diff --git a/pkg/commands/models/commit_store.go b/pkg/commands/models/commit_store.go new file mode 100644 index 000000000..72e972226 --- /dev/null +++ b/pkg/commands/models/commit_store.go @@ -0,0 +1,164 @@ +package models + +import ( + "sync" + + "github.com/jesseduffield/generics/slices" + "github.com/samber/lo" +) + +// This stores immutable commits as a graph for fast lookup +type CommitStore struct { + commitsMap map[string]ImmutableCommit + mutex *sync.RWMutex +} + +func NewCommitStore() *CommitStore { + return &CommitStore{commitsMap: make(map[string]ImmutableCommit), mutex: &sync.RWMutex{}} +} + +func (self *CommitStore) Add(commit ImmutableCommit) { + self.mutex.Lock() + defer self.mutex.Unlock() + + self.commitsMap[commit.Hash()] = commit +} + +func (self *CommitStore) AddSlice(commit []ImmutableCommit) { + self.mutex.Lock() + defer self.mutex.Unlock() + + for _, commit := range commit { + self.commitsMap[commit.Hash()] = commit + } +} + +func (self *CommitStore) GetCommit(hash string) (ImmutableCommit, bool) { + self.mutex.RLock() + defer self.mutex.RUnlock() + + commit, ok := self.commitsMap[hash] + return commit, ok +} + +func (self *CommitStore) GetParents(hash string) ([]ImmutableCommit, bool) { + commit, ok := self.GetCommit(hash) + if !ok { + return nil, false + } + + parents := make([]ImmutableCommit, len(commit.ParentHashes())) + for i, parentHash := range commit.ParentHashes() { + parents[i], ok = self.GetCommit(parentHash) + if !ok { + return nil, false + } + } + + return parents, true +} + +type IsAncestorResponse int + +const ( + IsAncestorResponseYes IsAncestorResponse = iota + IsAncestorResponseNo + // returned when we can't find the commit, or if one of the ancestors along the chain can't be found + IsAncestorResponseUnknown +) + +var IsAncestorResponseStrings = []string{ + "yes", + "no", + "unknown", +} + +func (self *CommitStore) IsAncestor(hash string, ancestorHash string) IsAncestorResponse { + if hash == ancestorHash { + return IsAncestorResponseYes + } + + commit, ok := self.GetCommit(hash) + if !ok { + return IsAncestorResponseUnknown + } + + if commit.IsRoot() { + return IsAncestorResponseNo + } + + parentHashes := commit.ParentHashes() + + // first check the parent hashes themselves: spares us attempting a lookup of the actual parent structs + for _, parentHash := range parentHashes { + if parentHash == ancestorHash { + return IsAncestorResponseYes + } + } + + unknown := false + for _, parentHash := range parentHashes { + response := self.IsAncestor(parentHash, ancestorHash) + if response == IsAncestorResponseYes { + return IsAncestorResponseYes + } + if response == IsAncestorResponseUnknown { + unknown = true + } + } + + if unknown { + return IsAncestorResponseUnknown + } + + return IsAncestorResponseNo +} + +// used for testing +func (self *CommitStore) Slice() []ImmutableCommit { + self.mutex.RLock() + defer self.mutex.RUnlock() + + commits := lo.Values(self.commitsMap) + + // sort by hash for deterministic result + slices.SortFunc(commits, func(a, b ImmutableCommit) bool { + return a.Hash() < b.Hash() + }) + + return commits +} + +func (self *CommitStore) Size() int { + return len(self.commitsMap) +} + +func (self *CommitStore) dfs(hash string, visited map[string]bool) { + visited[hash] = true + commit, ok := self.GetCommit(hash) + if !ok { + return + } + + // Traverse the parent hashes + for _, parentHash := range commit.ParentHashes() { + if !visited[parentHash] { + self.dfs(parentHash, visited) + } + } +} + +// returns subset of candidates which are ancestors of the given hash +func (self *CommitStore) FindAncestors(hash string, candidates []string) map[string]bool { + visited := make(map[string]bool) + self.dfs(hash, visited) + + ancestors := map[string]bool{} + for _, commitHash := range candidates { + if visited[commitHash] { + ancestors[commitHash] = true + } + } + + return ancestors +} diff --git a/pkg/commands/models/commit_store_test.go b/pkg/commands/models/commit_store_test.go new file mode 100644 index 000000000..1142c25d7 --- /dev/null +++ b/pkg/commands/models/commit_store_test.go @@ -0,0 +1,88 @@ +package models + +import ( + "testing" +) + +func TestCommitStore(t *testing.T) { + scenarios := []struct { + testName string + commits []ImmutableCommit + hash string + ancestorHash string + expected IsAncestorResponse + }{ + { + testName: "Empty commit store, same hash", + commits: []ImmutableCommit{}, + hash: "a", + ancestorHash: "a", + expected: IsAncestorResponseYes, + }, + { + testName: "Empty commit store, different hash", + commits: []ImmutableCommit{}, + hash: "a", + ancestorHash: "b", + expected: IsAncestorResponseUnknown, + }, + { + testName: "Hash found, ancestor not found", + commits: []ImmutableCommit{ + NewImmutableCommit("a", []string{"c"}), + NewImmutableCommit("c", []string{}), + }, + hash: "a", + ancestorHash: "b", + expected: IsAncestorResponseNo, + }, + { + testName: "Hash found, ancestor is parent", + commits: []ImmutableCommit{ + NewImmutableCommit("a", []string{"b"}), + NewImmutableCommit("b", []string{"c"}), + }, + hash: "a", + ancestorHash: "b", + expected: IsAncestorResponseYes, + }, + { + testName: "Hash found, ancestor is grandparent", + commits: []ImmutableCommit{ + NewImmutableCommit("a", []string{"b"}), + NewImmutableCommit("b", []string{"c"}), + }, + hash: "a", + ancestorHash: "c", + expected: IsAncestorResponseYes, + }, + { + testName: "Hash found, not an ancestor", + commits: []ImmutableCommit{ + NewImmutableCommit("a", []string{"b"}), + NewImmutableCommit("b", []string{"d"}), + NewImmutableCommit("c", []string{"d"}), + NewImmutableCommit("d", []string{}), + }, + hash: "a", + ancestorHash: "c", + expected: IsAncestorResponseNo, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.testName, func(t *testing.T) { + commitStore := NewCommitStore() + commitStore.AddSlice(scenario.commits) + + response := commitStore.IsAncestor(scenario.hash, scenario.ancestorHash) + + if response != scenario.expected { + responseStr := IsAncestorResponseStrings[response] + expectedStr := IsAncestorResponseStrings[scenario.expected] + + t.Errorf("Expected %s, got %s", expectedStr, responseStr) + } + }) + } +} diff --git a/pkg/commands/models/immutable_commit.go b/pkg/commands/models/immutable_commit.go new file mode 100644 index 000000000..90ea30826 --- /dev/null +++ b/pkg/commands/models/immutable_commit.go @@ -0,0 +1,30 @@ +package models + +// This model contains the information that is intrinsic to a commit, meaning +// that we can depend on it not changing over time. Git commits are immutable, +// but our other Commit model has extra fields added for ease of use. +type ImmutableCommit struct { + hash string + + // hashes of parent commits (will be multiple if it's a merge commit) + parentHashes []string +} + +func NewImmutableCommit(hash string, parentHashes []string) ImmutableCommit { + return ImmutableCommit{ + hash: hash, + parentHashes: parentHashes, + } +} + +func (self *ImmutableCommit) Hash() string { + return self.hash +} + +func (self *ImmutableCommit) ParentHashes() []string { + return self.parentHashes +} + +func (self *ImmutableCommit) IsRoot() bool { + return len(self.parentHashes) == 0 +} diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 7b37585a5..59476ed8f 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -222,7 +222,7 @@ func (self *RefreshHelper) refreshReflogCommitsConsideringStartup() { // e.g. in the case of switching branches. func (self *RefreshHelper) refreshCommits() { wg := sync.WaitGroup{} - wg.Add(2) + wg.Add(3) go utils.Safe(func() { self.refreshReflogCommitsConsideringStartup() @@ -231,6 +231,12 @@ func (self *RefreshHelper) refreshCommits() { wg.Done() }) + go utils.Safe(func() { + self.refreshCommitStore() + + wg.Done() + }) + go utils.Safe(func() { _ = self.refreshCommitsWithLimit() ctx, ok := self.c.Contexts().CommitFiles.GetParentContext() @@ -254,6 +260,16 @@ func (self *RefreshHelper) refreshCommits() { wg.Wait() } +func (self *RefreshHelper) refreshCommitStore() { + self.c.Mutexes().CommitStoreMutex.Lock() + defer self.c.Mutexes().CommitStoreMutex.Unlock() + + err := self.c.Git().Loaders.CommitStoreLoader.Load(self.c.Model().CommitStore) + if err != nil { + _ = self.c.Error(err) + } +} + func (self *RefreshHelper) refreshCommitsWithLimit() error { self.c.Mutexes().LocalCommitsMutex.Lock() defer self.c.Mutexes().LocalCommitsMutex.Unlock() diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index b429bda27..776e02daf 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -342,6 +342,7 @@ func (gui *Gui) resetState(startArgs appTypes.StartArgs, reuseState bool) types. CommitFiles: nil, Files: make([]*models.File, 0), Commits: make([]*models.Commit, 0), + CommitStore: models.NewCommitStore(), StashEntries: make([]*models.StashEntry, 0), FilteredReflogCommits: make([]*models.Commit, 0), ReflogCommits: make([]*models.Commit, 0), @@ -451,6 +452,7 @@ func NewGui( RefreshingStatusMutex: &deadlock.Mutex{}, SyncMutex: &deadlock.Mutex{}, LocalCommitsMutex: &deadlock.Mutex{}, + CommitStoreMutex: &deadlock.Mutex{}, SubCommitsMutex: &deadlock.Mutex{}, SubprocessMutex: &deadlock.Mutex{}, PopupMutex: &deadlock.Mutex{}, diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 3709d3c7b..2be83db39 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -192,6 +192,7 @@ type Model struct { Submodules []*models.SubmoduleConfig Branches []*models.Branch Commits []*models.Commit + CommitStore *models.CommitStore StashEntries []*models.StashEntry SubCommits []*models.Commit Remotes []*models.Remote @@ -220,6 +221,7 @@ type Mutexes struct { RefreshingStatusMutex *deadlock.Mutex SyncMutex *deadlock.Mutex LocalCommitsMutex *deadlock.Mutex + CommitStoreMutex *deadlock.Mutex SubCommitsMutex *deadlock.Mutex SubprocessMutex *deadlock.Mutex PopupMutex *deadlock.Mutex