From 7a670964cd3951c20e42a7b215f403f8ec4136c2 Mon Sep 17 00:00:00 2001 From: John Whitley Date: Mon, 29 Jan 2024 16:58:35 -0800 Subject: [PATCH] Optimize number of early calls to GetRepoPaths This change reduces the number of calls during application startup to one, calling GetRepoPaths() earlier than previously and plumbing the repoPaths struct around to achieve this end. --- pkg/app/app.go | 24 ++++++---- pkg/commands/git_commands/repo_paths.go | 35 ++++++++++----- pkg/commands/git_commands/repo_paths_test.go | 47 +++++++++++++++++--- pkg/commands/git_commands/status.go | 18 +------- pkg/gui/dummies.go | 2 +- pkg/gui/recent_repos_panel.go | 7 +-- 6 files changed, 85 insertions(+), 48 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index a16fbcc1f..e12461e28 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -14,7 +14,6 @@ import ( "github.com/spf13/afero" appTypes "github.com/jesseduffield/lazygit/pkg/app/types" - "github.com/jesseduffield/lazygit/pkg/commands" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/common" @@ -119,7 +118,14 @@ func NewApp(config config.AppConfigurer, test integrationTypes.IntegrationTest, return app, err } - showRecentRepos, err := app.setupRepo() + // If we're not in a repo, repoPaths will be nil. The error is moot for us + // at this stage, since we'll try to init a new repo in setupRepo(), below + repoPaths, err := git_commands.GetRepoPaths(app.OSCommand.Cmd, gitVersion) + if err != nil { + return app, err + } + + showRecentRepos, err := app.setupRepo(repoPaths) if err != nil { return app, err } @@ -168,14 +174,16 @@ func openRecentRepo(app *App) bool { return false } -func (app *App) setupRepo() (bool, error) { +func (app *App) setupRepo( + repoPaths *git_commands.RepoPaths, +) (bool, error) { if env.GetGitDirEnv() != "" { - // we've been given the git dir directly. We'll verify this dir when initializing our Git object + // we've been given the git dir directly. Skip setup return false, nil } // if we are not in a git repo, we ask if we want to `git init` - if err := commands.VerifyInGitRepo(app.OSCommand); err != nil { + if repoPaths == nil { cwd, err := os.Getwd() if err != nil { return false, err @@ -221,6 +229,7 @@ func (app *App) setupRepo() (bool, error) { if err := app.OSCommand.Cmd.New(args).Run(); err != nil { return false, err } + return false, nil } @@ -238,10 +247,7 @@ func (app *App) setupRepo() (bool, error) { } // Run this afterward so that the previous repo creation steps can run without this interfering - if isBare, err := git_commands.IsBareRepo(app.OSCommand); isBare { - if err != nil { - return false, err - } + if repoPaths.IsBareRepo() { fmt.Print(app.Tr.BareRepo) diff --git a/pkg/commands/git_commands/repo_paths.go b/pkg/commands/git_commands/repo_paths.go index b0e1970db..c2e77d446 100644 --- a/pkg/commands/git_commands/repo_paths.go +++ b/pkg/commands/git_commands/repo_paths.go @@ -2,6 +2,7 @@ package git_commands import ( ioFs "io/fs" + "os" "path" "path/filepath" "strings" @@ -18,6 +19,7 @@ type RepoPaths struct { repoPath string repoGitDirPath string repoName string + isBareRepo bool } var gitPathFormatVersion GitVersion = GitVersion{2, 31, 0, ""} @@ -54,6 +56,10 @@ func (self *RepoPaths) RepoName() string { return self.repoName } +func (self *RepoPaths) IsBareRepo() bool { + return self.isBareRepo +} + // Returns the repo paths for a typical repo func MockRepoPaths(currentPath string) *RepoPaths { return &RepoPaths{ @@ -62,6 +68,7 @@ func MockRepoPaths(currentPath string) *RepoPaths { repoPath: currentPath, repoGitDirPath: path.Join(currentPath, ".git"), repoName: "lazygit", + isBareRepo: false, } } @@ -69,7 +76,19 @@ func GetRepoPaths( cmd oscommands.ICmdObjBuilder, version *GitVersion, ) (*RepoPaths, error) { - gitDirOutput, err := callGitRevParse(cmd, version, "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree") + cwd, err := os.Getwd() + if err != nil { + return nil, err + } + return GetRepoPathsForDir(cwd, cmd, version) +} + +func GetRepoPathsForDir( + dir string, + cmd oscommands.ICmdObjBuilder, + version *GitVersion, +) (*RepoPaths, error) { + gitDirOutput, err := callGitRevParseWithDir(cmd, version, dir, "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--is-bare-repository", "--show-superproject-working-tree") if err != nil { return nil, err } @@ -84,13 +103,14 @@ func GetRepoPaths( return nil, err } } + isBareRepo := gitDirResults[3] == "true" // If we're in a submodule, --show-superproject-working-tree will return - // a value, meaning gitDirResults will be length 4. In that case + // a value, meaning gitDirResults will be length 5. In that case // return the worktree path as the repoPath. Otherwise we're in a // normal repo or a worktree so return the parent of the git common // dir (repoGitDirPath) - isSubmodule := len(gitDirResults) == 4 + isSubmodule := len(gitDirResults) == 5 var repoPath string if isSubmodule { @@ -106,17 +126,10 @@ func GetRepoPaths( repoPath: repoPath, repoGitDirPath: repoGitDirPath, repoName: repoName, + isBareRepo: isBareRepo, }, nil } -func callGitRevParse( - cmd oscommands.ICmdObjBuilder, - version *GitVersion, - gitRevArgs ...string, -) (string, error) { - return callGitRevParseWithDir(cmd, version, "", gitRevArgs...) -} - func callGitRevParseWithDir( cmd oscommands.ICmdObjBuilder, version *GitVersion, diff --git a/pkg/commands/git_commands/repo_paths_test.go b/pkg/commands/git_commands/repo_paths_test.go index 97cfc8119..9ee41a3fc 100644 --- a/pkg/commands/git_commands/repo_paths_test.go +++ b/pkg/commands/git_commands/repo_paths_test.go @@ -36,10 +36,12 @@ func TestGetRepoPaths(t *testing.T) { "/path/to/repo/.git", // --git-common-dir "/path/to/repo/.git", + // --is-bare-repository + "false", // --show-superproject-working-tree } runner.ExpectGitArgs( - append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree"), + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--is-bare-repository", "--show-superproject-working-tree"), strings.Join(expectedOutput, "\n"), nil) }, @@ -50,6 +52,38 @@ func TestGetRepoPaths(t *testing.T) { repoPath: "/path/to/repo", repoGitDirPath: "/path/to/repo/.git", repoName: "repo", + isBareRepo: false, + }, + Err: nil, + }, + { + Name: "bare repo", + BeforeFunc: func(runner *oscommands.FakeCmdObjRunner, getRevParseArgs argFn) { + // setup for main worktree + expectedOutput := []string{ + // --show-toplevel + "/path/to/repo", + // --git-dir + "/path/to/bare_repo/bare.git", + // --git-common-dir + "/path/to/bare_repo/bare.git", + // --is-bare-repository + "true", + // --show-superproject-working-tree + } + runner.ExpectGitArgs( + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--is-bare-repository", "--show-superproject-working-tree"), + strings.Join(expectedOutput, "\n"), + nil) + }, + Path: "/path/to/repo", + Expected: &RepoPaths{ + worktreePath: "/path/to/repo", + worktreeGitDirPath: "/path/to/bare_repo/bare.git", + repoPath: "/path/to/bare_repo", + repoGitDirPath: "/path/to/bare_repo/bare.git", + repoName: "bare_repo", + isBareRepo: true, }, Err: nil, }, @@ -63,11 +97,13 @@ func TestGetRepoPaths(t *testing.T) { "/path/to/repo/.git/modules/submodule1", // --git-common-dir "/path/to/repo/.git/modules/submodule1", + // --is-bare-repository + "false", // --show-superproject-working-tree "/path/to/repo", } runner.ExpectGitArgs( - append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree"), + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--is-bare-repository", "--show-superproject-working-tree"), strings.Join(expectedOutput, "\n"), nil) }, @@ -78,6 +114,7 @@ func TestGetRepoPaths(t *testing.T) { repoPath: "/path/to/repo/submodule1", repoGitDirPath: "/path/to/repo/.git/modules/submodule1", repoName: "submodule1", + isBareRepo: false, }, Err: nil, }, @@ -85,7 +122,7 @@ func TestGetRepoPaths(t *testing.T) { Name: "git rev-parse returns an error", BeforeFunc: func(runner *oscommands.FakeCmdObjRunner, getRevParseArgs argFn) { runner.ExpectGitArgs( - append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree"), + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--is-bare-repository", "--show-superproject-working-tree"), "", errors.New("fatal: invalid gitfile format: /path/to/repo/worktree2/.git")) }, @@ -94,7 +131,7 @@ func TestGetRepoPaths(t *testing.T) { Err: func(getRevParseArgs argFn) error { args := strings.Join(getRevParseArgs(), " ") return errors.New( - fmt.Sprintf("'git %v --show-toplevel --absolute-git-dir --git-common-dir --show-superproject-working-tree' failed: fatal: invalid gitfile format: /path/to/repo/worktree2/.git", args), + fmt.Sprintf("'git %v --show-toplevel --absolute-git-dir --git-common-dir --is-bare-repository --show-superproject-working-tree' failed: fatal: invalid gitfile format: /path/to/repo/worktree2/.git", args), ) }, }, @@ -120,7 +157,7 @@ func TestGetRepoPaths(t *testing.T) { // prepare the filesystem for the scenario s.BeforeFunc(runner, getRevParseArgs) - repoPaths, err := GetRepoPaths(cmd, version) + repoPaths, err := GetRepoPathsForDir("", cmd, version) // check the error and the paths if s.Err != nil { diff --git a/pkg/commands/git_commands/status.go b/pkg/commands/git_commands/status.go index 65b29deef..0e0ef37fc 100644 --- a/pkg/commands/git_commands/status.go +++ b/pkg/commands/git_commands/status.go @@ -3,10 +3,8 @@ package git_commands import ( "os" "path/filepath" - "strconv" "strings" - "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" ) @@ -49,20 +47,8 @@ func (self *StatusCommands) WorkingTreeState() enums.RebaseMode { return enums.REBASE_MODE_NONE } -func (self *StatusCommands) IsBareRepo() (bool, error) { - return IsBareRepo(self.os) -} - -func IsBareRepo(osCommand *oscommands.OSCommand) (bool, error) { - res, err := osCommand.Cmd.New( - NewGitCmd("rev-parse").Arg("--is-bare-repository").ToArgv(), - ).DontLog().RunWithOutput() - if err != nil { - return false, err - } - - // The command returns output with a newline, so we need to strip - return strconv.ParseBool(strings.TrimSpace(res)) +func (self *StatusCommands) IsBareRepo() bool { + return self.repoPaths.isBareRepo } func (self *StatusCommands) IsInNormalRebase() (bool, error) { diff --git a/pkg/gui/dummies.go b/pkg/gui/dummies.go index 7bc36ff33..2350d215e 100644 --- a/pkg/gui/dummies.go +++ b/pkg/gui/dummies.go @@ -17,6 +17,6 @@ func NewDummyUpdater() *updates.Updater { // NewDummyGui creates a new dummy GUI for testing func NewDummyGui() *Gui { newAppConfig := config.NewDummyAppConfig() - dummyGui, _ := NewGui(utils.NewDummyCommon(), newAppConfig, &git_commands.GitVersion{}, NewDummyUpdater(), false, "", nil) + dummyGui, _ := NewGui(utils.NewDummyCommon(), newAppConfig, &git_commands.GitVersion{Major: 2, Minor: 0, Patch: 0}, NewDummyUpdater(), false, "", nil) return dummyGui } diff --git a/pkg/gui/recent_repos_panel.go b/pkg/gui/recent_repos_panel.go index acdb20672..0f2f2c704 100644 --- a/pkg/gui/recent_repos_panel.go +++ b/pkg/gui/recent_repos_panel.go @@ -8,12 +8,7 @@ import ( // updateRecentRepoList registers the fact that we opened lazygit in this repo, // so that we can open the same repo via the 'recent repos' menu func (gui *Gui) updateRecentRepoList() error { - isBareRepo, err := gui.git.Status.IsBareRepo() - if err != nil { - return err - } - - if isBareRepo { + if gui.git.Status.IsBareRepo() { // we could totally do this but it would require storing both the git-dir and the // worktree in our recent repos list, which is a change that would need to be // backwards compatible