From 09ce430240fdc348a7bdbd03318ce1ccb75c1c05 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 09:37:14 +1000 Subject: [PATCH 1/9] Log duration of refresh --- pkg/gui/controllers/helpers/refresh_helper.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 5a70414b6..cc0acd317 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" "sync" + "time" "github.com/jesseduffield/generics/set" "github.com/jesseduffield/generics/slices" @@ -51,6 +52,11 @@ func NewRefreshHelper( } func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { + t := time.Now() + defer func() { + self.c.Log.Infof(fmt.Sprintf("Refresh took %s", time.Since(t))) + }() + if options.Scope == nil { self.c.Log.Infof( "refreshing all scopes in %s mode", From 71cab4fadc84fff893aa797482b3436ea2faae8a Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 09:31:42 +1000 Subject: [PATCH 2/9] Log duration of commands This will help us diagnose performance issues --- pkg/commands/oscommands/cmd_obj_runner.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/commands/oscommands/cmd_obj_runner.go b/pkg/commands/oscommands/cmd_obj_runner.go index b82e3a25c..23ec47070 100644 --- a/pkg/commands/oscommands/cmd_obj_runner.go +++ b/pkg/commands/oscommands/cmd_obj_runner.go @@ -6,6 +6,7 @@ import ( "io" "regexp" "strings" + "time" "github.com/go-errors/errors" "github.com/jesseduffield/gocui" @@ -102,10 +103,14 @@ func (self *cmdObjRunner) RunWithOutputAux(cmdObj ICmdObj) (string, error) { self.logCmdObj(cmdObj) } + t := time.Now() output, err := sanitisedCommandOutput(cmdObj.GetCmd().CombinedOutput()) if err != nil { self.log.WithField("command", cmdObj.ToString()).Error(output) } + + self.log.Infof("%s (%s)", cmdObj.ToString(), time.Since(t)) + return output, err } @@ -116,12 +121,15 @@ func (self *cmdObjRunner) RunWithOutputsAux(cmdObj ICmdObj) (string, string, err self.logCmdObj(cmdObj) } + t := time.Now() var outBuffer, errBuffer bytes.Buffer cmd := cmdObj.GetCmd() cmd.Stdout = &outBuffer cmd.Stderr = &errBuffer err := cmd.Run() + self.log.Infof("%s (%s)", cmdObj.ToString(), time.Since(t)) + stdout := outBuffer.String() stderr, err := sanitisedCommandOutput(errBuffer.Bytes(), err) if err != nil { @@ -144,6 +152,7 @@ func (self *cmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(line st if cmdObj.ShouldLog() { self.logCmdObj(cmdObj) } + t := time.Now() cmd := cmdObj.GetCmd() stdoutPipe, err := cmd.StdoutPipe() @@ -171,6 +180,8 @@ func (self *cmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(line st _ = cmd.Wait() + self.log.Infof("%s (%s)", cmdObj.ToString(), time.Since(t)) + return nil } @@ -237,9 +248,14 @@ func (self *cmdObjRunner) runAndStreamAux( } }() + t := time.Now() + onRun(handler, cmdWriter) err = cmd.Wait() + + self.log.Infof("%s (%s)", cmdObj.ToString(), time.Since(t)) + if err != nil { errStr := stderr.String() if errStr != "" { From 5f30ccfbc3f560520c8eae50d7fbf138cdf4d0da Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 09:32:58 +1000 Subject: [PATCH 3/9] Log duration of post-refresh-update call Notably, the reflog view is taking ages here because it's got a few thousand lines to write to the view. In future we should only populate the view's viewport. --- pkg/gui/view_helpers.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 85ec21128..58c973712 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -1,6 +1,8 @@ package gui import ( + "time" + "github.com/jesseduffield/gocui" "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/tasks" @@ -129,6 +131,11 @@ func (gui *Gui) render() { // if the context's view is set to another context we do nothing. // if the context's view is the current view we trigger a focus; re-selecting the current item. func (gui *Gui) postRefreshUpdate(c types.Context) error { + t := time.Now() + defer func() { + gui.Log.Infof("postRefreshUpdate for %s took %s", c.GetKey(), time.Since(t)) + }() + if err := c.HandleRender(); err != nil { return err } From 5d8a85f7e78e49507e35c06a7e646b33f7018edb Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Fri, 28 Jul 2023 23:04:24 +1000 Subject: [PATCH 4/9] Use wait groups to speed up commit loading The speedup is most noticeable on first load, when we haven't yet fetched out main branches. I saw a speedup from 105ms to 60ms. On subsequent loads the gain is more modest; 54ms to 40ms --- pkg/commands/git_commands/commit_loader.go | 89 +++++++++++++++++----- 1 file changed, 68 insertions(+), 21 deletions(-) diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 59a704b27..7e304ea44 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -8,6 +8,7 @@ import ( "regexp" "strconv" "strings" + "sync" "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/generics/slices" @@ -15,6 +16,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" "github.com/jesseduffield/lazygit/pkg/common" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" ) @@ -84,31 +86,60 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, commits = append(commits, rebasingCommits...) } + wg := sync.WaitGroup{} + + wg.Add(2) + + var logErr error + go utils.Safe(func() { + defer wg.Done() + + logErr = self.getLogCmd(opts).RunAndProcessLines(func(line string) (bool, error) { + commit := self.extractCommitFromLine(line) + commits = append(commits, commit) + return false, nil + }) + }) + + var ancestor string + go utils.Safe(func() { + defer wg.Done() + + ancestor = self.getMergeBase(opts.RefName) + }) + passedFirstPushedCommit := false + // I can get this before firstPushedCommit, err := self.getFirstPushedCommit(opts.RefName) if err != nil { // must have no upstream branch so we'll consider everything as pushed passedFirstPushedCommit = true } - err = self.getLogCmd(opts).RunAndProcessLines(func(line string) (bool, error) { - commit := self.extractCommitFromLine(line) + wg.Wait() + + if logErr != nil { + return nil, logErr + } + + for _, commit := range commits { if commit.Sha == firstPushedCommit { passedFirstPushedCommit = true } - commit.Status = map[bool]models.CommitStatus{true: models.StatusUnpushed, false: models.StatusPushed}[!passedFirstPushedCommit] - commits = append(commits, commit) - return false, nil - }) - if err != nil { - return nil, err + if passedFirstPushedCommit { + commit.Status = models.StatusPushed + } else { + commit.Status = models.StatusUnpushed + } } if len(commits) == 0 { return commits, nil } - commits = self.setCommitMergedStatuses(opts.RefName, commits) + if ancestor != "" { + commits = self.setCommitMergedStatuses(ancestor, commits) + } return commits, nil } @@ -464,11 +495,7 @@ func (self *CommitLoader) commitFromPatch(content string) *models.Commit { } } -func (self *CommitLoader) setCommitMergedStatuses(refName string, commits []*models.Commit) []*models.Commit { - ancestor := self.getMergeBase(refName) - if ancestor == "" { - return commits - } +func (self *CommitLoader) setCommitMergedStatuses(ancestor string, commits []*models.Commit) []*models.Commit { passedAncestor := false for i, commit := range commits { if strings.HasPrefix(ancestor, commit.Sha) { @@ -510,13 +537,25 @@ func (self *CommitLoader) getMergeBase(refName string) string { } func (self *CommitLoader) getExistingMainBranches() []string { - return lo.FilterMap(self.UserConfig.Git.MainBranches, - func(branchName string, _ int) (string, bool) { + var existingBranches []string + var wg sync.WaitGroup + + mainBranches := self.UserConfig.Git.MainBranches + existingBranches = make([]string, len(mainBranches)) + + for i, branchName := range mainBranches { + wg.Add(1) + i := i + branchName := branchName + go utils.Safe(func() { + defer wg.Done() + // Try to determine upstream of local main branch if ref, err := self.cmd.New( NewGitCmd("rev-parse").Arg("--symbolic-full-name", branchName+"@{u}").ToArgv(), ).DontLog().RunWithOutput(); err == nil { - return strings.TrimSpace(ref), true + existingBranches[i] = strings.TrimSpace(ref) + return } // If this failed, a local branch for this main branch doesn't exist or it @@ -525,7 +564,8 @@ func (self *CommitLoader) getExistingMainBranches() []string { if err := self.cmd.New( NewGitCmd("rev-parse").Arg("--verify", "--quiet", ref).ToArgv(), ).DontLog().Run(); err == nil { - return ref, true + existingBranches[i] = ref + return } // If this failed as well, try if we have the main branch as a local @@ -535,11 +575,18 @@ func (self *CommitLoader) getExistingMainBranches() []string { if err := self.cmd.New( NewGitCmd("rev-parse").Arg("--verify", "--quiet", ref).ToArgv(), ).DontLog().Run(); err == nil { - return ref, true + existingBranches[i] = ref } - - return "", false }) + } + + wg.Wait() + + existingBranches = lo.Filter(existingBranches, func(branch string, _ int) bool { + return branch != "" + }) + + return existingBranches } func ignoringWarnings(commandOutput string) string { From 862ebd25cb24cb17bb823d8068398ddfcefd991a Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 08:16:48 +1000 Subject: [PATCH 5/9] Speed up remote loader We're: * using concurrency with wait groups * avoiding regex * processing lines of input as they come rather than storing everything in one string * avoiding an inner loop by creating a mapping of remote names to branches --- pkg/commands/git_commands/remote_loader.go | 75 +++++++++++++++++----- 1 file changed, 58 insertions(+), 17 deletions(-) diff --git a/pkg/commands/git_commands/remote_loader.go b/pkg/commands/git_commands/remote_loader.go index 553a8f0c0..c2a8da025 100644 --- a/pkg/commands/git_commands/remote_loader.go +++ b/pkg/commands/git_commands/remote_loader.go @@ -1,15 +1,15 @@ package git_commands import ( - "fmt" - "regexp" "strings" + "sync" "github.com/jesseduffield/generics/slices" gogit "github.com/jesseduffield/go-git/v5" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/common" + "github.com/jesseduffield/lazygit/pkg/utils" ) type RemoteLoader struct { @@ -31,29 +31,31 @@ func NewRemoteLoader( } func (self *RemoteLoader) GetRemotes() ([]*models.Remote, error) { - cmdArgs := NewGitCmd("branch").Arg("-r").ToArgv() - remoteBranchesStr, err := self.cmd.New(cmdArgs).DontLog().RunWithOutput() - if err != nil { - return nil, err - } + wg := sync.WaitGroup{} + wg.Add(1) + + var remoteBranchesByRemoteName map[string][]*models.RemoteBranch + var remoteBranchesErr error + go utils.Safe(func() { + defer wg.Done() + + remoteBranchesByRemoteName, remoteBranchesErr = self.getRemoteBranchesByRemoteName() + }) goGitRemotes, err := self.getGoGitRemotes() if err != nil { return nil, err } - // first step is to get our remotes from go-git + wg.Wait() + + if remoteBranchesErr != nil { + return nil, remoteBranchesErr + } + remotes := slices.Map(goGitRemotes, func(goGitRemote *gogit.Remote) *models.Remote { remoteName := goGitRemote.Config().Name - - re := regexp.MustCompile(fmt.Sprintf(`(?m)^\s*%s\/([\S]+)`, regexp.QuoteMeta(remoteName))) - matches := re.FindAllStringSubmatch(remoteBranchesStr, -1) - branches := slices.Map(matches, func(match []string) *models.RemoteBranch { - return &models.RemoteBranch{ - Name: match[1], - RemoteName: remoteName, - } - }) + branches := remoteBranchesByRemoteName[remoteName] return &models.Remote{ Name: goGitRemote.Config().Name, @@ -76,3 +78,42 @@ func (self *RemoteLoader) GetRemotes() ([]*models.Remote, error) { return remotes, nil } + +func (self *RemoteLoader) getRemoteBranchesByRemoteName() (map[string][]*models.RemoteBranch, error) { + remoteBranchesByRemoteName := make(map[string][]*models.RemoteBranch) + + cmdArgs := NewGitCmd("branch").Arg("-r").ToArgv() + err := self.cmd.New(cmdArgs).DontLog().RunAndProcessLines(func(line string) (bool, error) { + // excluding lines like 'origin/HEAD -> origin/master' (there will be a separate + // line for 'origin/master') + if strings.Contains(line, "->") { + return false, nil + } + + line = strings.TrimSpace(line) + + split := strings.SplitN(line, "/", 2) + if len(split) != 2 { + return false, nil + } + remoteName := split[0] + name := split[1] + + _, ok := remoteBranchesByRemoteName[remoteName] + if !ok { + remoteBranchesByRemoteName[remoteName] = []*models.RemoteBranch{} + } + + remoteBranchesByRemoteName[remoteName] = append(remoteBranchesByRemoteName[remoteName], + &models.RemoteBranch{ + Name: name, + RemoteName: remoteName, + }) + return false, nil + }) + if err != nil { + return nil, err + } + + return remoteBranchesByRemoteName, nil +} From 272e021c080fff92d4b38fd1fa7a07d520501edb Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 09:02:56 +1000 Subject: [PATCH 6/9] Refactor reflog commit loader No functional changes --- .../git_commands/reflog_commit_loader.go | 50 ++++++++++++------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/pkg/commands/git_commands/reflog_commit_loader.go b/pkg/commands/git_commands/reflog_commit_loader.go index 5033b03c1..1160839d6 100644 --- a/pkg/commands/git_commands/reflog_commit_loader.go +++ b/pkg/commands/git_commands/reflog_commit_loader.go @@ -35,34 +35,19 @@ func (self *ReflogCommitLoader) GetReflogCommits(lastReflogCommit *models.Commit ToArgv() cmdObj := self.cmd.New(cmdArgs).DontLog() + onlyObtainedNewReflogCommits := false err := cmdObj.RunAndProcessLines(func(line string) (bool, error) { - fields := strings.SplitN(line, "\x00", 4) - if len(fields) <= 3 { + commit, ok := self.parseLine(line) + if !ok { return false, nil } - unixTimestamp, _ := strconv.Atoi(fields[1]) - - parentHashes := fields[3] - parents := []string{} - if len(parentHashes) > 0 { - parents = strings.Split(parentHashes, " ") - } - - commit := &models.Commit{ - Sha: fields[0], - Name: fields[2], - UnixTimestamp: int64(unixTimestamp), - Status: models.StatusReflog, - Parents: parents, - } - // note that the unix timestamp here is the timestamp of the COMMIT, not the reflog entry itself, // so two consecutive reflog entries may have both the same SHA and therefore same timestamp. // We use the reflog message to disambiguate, and fingers crossed that we never see the same of those // twice in a row. Reason being that it would mean we'd be erroneously exiting early. - if lastReflogCommit != nil && commit.Sha == lastReflogCommit.Sha && commit.UnixTimestamp == lastReflogCommit.UnixTimestamp && commit.Name == lastReflogCommit.Name { + if lastReflogCommit != nil && self.sameReflogCommit(commit, lastReflogCommit) { onlyObtainedNewReflogCommits = true // after this point we already have these reflogs loaded so we'll simply return the new ones return true, nil @@ -77,3 +62,30 @@ func (self *ReflogCommitLoader) GetReflogCommits(lastReflogCommit *models.Commit return commits, onlyObtainedNewReflogCommits, nil } + +func (self *ReflogCommitLoader) sameReflogCommit(a *models.Commit, b *models.Commit) bool { + return a.Sha == b.Sha && a.UnixTimestamp == b.UnixTimestamp && a.Name == b.Name +} + +func (self *ReflogCommitLoader) parseLine(line string) (*models.Commit, bool) { + fields := strings.SplitN(line, "\x00", 4) + if len(fields) <= 3 { + return nil, false + } + + unixTimestamp, _ := strconv.Atoi(fields[1]) + + parentHashes := fields[3] + parents := []string{} + if len(parentHashes) > 0 { + parents = strings.Split(parentHashes, " ") + } + + return &models.Commit{ + Sha: fields[0], + Name: fields[2], + UnixTimestamp: int64(unixTimestamp), + Status: models.StatusReflog, + Parents: parents, + }, true +} From 63e57904102a2971bc6803f50dbbfae1fd2da123 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 09:34:17 +1000 Subject: [PATCH 7/9] Speed up refresh using concurrency and wait groups Previously our synchronous refreshes took far longer because nothing was happening concurrently. We now run refresh functions concurrently and use a wait group to ensure they're all done before returning --- pkg/gui/controllers/helpers/refresh_helper.go | 91 ++++++++++--------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index cc0acd317..4437b849c 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -91,59 +91,70 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { scopeSet = set.NewFromSlice(options.Scope) } - refresh := func(f func()) { + wg := sync.WaitGroup{} + refresh := func(name string, f func()) { if options.Mode == types.ASYNC { self.c.OnWorker(func(t gocui.Task) { f() }) } else { - f() + wg.Add(1) + go utils.Safe(func() { + t := time.Now() + defer wg.Done() + f() + self.c.Log.Infof(fmt.Sprintf("refreshed %s in %s", name, time.Since(t))) + }) } } if scopeSet.Includes(types.COMMITS) || scopeSet.Includes(types.BRANCHES) || scopeSet.Includes(types.REFLOG) || scopeSet.Includes(types.BISECT_INFO) { - refresh(self.refreshCommits) + // whenever we change commits, we should update branches because the upstream/downstream + // counts can change. Whenever we change branches we should also change commits + // e.g. in the case of switching branches. + refresh("commits and commit files", self.refreshCommitsAndCommitFiles) + refresh("reflog and branches", self.refreshReflogAndBranches) } else if scopeSet.Includes(types.REBASE_COMMITS) { // the above block handles rebase commits so we only need to call this one // if we've asked specifically for rebase commits and not those other things - refresh(func() { _ = self.refreshRebaseCommits() }) + refresh("rebase commits", func() { _ = self.refreshRebaseCommits() }) } if scopeSet.Includes(types.SUB_COMMITS) { - refresh(func() { _ = self.refreshSubCommitsWithLimit() }) + refresh("sub commits", func() { _ = self.refreshSubCommitsWithLimit() }) } // reason we're not doing this if the COMMITS type is included is that if the COMMITS type _is_ included we will refresh the commit files context anyway if scopeSet.Includes(types.COMMIT_FILES) && !scopeSet.Includes(types.COMMITS) { - refresh(func() { _ = self.refreshCommitFilesContext() }) + refresh("commit files", func() { _ = self.refreshCommitFilesContext() }) } if scopeSet.Includes(types.FILES) || scopeSet.Includes(types.SUBMODULES) { - refresh(func() { _ = self.refreshFilesAndSubmodules() }) + refresh("files", func() { _ = self.refreshFilesAndSubmodules() }) } if scopeSet.Includes(types.STASH) { - refresh(func() { _ = self.refreshStashEntries() }) + refresh("stash", func() { _ = self.refreshStashEntries() }) } if scopeSet.Includes(types.TAGS) { - refresh(func() { _ = self.refreshTags() }) + refresh("tags", func() { _ = self.refreshTags() }) } if scopeSet.Includes(types.REMOTES) { - refresh(func() { _ = self.refreshRemotes() }) + refresh("remotes", func() { _ = self.refreshRemotes() }) } if scopeSet.Includes(types.STAGING) { - refresh(func() { _ = self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) }) + refresh("staging", func() { _ = self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) }) } if scopeSet.Includes(types.PATCH_BUILDING) { - refresh(func() { _ = self.patchBuildingHelper.RefreshPatchBuildingPanel(types.OnFocusOpts{}) }) + refresh("patch building", func() { _ = self.patchBuildingHelper.RefreshPatchBuildingPanel(types.OnFocusOpts{}) }) } if scopeSet.Includes(types.MERGE_CONFLICTS) || scopeSet.Includes(types.FILES) { - refresh(func() { _ = self.mergeConflictsHelper.RefreshMergeState() }) + refresh("merge conflicts", func() { _ = self.mergeConflictsHelper.RefreshMergeState() }) } self.refreshStatus() @@ -151,6 +162,8 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { if options.Then != nil { options.Then() } + + wg.Wait() } if options.Mode == types.BLOCK_UI { @@ -218,41 +231,29 @@ func (self *RefreshHelper) refreshReflogCommitsConsideringStartup() { } } -// whenever we change commits, we should update branches because the upstream/downstream -// counts can change. Whenever we change branches we should probably also change commits -// e.g. in the case of switching branches. -func (self *RefreshHelper) refreshCommits() { - wg := sync.WaitGroup{} - wg.Add(2) +func (self *RefreshHelper) refreshReflogAndBranches() { + self.refreshReflogCommitsConsideringStartup() - go utils.Safe(func() { - self.refreshReflogCommitsConsideringStartup() + self.refreshBranches() +} - self.refreshBranches() - wg.Done() - }) - - go utils.Safe(func() { - _ = self.refreshCommitsWithLimit() - ctx, ok := self.c.Contexts().CommitFiles.GetParentContext() - if ok && ctx.GetKey() == context.LOCAL_COMMITS_CONTEXT_KEY { - // This makes sense when we've e.g. just amended a commit, meaning we get a new commit SHA at the same position. - // However if we've just added a brand new commit, it pushes the list down by one and so we would end up - // showing the contents of a different commit than the one we initially entered. - // Ideally we would know when to refresh the commit files context and when not to, - // or perhaps we could just pop that context off the stack whenever cycling windows. - // For now the awkwardness remains. - commit := self.c.Contexts().LocalCommits.GetSelected() - if commit != nil { - self.c.Contexts().CommitFiles.SetRef(commit) - self.c.Contexts().CommitFiles.SetTitleRef(commit.RefName()) - _ = self.refreshCommitFilesContext() - } +func (self *RefreshHelper) refreshCommitsAndCommitFiles() { + _ = self.refreshCommitsWithLimit() + ctx, ok := self.c.Contexts().CommitFiles.GetParentContext() + if ok && ctx.GetKey() == context.LOCAL_COMMITS_CONTEXT_KEY { + // This makes sense when we've e.g. just amended a commit, meaning we get a new commit SHA at the same position. + // However if we've just added a brand new commit, it pushes the list down by one and so we would end up + // showing the contents of a different commit than the one we initially entered. + // Ideally we would know when to refresh the commit files context and when not to, + // or perhaps we could just pop that context off the stack whenever cycling windows. + // For now the awkwardness remains. + commit := self.c.Contexts().LocalCommits.GetSelected() + if commit != nil { + self.c.Contexts().CommitFiles.SetRef(commit) + self.c.Contexts().CommitFiles.SetTitleRef(commit.RefName()) + _ = self.refreshCommitFilesContext() } - wg.Done() - }) - - wg.Wait() + } } func (self *RefreshHelper) refreshCommitsWithLimit() error { From 39b77c0fcaf1316f2fa7926b285800492e3f7ae2 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 10:20:15 +1000 Subject: [PATCH 8/9] Have staging refresh wait for files to refresh first --- pkg/gui/controllers/helpers/refresh_helper.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 4437b849c..c8fbe8627 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -129,8 +129,13 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { refresh("commit files", func() { _ = self.refreshCommitFilesContext() }) } + fileWg := sync.WaitGroup{} if scopeSet.Includes(types.FILES) || scopeSet.Includes(types.SUBMODULES) { - refresh("files", func() { _ = self.refreshFilesAndSubmodules() }) + fileWg.Add(1) + refresh("files", func() { + _ = self.refreshFilesAndSubmodules() + fileWg.Done() + }) } if scopeSet.Includes(types.STASH) { @@ -146,7 +151,10 @@ func (self *RefreshHelper) Refresh(options types.RefreshOptions) error { } if scopeSet.Includes(types.STAGING) { - refresh("staging", func() { _ = self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) }) + refresh("staging", func() { + fileWg.Wait() + _ = self.stagingHelper.RefreshStagingPanel(types.OnFocusOpts{}) + }) } if scopeSet.Includes(types.PATCH_BUILDING) { From 429225da80b90c53da65e4eb1b5723ae58e3ba4c Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sat, 29 Jul 2023 12:32:59 +1000 Subject: [PATCH 9/9] Support random order of command execution in unit tests Now that we run code concurrently in our loaders, we need to handle that in our tests. We could enforce a deterministic ordering by mocking waitgroup or something like that, but I think it's fine to let our tests handle some randomness given that prod itself will have that randomness. I've removed the patch test file because it was clunky, not providing much value, and it would have been hard to refactor to the new pattern --- .../git_commands/commit_loader_test.go | 2 +- pkg/commands/git_commands/deps_test.go | 4 +- pkg/commands/git_commands/patch_test.go | 68 ---------- pkg/commands/git_commands/rebase_test.go | 8 +- .../oscommands/fake_cmd_obj_runner.go | 124 +++++++++++++----- 5 files changed, 97 insertions(+), 109 deletions(-) delete mode 100644 pkg/commands/git_commands/patch_test.go diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 8398613a8..bcbc33c02 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -1,12 +1,12 @@ package git_commands import ( - "errors" "path/filepath" "strings" "testing" "github.com/fsmiamoto/git-todo-parser/todo" + "github.com/go-errors/errors" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" diff --git a/pkg/commands/git_commands/deps_test.go b/pkg/commands/git_commands/deps_test.go index 5926584b3..a960098df 100644 --- a/pkg/commands/git_commands/deps_test.go +++ b/pkg/commands/git_commands/deps_test.go @@ -118,7 +118,7 @@ func buildWorkingTreeCommands(deps commonDeps) *WorkingTreeCommands { return NewWorkingTreeCommands(gitCommon, submoduleCommands, fileLoader) } -func buildPatchCommands(deps commonDeps) *PatchCommands { +func buildPatchCommands(deps commonDeps) *PatchCommands { //nolint:golint,unused gitCommon := buildGitCommon(deps) rebaseCommands := buildRebaseCommands(deps) commitCommands := buildCommitCommands(deps) @@ -132,7 +132,7 @@ func buildPatchCommands(deps commonDeps) *PatchCommands { return NewPatchCommands(gitCommon, rebaseCommands, commitCommands, statusCommands, stashCommands, patchBuilder) } -func buildStatusCommands(deps commonDeps) *StatusCommands { +func buildStatusCommands(deps commonDeps) *StatusCommands { //nolint:golint,unused gitCommon := buildGitCommon(deps) return NewStatusCommands(gitCommon) diff --git a/pkg/commands/git_commands/patch_test.go b/pkg/commands/git_commands/patch_test.go deleted file mode 100644 index 8acf471d4..000000000 --- a/pkg/commands/git_commands/patch_test.go +++ /dev/null @@ -1,68 +0,0 @@ -package git_commands - -import ( - "fmt" - "os" - "testing" - - "github.com/go-errors/errors" - "github.com/jesseduffield/lazygit/pkg/commands/oscommands" - "github.com/stretchr/testify/assert" -) - -func TestPatchApplyPatch(t *testing.T) { - type scenario struct { - testName string - opts ApplyPatchOpts - runner *oscommands.FakeCmdObjRunner - test func(error) - } - - // expectedArgs excludes the last argument which is an indeterminate filename - expectFn := func(expectedArgs []string, errToReturn error) func(cmdObj oscommands.ICmdObj) (string, error) { - return func(cmdObj oscommands.ICmdObj) (string, error) { - args := cmdObj.Args() - - assert.Equal(t, len(args), len(expectedArgs)+1, fmt.Sprintf("unexpected command: %s", cmdObj.ToString())) - - filename := args[len(args)-1] - - content, err := os.ReadFile(filename) - assert.NoError(t, err) - - assert.Equal(t, "test", string(content)) - - return "", errToReturn - } - } - - scenarios := []scenario{ - { - testName: "valid case", - opts: ApplyPatchOpts{Cached: true}, - runner: oscommands.NewFakeRunner(t). - ExpectFunc(expectFn([]string{"git", "apply", "--cached"}, nil)), - test: func(err error) { - assert.NoError(t, err) - }, - }, - { - testName: "command returns error", - opts: ApplyPatchOpts{Cached: true}, - runner: oscommands.NewFakeRunner(t). - ExpectFunc(expectFn([]string{"git", "apply", "--cached"}, errors.New("error"))), - test: func(err error) { - assert.Error(t, err) - }, - }, - } - - for _, s := range scenarios { - s := s - t.Run(s.testName, func(t *testing.T) { - instance := buildPatchCommands(commonDeps{runner: s.runner}) - s.test(instance.ApplyPatch("test", s.opts)) - s.runner.CheckForMissingCalls() - }) - } -} diff --git a/pkg/commands/git_commands/rebase_test.go b/pkg/commands/git_commands/rebase_test.go index b616ec609..d10746220 100644 --- a/pkg/commands/git_commands/rebase_test.go +++ b/pkg/commands/git_commands/rebase_test.go @@ -79,7 +79,7 @@ func TestRebaseRebaseBranch(t *testing.T) { // environment variables that suppress an interactive editor func TestRebaseSkipEditorCommand(t *testing.T) { cmdArgs := []string{"git", "blah"} - runner := oscommands.NewFakeRunner(t).ExpectFunc(func(cmdObj oscommands.ICmdObj) (string, error) { + runner := oscommands.NewFakeRunner(t).ExpectFunc("matches editor env var", func(cmdObj oscommands.ICmdObj) bool { assert.EqualValues(t, cmdArgs, cmdObj.Args()) envVars := cmdObj.GetEnvVars() for _, regexStr := range []string{ @@ -94,11 +94,11 @@ func TestRebaseSkipEditorCommand(t *testing.T) { return regexp.MustCompile(regexStr).MatchString(envVar) }) if !foundMatch { - t.Errorf("expected environment variable %s to be set", regexStr) + return false } } - return "", nil - }) + return true + }, "", nil) instance := buildRebaseCommands(commonDeps{runner: runner}) err := instance.runSkipEditorCommand(instance.cmd.New(cmdArgs)) assert.NoError(t, err) diff --git a/pkg/commands/oscommands/fake_cmd_obj_runner.go b/pkg/commands/oscommands/fake_cmd_obj_runner.go index fa34f01be..dc75228a0 100644 --- a/pkg/commands/oscommands/fake_cmd_obj_runner.go +++ b/pkg/commands/oscommands/fake_cmd_obj_runner.go @@ -6,18 +6,36 @@ import ( "regexp" "runtime" "strings" + "sync" "testing" "github.com/go-errors/errors" - "github.com/stretchr/testify/assert" + "github.com/samber/lo" + "golang.org/x/exp/slices" ) // for use in testing type FakeCmdObjRunner struct { - t *testing.T - expectedCmds []func(ICmdObj) (string, error) - expectedCmdIndex int + t *testing.T + // commands can be run in any order; mimicking the concurrent behaviour of + // production code. + expectedCmds []CmdObjMatcher + + invokedCmdIndexes []int + + mutex sync.Mutex +} + +type CmdObjMatcher struct { + description string + // returns true if the matcher matches the command object + test func(ICmdObj) bool + + // output of the command + output string + // error of the command + err error } var _ ICmdObjRunner = &FakeCmdObjRunner{} @@ -26,23 +44,40 @@ func NewFakeRunner(t *testing.T) *FakeCmdObjRunner { //nolint:thelper return &FakeCmdObjRunner{t: t} } +func (self *FakeCmdObjRunner) remainingExpectedCmds() []CmdObjMatcher { + return lo.Filter(self.expectedCmds, func(_ CmdObjMatcher, i int) bool { + return !lo.Contains(self.invokedCmdIndexes, i) + }) +} + func (self *FakeCmdObjRunner) Run(cmdObj ICmdObj) error { _, err := self.RunWithOutput(cmdObj) return err } func (self *FakeCmdObjRunner) RunWithOutput(cmdObj ICmdObj) (string, error) { - if self.expectedCmdIndex > len(self.expectedCmds)-1 { + self.mutex.Lock() + defer self.mutex.Unlock() + + if len(self.remainingExpectedCmds()) == 0 { self.t.Errorf("ran too many commands. Unexpected command: `%s`", cmdObj.ToString()) return "", errors.New("ran too many commands") } - expectedCmd := self.expectedCmds[self.expectedCmdIndex] - output, err := expectedCmd(cmdObj) + for i := range self.expectedCmds { + if lo.Contains(self.invokedCmdIndexes, i) { + continue + } + expectedCmd := self.expectedCmds[i] + matched := expectedCmd.test(cmdObj) + if matched { + self.invokedCmdIndexes = append(self.invokedCmdIndexes, i) + return expectedCmd.output, expectedCmd.err + } + } - self.expectedCmdIndex++ - - return output, err + self.t.Errorf("Unexpected command: `%s`", cmdObj.ToString()) + return "", nil } func (self *FakeCmdObjRunner) RunWithOutputs(cmdObj ICmdObj) (string, string, error) { @@ -72,63 +107,84 @@ func (self *FakeCmdObjRunner) RunAndProcessLines(cmdObj ICmdObj, onLine func(lin return nil } -func (self *FakeCmdObjRunner) ExpectFunc(fn func(cmdObj ICmdObj) (string, error)) *FakeCmdObjRunner { - self.expectedCmds = append(self.expectedCmds, fn) +func (self *FakeCmdObjRunner) ExpectFunc(description string, fn func(cmdObj ICmdObj) bool, output string, err error) *FakeCmdObjRunner { + self.mutex.Lock() + defer self.mutex.Unlock() - return self -} - -func (self *FakeCmdObjRunner) Expect(expectedCmdStr string, output string, err error) *FakeCmdObjRunner { - self.ExpectFunc(func(cmdObj ICmdObj) (string, error) { - cmdStr := cmdObj.ToString() - assert.Equal(self.t, expectedCmdStr, cmdStr, fmt.Sprintf("expected command %d to be %s, but was %s", self.expectedCmdIndex+1, expectedCmdStr, cmdStr)) - - return output, err + self.expectedCmds = append(self.expectedCmds, CmdObjMatcher{ + test: fn, + output: output, + err: err, + description: description, }) return self } func (self *FakeCmdObjRunner) ExpectArgs(expectedArgs []string, output string, err error) *FakeCmdObjRunner { - self.ExpectFunc(func(cmdObj ICmdObj) (string, error) { + description := fmt.Sprintf("matches args %s", strings.Join(expectedArgs, " ")) + self.ExpectFunc(description, func(cmdObj ICmdObj) bool { args := cmdObj.GetCmd().Args if runtime.GOOS == "windows" { // thanks to the secureexec package, the first arg is something like // '"C:\\Program Files\\Git\\mingw64\\bin\\.exe" // on windows so we'll just ensure it contains our program - assert.Contains(self.t, args[0], expectedArgs[0]) + if !strings.Contains(args[0], expectedArgs[0]) { + return false + } } else { // first arg is the program name - assert.Equal(self.t, expectedArgs[0], args[0]) + if expectedArgs[0] != args[0] { + return false + } } - assert.EqualValues(self.t, expectedArgs[1:], args[1:], fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1)) + if !slices.Equal(expectedArgs[1:], args[1:]) { + return false + } - return output, err - }) + return true + }, output, err) return self } func (self *FakeCmdObjRunner) ExpectGitArgs(expectedArgs []string, output string, err error) *FakeCmdObjRunner { - self.ExpectFunc(func(cmdObj ICmdObj) (string, error) { + description := fmt.Sprintf("matches git args %s", strings.Join(expectedArgs, " ")) + self.ExpectFunc(description, func(cmdObj ICmdObj) bool { // first arg is 'git' on unix and something like '"C:\\Program Files\\Git\\mingw64\\bin\\git.exe" on windows so we'll just ensure it ends in either 'git' or 'git.exe' re := regexp.MustCompile(`git(\.exe)?$`) args := cmdObj.GetCmd().Args if !re.MatchString(args[0]) { - self.t.Errorf("expected first arg to end in .git or .git.exe but was %s", args[0]) + return false } - assert.EqualValues(self.t, expectedArgs, args[1:], fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1)) - return output, err - }) + if !slices.Equal(expectedArgs, args[1:]) { + return false + } + + return true + }, output, err) return self } func (self *FakeCmdObjRunner) CheckForMissingCalls() { - if self.expectedCmdIndex < len(self.expectedCmds) { - self.t.Errorf("expected command %d to be called, but was not", self.expectedCmdIndex+1) + self.mutex.Lock() + defer self.mutex.Unlock() + + remaining := self.remainingExpectedCmds() + if len(remaining) > 0 { + self.t.Errorf( + "expected %d more command(s) to be run. Remaining commands:\n%s", + len(remaining), + strings.Join( + lo.Map(remaining, func(cmdObj CmdObjMatcher, _ int) string { + return cmdObj.description + }), + "\n", + ), + ) } }