Skip to content

Commit

Permalink
Change RepoPaths to be acquired via RepoPathCache (#3284)
Browse files Browse the repository at this point in the history 
### **PR Description**
In order to optimize the number of git calls made this change implements
a RepoPathCache as the API by which RepoPaths instances are acquired.
This primarily affects app startup and worktree enumeration.

This introduces a new dependency on
[go-memoize](https://github.com/kofalt/go-memoize), which is a
lightweight wrapper around go-cache and singleflight, in order to ensure
that the cache is concurrency safe. (As compared to a simple map, e.g.)
See the go-memoize README for details.

Fixes #3227.

### **Please check if the PR fulfills these requirements**

* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [x] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [x] Docs (specifically `docs/Config.md`) have been updated if
necessary
* [x] You've read through your own file changes for silly mistakes etc
  • Loading branch information
@jesseduffield
jesseduffield committed Jul 7, 2024
2 parents a138a31 + 7a67096 commit b004b2e
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 48 deletions.
24 changes: 15 additions & 9 deletions pkg/app/app.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand All @@ -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)

Expand Down
35 changes: 24 additions & 11 deletions pkg/commands/git_commands/repo_paths.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package git_commands

import (
ioFs "io/fs"
"os"
"path"
"path/filepath"
"strings"
Expand All @@ -18,6 +19,7 @@ type RepoPaths struct {
repoPath string
repoGitDirPath string
repoName string
isBareRepo bool
}

var gitPathFormatVersion GitVersion = GitVersion{2, 31, 0, ""}
Expand Down Expand Up @@ -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{
Expand All @@ -62,14 +68,27 @@ func MockRepoPaths(currentPath string) *RepoPaths {
repoPath: currentPath,
repoGitDirPath: path.Join(currentPath, ".git"),
repoName: "lazygit",
isBareRepo: false,
}
}

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
}
Expand All @@ -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 {
Expand All @@ -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,
Expand Down
47 changes: 42 additions & 5 deletions pkg/commands/git_commands/repo_paths_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
Expand All @@ -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,
},
Expand All @@ -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)
},
Expand All @@ -78,14 +114,15 @@ func TestGetRepoPaths(t *testing.T) {
repoPath: "/path/to/repo/submodule1",
repoGitDirPath: "/path/to/repo/.git/modules/submodule1",
repoName: "submodule1",
isBareRepo: false,
},
Err: nil,
},
{
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"))
},
Expand All @@ -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),
)
},
},
Expand All @@ -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 {
Expand Down
18 changes: 2 additions & 16 deletions pkg/commands/git_commands/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/gui/dummies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
7 changes: 1 addition & 6 deletions pkg/gui/recent_repos_panel.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b004b2e

Please sign in to comment.