Repository: jesseduffield/lazygit (language: go)

Issue:
Follow-on consolidation of git rev-parse calls
In #3183, @jesseduffield writes:
> there's an opportunity to pack even more stuff into the git rev-parse command: e.g. in pkg/app/app.go we call VerifyInGitRepo which does git rev-parse --git-dir and IsBareRepo which does git rev-parse --is-bare-repository.

This is a follow-on issue to track the opportunity to further consolidate lazygit's info retrieval from git rev-parse. During #3183, Jesse observed that rev-parse can take multiple arguments and return the requested information on a line-by-line basis, which reduces overhead vs. multiple git invocations.

See `GetRepoPaths()` in `pkg/commands/git_commands/repo_paths.go` for the new centralized invocation of git rev-parse (on the #3183 branch, or on `master` once that's been merged.)


Discussion / hints:
Thanks for making this issue @jwhitley . To clarify: the goal here is to reduce lazygit startup time as much as possible by reducing the number of git calls required (each git call takes ~10ms on my machine). Given that git rev-parse can take multiple arguments and return multiple values in one go, we want to try and have all the git rev-parse calls on startup condensed into a single call
FWIW, I think it's also possible to consolidate `VerifyInGitRepo()`, which is another `git rev-parse --git-dir` call.  The idea would be to just call `GetRepoPaths()` and either 1) return the error if not in a git repo or 2) cache the path results that much earlier. The real question there is the dataflow/architectural impact of doing that.  I'll have a look at that, and see what might be done.
Sounds good

Fix patch:
diff --git a/pkg/app/app.go b/pkg/app/app.go
index a16fbcc1fe7..e12461e2803 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 b0e1970dbe8..c2e77d4464a 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/status.go b/pkg/commands/git_commands/status.go
index 65b29deef13..0e0ef37fcc5 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 7bc36ff339b..2350d215e94 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 acdb2067220..0f2f2c7049c 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


Test patch:
diff --git a/pkg/commands/git_commands/repo_paths_test.go b/pkg/commands/git_commands/repo_paths_test.go
index 97cfc811908..9ee41a3fc8e 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 {