From 3d9f1e02e5063e1ce24f4b9122963eaef64b7262 Mon Sep 17 00:00:00 2001 From: John Whitley Date: Sun, 24 Dec 2023 08:46:02 -0800 Subject: [PATCH] Refactor repo_paths.go to use git rev-parse This changes GetRepoPaths() to pull information from `git rev-parse` instead of effectively reimplementing git's logic for pathfinding. This change fixes issues with bare repos, esp. versioned homedir use cases, by aligning lazygit's path handling to what git itself does. This change also enables lazygit to run from arbitrary subdirectories of a repository, including correct handling of symlinks, including "deep" symlinks into a repo, worktree, a repo's submodules, etc. Integration tests are now resilient against unintended side effects from the host's environment variables. Of necessity, $PATH and $TERM are the only env vars allowed through now. --- .editorconfig | 4 + pkg/commands/git.go | 63 +----- pkg/commands/git_commands/commit.go | 1 + pkg/commands/git_commands/commit_test.go | 15 +- pkg/commands/git_commands/diff.go | 9 +- pkg/commands/git_commands/repo_paths.go | 197 ++++-------------- pkg/commands/git_commands/repo_paths_test.go | 166 +++++++-------- pkg/commands/git_commands/stash.go | 1 + pkg/commands/git_commands/stash_test.go | 11 +- pkg/commands/git_commands/working_tree.go | 2 + .../git_commands/working_tree_test.go | 28 ++- pkg/commands/git_commands/worktree_loader.go | 33 ++- .../git_commands/worktree_loader_test.go | 37 +++- pkg/commands/git_test.go | 74 ------- pkg/commands/oscommands/cmd_obj_builder.go | 8 +- pkg/integration/components/env.go | 65 ++++++ pkg/integration/components/runner.go | 34 +-- pkg/integration/components/shell.go | 14 +- pkg/integration/components/test.go | 8 +- pkg/integration/tests/test_list.go | 3 + .../worktree/bare_repo_worktree_config.go | 92 ++++++++ .../double_nested_linked_submodule.go | 93 +++++++++ .../worktree/symlink_into_repo_subdir.go | 63 ++++++ pkg/tasks/tasks.go | 2 +- test/.gitconfig | 1 + 25 files changed, 586 insertions(+), 438 deletions(-) create mode 100644 .editorconfig delete mode 100644 pkg/commands/git_test.go create mode 100644 pkg/integration/components/env.go create mode 100644 pkg/integration/tests/worktree/bare_repo_worktree_config.go create mode 100644 pkg/integration/tests/worktree/double_nested_linked_submodule.go create mode 100644 pkg/integration/tests/worktree/symlink_into_repo_subdir.go create mode 120000 test/.gitconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000000..62f9670c83d --- /dev/null +++ b/.editorconfig @@ -0,0 +1,4 @@ +root = true + +[*.go] +indent_style = tab diff --git a/pkg/commands/git.go b/pkg/commands/git.go index b1b04a72fa0..b43c8c4e55a 100644 --- a/pkg/commands/git.go +++ b/pkg/commands/git.go @@ -2,12 +2,9 @@ package commands import ( "os" - "path" - "path/filepath" "strings" "github.com/go-errors/errors" - "github.com/spf13/afero" gogit "github.com/jesseduffield/go-git/v5" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" @@ -15,7 +12,6 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/patch" "github.com/jesseduffield/lazygit/pkg/common" - "github.com/jesseduffield/lazygit/pkg/env" "github.com/jesseduffield/lazygit/pkg/utils" ) @@ -64,39 +60,14 @@ func NewGitCommand( osCommand *oscommands.OSCommand, gitConfig git_config.IGitConfig, ) (*GitCommand, error) { - currentPath, err := os.Getwd() + repoPaths, err := git_commands.GetRepoPaths(osCommand.Cmd, version) if err != nil { - return nil, utils.WrapError(err) - } - - // converting to forward slashes for the sake of windows (which uses backwards slashes). We want everything - // to have forward slashes internally - currentPath = filepath.ToSlash(currentPath) - - gitDir := env.GetGitDirEnv() - if gitDir != "" { - // we've been given the git directory explicitly so no need to navigate to it - _, err := cmn.Fs.Stat(gitDir) - if err != nil { - return nil, utils.WrapError(err) - } - } else { - // we haven't been given the git dir explicitly so we assume it's in the current working directory as `.git/` (or an ancestor directory) - - rootDirectory, err := findWorktreeRoot(cmn.Fs, currentPath) - if err != nil { - return nil, utils.WrapError(err) - } - currentPath = rootDirectory - err = os.Chdir(rootDirectory) - if err != nil { - return nil, utils.WrapError(err) - } + return nil, errors.Errorf("Error getting repo paths: %v", err) } - repoPaths, err := git_commands.GetRepoPaths(cmn.Fs, currentPath) + err = os.Chdir(repoPaths.WorktreePath()) if err != nil { - return nil, errors.Errorf("Error getting repo paths: %v", err) + return nil, utils.WrapError(err) } repository, err := gogit.PlainOpenWithOptions( @@ -208,32 +179,6 @@ func NewGitCommandAux( } } -// this returns the root of the current worktree. So if you start lazygit from within -// a subdirectory of the worktree, it will start in the context of the root of that worktree -func findWorktreeRoot(fs afero.Fs, currentPath string) (string, error) { - for { - // we don't care if .git is a directory or a file: either is okay. - _, err := fs.Stat(path.Join(currentPath, ".git")) - - if err == nil { - return currentPath, nil - } - - if !os.IsNotExist(err) { - return "", utils.WrapError(err) - } - - currentPath = path.Dir(currentPath) - - atRoot := currentPath == path.Dir(currentPath) - if atRoot { - // we should never really land here: the code that creates GitCommand should - // verify we're in a git directory - return "", errors.New("Must open lazygit in a git repository") - } - } -} - func VerifyInGitRepo(osCommand *oscommands.OSCommand) error { return osCommand.Cmd.New(git_commands.NewGitCmd("rev-parse").Arg("--git-dir").ToArgv()).DontLog().Run() } diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index e0b5b8a9aeb..dfb0b408510 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -250,6 +250,7 @@ func (self *CommitCommands) ShowCmdObj(sha string, filterPath string) oscommands Arg(sha). ArgIf(self.AppState.IgnoreWhitespaceInDiffView, "--ignore-all-space"). ArgIf(filterPath != "", "--", filterPath). + Dir(self.repoPaths.worktreePath). ToArgv() return self.cmd.New(cmdArgs).DontLog() diff --git a/pkg/commands/git_commands/commit_test.go b/pkg/commands/git_commands/commit_test.go index 0ade25a8de5..a2b674eebff 100644 --- a/pkg/commands/git_commands/commit_test.go +++ b/pkg/commands/git_commands/commit_test.go @@ -197,7 +197,7 @@ func TestCommitShowCmdObj(t *testing.T) { contextSize: 3, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890"}, + expected: []string{"-C", "/path/to/worktree", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890"}, }, { testName: "Default case with filter path", @@ -205,7 +205,7 @@ func TestCommitShowCmdObj(t *testing.T) { contextSize: 3, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--", "file.txt"}, + expected: []string{"-C", "/path/to/worktree", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--", "file.txt"}, }, { testName: "Show diff with custom context size", @@ -213,7 +213,7 @@ func TestCommitShowCmdObj(t *testing.T) { contextSize: 77, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890"}, + expected: []string{"-C", "/path/to/worktree", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890"}, }, { testName: "Show diff, ignoring whitespace", @@ -221,7 +221,7 @@ func TestCommitShowCmdObj(t *testing.T) { contextSize: 77, ignoreWhitespace: true, extDiffCmd: "", - expected: []string{"show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--ignore-all-space"}, + expected: []string{"-C", "/path/to/worktree", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--ignore-all-space"}, }, { testName: "Show diff with external diff command", @@ -229,7 +229,7 @@ func TestCommitShowCmdObj(t *testing.T) { contextSize: 3, ignoreWhitespace: false, extDiffCmd: "difft --color=always", - expected: []string{"-c", "diff.external=difft --color=always", "show", "--ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.external=difft --color=always", "show", "--ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890"}, }, } @@ -243,7 +243,10 @@ func TestCommitShowCmdObj(t *testing.T) { appState.DiffContextSize = s.contextSize runner := oscommands.NewFakeRunner(t).ExpectGitArgs(s.expected, "", nil) - instance := buildCommitCommands(commonDeps{userConfig: userConfig, appState: appState, runner: runner}) + repoPaths := RepoPaths{ + worktreePath: "/path/to/worktree", + } + instance := buildCommitCommands(commonDeps{userConfig: userConfig, appState: appState, runner: runner, repoPaths: &repoPaths}) assert.NoError(t, instance.ShowCmdObj("1234567890", s.filterPath).Run()) runner.CheckForMissingCalls() diff --git a/pkg/commands/git_commands/diff.go b/pkg/commands/git_commands/diff.go index 3729390241a..73b30bc4848 100644 --- a/pkg/commands/git_commands/diff.go +++ b/pkg/commands/git_commands/diff.go @@ -14,14 +14,19 @@ func NewDiffCommands(gitCommon *GitCommon) *DiffCommands { func (self *DiffCommands) DiffCmdObj(diffArgs []string) oscommands.ICmdObj { return self.cmd.New( - NewGitCmd("diff").Arg("--submodule", "--no-ext-diff", "--color").Arg(diffArgs...).ToArgv(), + NewGitCmd("diff"). + Arg("--submodule", "--no-ext-diff", "--color"). + Arg(diffArgs...). + Dir(self.repoPaths.worktreePath). + ToArgv(), ) } func (self *DiffCommands) internalDiffCmdObj(diffArgs ...string) *GitCommandBuilder { return NewGitCmd("diff"). Arg("--no-ext-diff", "--no-color"). - Arg(diffArgs...) + Arg(diffArgs...). + Dir(self.repoPaths.worktreePath) } func (self *DiffCommands) GetPathDiff(path string, staged bool) (string, error) { diff --git a/pkg/commands/git_commands/repo_paths.go b/pkg/commands/git_commands/repo_paths.go index 13cda86a451..b0e1970dbe8 100644 --- a/pkg/commands/git_commands/repo_paths.go +++ b/pkg/commands/git_commands/repo_paths.go @@ -1,21 +1,18 @@ package git_commands import ( - "fmt" ioFs "io/fs" - "os" "path" "path/filepath" "strings" "github.com/go-errors/errors" - "github.com/jesseduffield/lazygit/pkg/env" - "github.com/samber/lo" + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/spf13/afero" ) type RepoPaths struct { - currentPath string worktreePath string worktreeGitDirPath string repoPath string @@ -23,12 +20,7 @@ type RepoPaths struct { repoName string } -// Current working directory of the program. Currently, this will always -// be the same as WorktreePath(), but in future we may support running -// lazygit from inside a subdirectory of the worktree. -func (self *RepoPaths) CurrentPath() string { - return self.currentPath -} +var gitPathFormatVersion GitVersion = GitVersion{2, 31, 0, ""} // Path to the current worktree. If we're in the main worktree, this will // be the same as RepoPath() @@ -65,7 +57,6 @@ func (self *RepoPaths) RepoName() string { // Returns the repo paths for a typical repo func MockRepoPaths(currentPath string) *RepoPaths { return &RepoPaths{ - currentPath: currentPath, worktreePath: currentPath, worktreeGitDirPath: path.Join(currentPath, ".git"), repoPath: currentPath, @@ -75,44 +66,41 @@ func MockRepoPaths(currentPath string) *RepoPaths { } func GetRepoPaths( - fs afero.Fs, - currentPath string, -) (*RepoPaths, error) { - return getRepoPathsAux(afero.NewOsFs(), resolveSymlink, currentPath) -} - -func getRepoPathsAux( - fs afero.Fs, - resolveSymlinkFn func(string) (string, error), - currentPath string, + cmd oscommands.ICmdObjBuilder, + version *GitVersion, ) (*RepoPaths, error) { - worktreePath := currentPath - repoGitDirPath, repoPath, err := getCurrentRepoGitDirPath(fs, resolveSymlinkFn, currentPath) + gitDirOutput, err := callGitRevParse(cmd, version, "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree") if err != nil { - return nil, errors.Errorf("failed to get repo git dir path: %v", err) + return nil, err } - var worktreeGitDirPath string - if env.GetWorkTreeEnv() != "" { - // This env is set when you pass --work-tree to lazygit. In that case, - // we're not dealing with a linked work-tree, we're dealing with a 'specified' - // worktree (for lack of a better term). In this case, the worktree has no - // .git file and it just contains a bunch of files: it has no idea it's - // pointed to by a bare repo. As such it does not have its own git dir within - // the bare repo's git dir. Instead, we just use the bare repo's git dir. - worktreeGitDirPath = repoGitDirPath - } else { - var err error - worktreeGitDirPath, err = getWorktreeGitDirPath(fs, currentPath) + gitDirResults := strings.Split(utils.NormalizeLinefeeds(gitDirOutput), "\n") + worktreePath := gitDirResults[0] + worktreeGitDirPath := gitDirResults[1] + repoGitDirPath := gitDirResults[2] + if version.IsOlderThanVersion(&gitPathFormatVersion) { + repoGitDirPath, err = filepath.Abs(repoGitDirPath) if err != nil { - return nil, errors.Errorf("failed to get worktree git dir path: %v", err) + return nil, err } } + // If we're in a submodule, --show-superproject-working-tree will return + // a value, meaning gitDirResults will be length 4. 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 + + var repoPath string + if isSubmodule { + repoPath = worktreePath + } else { + repoPath = path.Dir(repoGitDirPath) + } repoName := path.Base(repoPath) return &RepoPaths{ - currentPath: currentPath, worktreePath: worktreePath, worktreeGitDirPath: worktreeGitDirPath, repoPath: repoPath, @@ -121,124 +109,31 @@ func getRepoPathsAux( }, nil } -// Returns the path of the git-dir for the worktree. For linked worktrees, the worktree has -// a .git file that points to the git-dir (which itself lives in the git-dir -// of the repo) -func getWorktreeGitDirPath(fs afero.Fs, worktreePath string) (string, error) { - // if .git is a file, we're in a linked worktree, otherwise we're in - // the main worktree - dotGitPath := path.Join(worktreePath, ".git") - gitFileInfo, err := fs.Stat(dotGitPath) - if err != nil { - return "", err - } - - if gitFileInfo.IsDir() { - return dotGitPath, nil - } - - return linkedWorktreeGitDirPath(fs, worktreePath) +func callGitRevParse( + cmd oscommands.ICmdObjBuilder, + version *GitVersion, + gitRevArgs ...string, +) (string, error) { + return callGitRevParseWithDir(cmd, version, "", gitRevArgs...) } -func linkedWorktreeGitDirPath(fs afero.Fs, worktreePath string) (string, error) { - dotGitPath := path.Join(worktreePath, ".git") - gitFileContents, err := afero.ReadFile(fs, dotGitPath) - if err != nil { - return "", err - } - - // The file will have `gitdir: /path/to/.git/worktrees/` - gitDirLine := lo.Filter(strings.Split(string(gitFileContents), "\n"), func(line string, _ int) bool { - return strings.HasPrefix(line, "gitdir: ") - }) - - if len(gitDirLine) == 0 { - return "", errors.New(fmt.Sprintf("%s is a file which suggests we are in a submodule or a worktree but the file's contents do not contain a gitdir pointing to the actual .git directory", dotGitPath)) - } - - gitDir := strings.TrimPrefix(gitDirLine[0], "gitdir: ") - - gitDir = filepath.Clean(gitDir) - // For windows support - gitDir = filepath.ToSlash(gitDir) - - return gitDir, nil -} - -func getCurrentRepoGitDirPath( - fs afero.Fs, - resolveSymlinkFn func(string) (string, error), - currentPath string, -) (string, string, error) { - var unresolvedGitPath string - if env.GetGitDirEnv() != "" { - unresolvedGitPath = env.GetGitDirEnv() - } else { - unresolvedGitPath = path.Join(currentPath, ".git") +func callGitRevParseWithDir( + cmd oscommands.ICmdObjBuilder, + version *GitVersion, + dir string, + gitRevArgs ...string, +) (string, error) { + gitRevParse := NewGitCmd("rev-parse").ArgIf(version.IsAtLeastVersion(&gitPathFormatVersion), "--path-format=absolute").Arg(gitRevArgs...) + if dir != "" { + gitRevParse.Dir(dir) } - gitPath, err := resolveSymlinkFn(unresolvedGitPath) + gitCmd := cmd.New(gitRevParse.ToArgv()).DontLog() + res, err := gitCmd.RunWithOutput() if err != nil { - return "", "", err + return "", errors.Errorf("'%s' failed: %v", gitCmd.ToString(), err) } - - // check if .git is a file or a directory - gitFileInfo, err := fs.Stat(gitPath) - if err != nil { - return "", "", err - } - - if gitFileInfo.IsDir() { - // must be in the main worktree - return gitPath, path.Dir(gitPath), nil - } - - // either in a submodule, or worktree - worktreeGitPath, err := linkedWorktreeGitDirPath(fs, currentPath) - if err != nil { - return "", "", errors.Errorf("could not find git dir for %s: %v", currentPath, err) - } - - _, err = fs.Stat(worktreeGitPath) - if err != nil { - if os.IsNotExist(err) { - // hardcoding error to get around windows-specific error message - return "", "", errors.Errorf("could not find git dir for %s. %s does not exist", currentPath, worktreeGitPath) - } - return "", "", errors.Errorf("could not find git dir for %s: %v", currentPath, err) - } - - // confirm whether the next directory up is the worktrees directory - parent := path.Dir(worktreeGitPath) - if path.Base(parent) == "worktrees" { - gitDirPath := path.Dir(parent) - return gitDirPath, path.Dir(gitDirPath), nil - } - - // Unlike worktrees, submodules can be nested arbitrarily deep, so we check - // if the `modules` directory is anywhere up the chain. - if strings.Contains(worktreeGitPath, "/modules/") { - // For submodules, we just return the path directly - return worktreeGitPath, currentPath, nil - } - - // If this error causes issues, we could relax the constraint and just always - // return the path - return "", "", errors.Errorf("could not find git dir for %s: the path '%s' is not under `worktrees` or `modules` directories", currentPath, worktreeGitPath) -} - -// takes a path containing a symlink and returns the true path -func resolveSymlink(path string) (string, error) { - l, err := os.Lstat(path) - if err != nil { - return "", err - } - - if l.Mode()&os.ModeSymlink == 0 { - return path, nil - } - - return filepath.EvalSymlinks(path) + return strings.TrimSpace(res), nil } // Returns the paths of linked worktrees diff --git a/pkg/commands/git_commands/repo_paths_test.go b/pkg/commands/git_commands/repo_paths_test.go index 5e6275522e9..ae452673771 100644 --- a/pkg/commands/git_commands/repo_paths_test.go +++ b/pkg/commands/git_commands/repo_paths_test.go @@ -1,34 +1,50 @@ package git_commands import ( + "fmt" + "strings" "testing" "github.com/go-errors/errors" - "github.com/spf13/afero" + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/stretchr/testify/assert" ) -func mockResolveSymlinkFn(p string) (string, error) { return p, nil } +type ( + argFn func() []string + errFn func(getRevParseArgs argFn) error +) type Scenario struct { Name string - BeforeFunc func(fs afero.Fs) + BeforeFunc func(runner *oscommands.FakeCmdObjRunner, getRevParseArgs argFn) Path string Expected *RepoPaths - Err error + Err errFn } -func TestGetRepoPathsAux(t *testing.T) { +func TestGetRepoPaths(t *testing.T) { scenarios := []Scenario{ { Name: "typical case", - BeforeFunc: func(fs afero.Fs) { + BeforeFunc: func(runner *oscommands.FakeCmdObjRunner, getRevParseArgs argFn) { // setup for main worktree - _ = fs.MkdirAll("/path/to/repo/.git", 0o755) + expectedOutput := []string{ + // --show-toplevel + "/path/to/repo", + // --git-dir + "/path/to/repo/.git", + // --git-common-dir + "/path/to/repo/.git", + // --show-superproject-working-tree + } + runner.ExpectGitArgs( + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree"), + strings.Join(expectedOutput, "\n"), + nil) }, Path: "/path/to/repo", Expected: &RepoPaths{ - currentPath: "/path/to/repo", worktreePath: "/path/to/repo", worktreeGitDirPath: "/path/to/repo/.git", repoPath: "/path/to/repo", @@ -37,71 +53,26 @@ func TestGetRepoPathsAux(t *testing.T) { }, Err: nil, }, - { - Name: "linked worktree", - BeforeFunc: func(fs afero.Fs) { - // setup for linked worktree - _ = fs.MkdirAll("/path/to/repo/.git/worktrees/worktree1", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/worktree1/.git", []byte("gitdir: /path/to/repo/.git/worktrees/worktree1"), 0o644) - }, - Path: "/path/to/repo/worktree1", - Expected: &RepoPaths{ - currentPath: "/path/to/repo/worktree1", - worktreePath: "/path/to/repo/worktree1", - worktreeGitDirPath: "/path/to/repo/.git/worktrees/worktree1", - repoPath: "/path/to/repo", - repoGitDirPath: "/path/to/repo/.git", - repoName: "repo", - }, - Err: nil, - }, - { - Name: "worktree with trailing separator in path", - BeforeFunc: func(fs afero.Fs) { - // setup for linked worktree - _ = fs.MkdirAll("/path/to/repo/.git/worktrees/worktree1", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/worktree1/.git", []byte("gitdir: /path/to/repo/.git/worktrees/worktree1/"), 0o644) - }, - Path: "/path/to/repo/worktree1", - Expected: &RepoPaths{ - currentPath: "/path/to/repo/worktree1", - worktreePath: "/path/to/repo/worktree1", - worktreeGitDirPath: "/path/to/repo/.git/worktrees/worktree1", - repoPath: "/path/to/repo", - repoGitDirPath: "/path/to/repo/.git", - repoName: "repo", - }, - Err: nil, - }, - { - Name: "worktree .git file missing gitdir directive", - BeforeFunc: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git/worktrees/worktree2", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/worktree2/.git", []byte("blah"), 0o644) - }, - Path: "/path/to/repo/worktree2", - Expected: nil, - Err: errors.New("failed to get repo git dir path: could not find git dir for /path/to/repo/worktree2: /path/to/repo/worktree2/.git is a file which suggests we are in a submodule or a worktree but the file's contents do not contain a gitdir pointing to the actual .git directory"), - }, - { - Name: "worktree .git file gitdir directive points to a non-existing directory", - BeforeFunc: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git/worktrees/worktree2", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/worktree2/.git", []byte("gitdir: /nonexistant"), 0o644) - }, - Path: "/path/to/repo/worktree2", - Expected: nil, - Err: errors.New("failed to get repo git dir path: could not find git dir for /path/to/repo/worktree2. /nonexistant does not exist"), - }, { Name: "submodule", - BeforeFunc: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git/modules/submodule1", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/submodule1/.git", []byte("gitdir: /path/to/repo/.git/modules/submodule1"), 0o644) + BeforeFunc: func(runner *oscommands.FakeCmdObjRunner, getRevParseArgs argFn) { + expectedOutput := []string{ + // --show-toplevel + "/path/to/repo/submodule1", + // --git-dir + "/path/to/repo/.git/modules/submodule1", + // --git-common-dir + "/path/to/repo/.git/modules/submodule1", + // --show-superproject-working-tree + "/path/to/repo", + } + runner.ExpectGitArgs( + append(getRevParseArgs(), "--show-toplevel", "--absolute-git-dir", "--git-common-dir", "--show-superproject-working-tree"), + strings.Join(expectedOutput, "\n"), + nil) }, Path: "/path/to/repo/submodule1", Expected: &RepoPaths{ - currentPath: "/path/to/repo/submodule1", worktreePath: "/path/to/repo/submodule1", worktreeGitDirPath: "/path/to/repo/.git/modules/submodule1", repoPath: "/path/to/repo/submodule1", @@ -111,49 +82,52 @@ func TestGetRepoPathsAux(t *testing.T) { Err: nil, }, { - Name: "submodule in nested directory", - BeforeFunc: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git/modules/my/submodule1", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/my/submodule1/.git", []byte("gitdir: /path/to/repo/.git/modules/my/submodule1"), 0o644) - }, - Path: "/path/to/repo/my/submodule1", - Expected: &RepoPaths{ - currentPath: "/path/to/repo/my/submodule1", - worktreePath: "/path/to/repo/my/submodule1", - worktreeGitDirPath: "/path/to/repo/.git/modules/my/submodule1", - repoPath: "/path/to/repo/my/submodule1", - repoGitDirPath: "/path/to/repo/.git/modules/my/submodule1", - repoName: "submodule1", - }, - Err: nil, - }, - { - Name: "submodule git dir not under .git/modules", - BeforeFunc: func(fs afero.Fs) { - _ = fs.MkdirAll("/random/submodule1", 0o755) - _ = afero.WriteFile(fs, "/path/to/repo/my/submodule1/.git", []byte("gitdir: /random/submodule1"), 0o644) + 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"), + "", + errors.New("fatal: invalid gitfile format: /path/to/repo/worktree2/.git")) }, - Path: "/path/to/repo/my/submodule1", + Path: "/path/to/repo/worktree2", Expected: nil, - Err: errors.New("failed to get repo git dir path: could not find git dir for /path/to/repo/my/submodule1: the path '/random/submodule1' is not under `worktrees` or `modules` directories"), + 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), + ) + }, }, } for _, s := range scenarios { s := s t.Run(s.Name, func(t *testing.T) { - fs := afero.NewMemMapFs() + runner := oscommands.NewFakeRunner(t) + cmd := oscommands.NewDummyCmdObjBuilder(runner) + version, err := GetGitVersion(oscommands.NewDummyOSCommand()) + if err != nil { + t.Fatal(err) + } + + getRevParseArgs := func() []string { + args := []string{"rev-parse"} + if version.IsAtLeast(2, 31, 0) { + args = append(args, "--path-format=absolute") + } + return args + } // prepare the filesystem for the scenario - s.BeforeFunc(fs) + s.BeforeFunc(runner, getRevParseArgs) - // run the function with the scenario path - repoPaths, err := getRepoPathsAux(fs, mockResolveSymlinkFn, s.Path) + repoPaths, err := GetRepoPaths(cmd, version) // check the error and the paths if s.Err != nil { + scenarioErr := s.Err(getRevParseArgs) assert.Error(t, err) - assert.EqualError(t, err, s.Err.Error()) + assert.EqualError(t, err, scenarioErr.Error()) } else { assert.Nil(t, err) assert.Equal(t, s.Expected, repoPaths) diff --git a/pkg/commands/git_commands/stash.go b/pkg/commands/git_commands/stash.go index fad73a472a9..29140e68e76 100644 --- a/pkg/commands/git_commands/stash.go +++ b/pkg/commands/git_commands/stash.go @@ -88,6 +88,7 @@ func (self *StashCommands) ShowStashEntryCmdObj(index int) oscommands.ICmdObj { Arg(fmt.Sprintf("--unified=%d", self.AppState.DiffContextSize)). ArgIf(self.AppState.IgnoreWhitespaceInDiffView, "--ignore-all-space"). Arg(fmt.Sprintf("stash@{%d}", index)). + Dir(self.repoPaths.worktreePath). ToArgv() return self.cmd.New(cmdArgs).DontLog() diff --git a/pkg/commands/git_commands/stash_test.go b/pkg/commands/git_commands/stash_test.go index 846c493ee54..6954a3cf817 100644 --- a/pkg/commands/git_commands/stash_test.go +++ b/pkg/commands/git_commands/stash_test.go @@ -112,21 +112,21 @@ func TestStashStashEntryCmdObj(t *testing.T) { index: 5, contextSize: 3, ignoreWhitespace: false, - expected: []string{"git", "stash", "show", "-p", "--stat", "--color=always", "--unified=3", "stash@{5}"}, + expected: []string{"git", "-C", "/path/to/worktree", "stash", "show", "-p", "--stat", "--color=always", "--unified=3", "stash@{5}"}, }, { testName: "Show diff with custom context size", index: 5, contextSize: 77, ignoreWhitespace: false, - expected: []string{"git", "stash", "show", "-p", "--stat", "--color=always", "--unified=77", "stash@{5}"}, + expected: []string{"git", "-C", "/path/to/worktree", "stash", "show", "-p", "--stat", "--color=always", "--unified=77", "stash@{5}"}, }, { testName: "Default case", index: 5, contextSize: 3, ignoreWhitespace: true, - expected: []string{"git", "stash", "show", "-p", "--stat", "--color=always", "--unified=3", "--ignore-all-space", "stash@{5}"}, + expected: []string{"git", "-C", "/path/to/worktree", "stash", "show", "-p", "--stat", "--color=always", "--unified=3", "--ignore-all-space", "stash@{5}"}, }, } @@ -137,7 +137,10 @@ func TestStashStashEntryCmdObj(t *testing.T) { appState := &config.AppState{} appState.IgnoreWhitespaceInDiffView = s.ignoreWhitespace appState.DiffContextSize = s.contextSize - instance := buildStashCommands(commonDeps{userConfig: userConfig, appState: appState}) + repoPaths := RepoPaths{ + worktreePath: "/path/to/worktree", + } + instance := buildStashCommands(commonDeps{userConfig: userConfig, appState: appState, repoPaths: &repoPaths}) cmdStr := instance.ShowStashEntryCmdObj(s.index).Args() assert.Equal(t, s.expected, cmdStr) diff --git a/pkg/commands/git_commands/working_tree.go b/pkg/commands/git_commands/working_tree.go index 9630e2870ee..51ad417aa1c 100644 --- a/pkg/commands/git_commands/working_tree.go +++ b/pkg/commands/git_commands/working_tree.go @@ -255,6 +255,7 @@ func (self *WorkingTreeCommands) WorktreeFileDiffCmdObj(node models.IFile, plain ArgIf(noIndex, "/dev/null"). Arg(node.GetPath()). ArgIf(prevPath != "", prevPath). + Dir(self.repoPaths.worktreePath). ToArgv() return self.cmd.New(cmdArgs).DontLog() @@ -290,6 +291,7 @@ func (self *WorkingTreeCommands) ShowFileDiffCmdObj(from string, to string, reve ArgIf(!plain && self.AppState.IgnoreWhitespaceInDiffView, "--ignore-all-space"). Arg("--"). Arg(fileName). + Dir(self.repoPaths.worktreePath). ToArgv() return self.cmd.New(cmdArgs).DontLog() diff --git a/pkg/commands/git_commands/working_tree_test.go b/pkg/commands/git_commands/working_tree_test.go index cc46d4994b6..51fa88b7ff2 100644 --- a/pkg/commands/git_commands/working_tree_test.go +++ b/pkg/commands/git_commands/working_tree_test.go @@ -231,7 +231,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--", "test.txt"}, expectedResult, nil), }, { testName: "cached", @@ -245,7 +245,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--cached", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--cached", "--", "test.txt"}, expectedResult, nil), }, { testName: "plain", @@ -259,7 +259,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=never", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=never", "--", "test.txt"}, expectedResult, nil), }, { testName: "File not tracked and file has no staged changes", @@ -273,7 +273,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--no-index", "--", "/dev/null", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--no-index", "--", "/dev/null", "test.txt"}, expectedResult, nil), }, { testName: "Default case (ignore whitespace)", @@ -287,7 +287,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: true, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--ignore-all-space", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--color=always", "--ignore-all-space", "--", "test.txt"}, expectedResult, nil), }, { testName: "Show diff with custom context size", @@ -301,7 +301,7 @@ func TestWorkingTreeDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 17, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=17", "--color=always", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=17", "--color=always", "--", "test.txt"}, expectedResult, nil), }, } @@ -312,8 +312,11 @@ func TestWorkingTreeDiff(t *testing.T) { appState := &config.AppState{} appState.IgnoreWhitespaceInDiffView = s.ignoreWhitespace appState.DiffContextSize = s.contextSize + repoPaths := RepoPaths{ + worktreePath: "/path/to/worktree", + } - instance := buildWorkingTreeCommands(commonDeps{runner: s.runner, userConfig: userConfig, appState: appState}) + instance := buildWorkingTreeCommands(commonDeps{runner: s.runner, userConfig: userConfig, appState: appState, repoPaths: &repoPaths}) result := instance.WorktreeFileDiff(s.file, s.plain, s.cached) assert.Equal(t, expectedResult, result) s.runner.CheckForMissingCalls() @@ -345,7 +348,7 @@ func TestWorkingTreeShowFileDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--no-renames", "--color=always", "1234567890", "0987654321", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--no-renames", "--color=always", "1234567890", "0987654321", "--", "test.txt"}, expectedResult, nil), }, { testName: "Show diff with custom context size", @@ -356,7 +359,7 @@ func TestWorkingTreeShowFileDiff(t *testing.T) { ignoreWhitespace: false, contextSize: 123, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=123", "--no-renames", "--color=always", "1234567890", "0987654321", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=123", "--no-renames", "--color=always", "1234567890", "0987654321", "--", "test.txt"}, expectedResult, nil), }, { testName: "Default case (ignore whitespace)", @@ -367,7 +370,7 @@ func TestWorkingTreeShowFileDiff(t *testing.T) { ignoreWhitespace: true, contextSize: 3, runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"diff", "--no-ext-diff", "--submodule", "--unified=3", "--no-renames", "--color=always", "1234567890", "0987654321", "--ignore-all-space", "--", "test.txt"}, expectedResult, nil), + ExpectGitArgs([]string{"-C", "/path/to/worktree", "diff", "--no-ext-diff", "--submodule", "--unified=3", "--no-renames", "--color=always", "1234567890", "0987654321", "--ignore-all-space", "--", "test.txt"}, expectedResult, nil), }, } @@ -378,8 +381,11 @@ func TestWorkingTreeShowFileDiff(t *testing.T) { appState := &config.AppState{} appState.IgnoreWhitespaceInDiffView = s.ignoreWhitespace appState.DiffContextSize = s.contextSize + repoPaths := RepoPaths{ + worktreePath: "/path/to/worktree", + } - instance := buildWorkingTreeCommands(commonDeps{runner: s.runner, userConfig: userConfig, appState: appState}) + instance := buildWorkingTreeCommands(commonDeps{runner: s.runner, userConfig: userConfig, appState: appState, repoPaths: &repoPaths}) result, err := instance.ShowFileDiff(s.from, s.to, s.reverse, "test.txt", s.plain) assert.NoError(t, err) diff --git a/pkg/commands/git_commands/worktree_loader.go b/pkg/commands/git_commands/worktree_loader.go index 0d5f01e3433..97839662cab 100644 --- a/pkg/commands/git_commands/worktree_loader.go +++ b/pkg/commands/git_commands/worktree_loader.go @@ -4,6 +4,7 @@ import ( iofs "io/fs" "path/filepath" "strings" + "sync" "github.com/go-errors/errors" "github.com/jesseduffield/lazygit/pkg/commands/models" @@ -57,18 +58,14 @@ func (self *WorktreeLoader) GetWorktrees() ([]*models.Worktree, error) { isCurrent := path == worktreePath isPathMissing := self.pathExists(path) - var gitDir string - gitDir, err := getWorktreeGitDirPath(self.Fs, path) - if err != nil { - self.Log.Warnf("Could not find git dir for worktree %s: %v", path, err) - } - current = &models.Worktree{ IsMain: isMain, IsCurrent: isCurrent, IsPathMissing: isPathMissing, Path: path, - GitDir: gitDir, + // we defer populating GitDir until a loop below so that + // we can parallelize the calls to git rev-parse + GitDir: "", } } else if strings.HasPrefix(splitLine, "branch ") { branch := strings.SplitN(splitLine, " ", 2)[1] @@ -76,6 +73,28 @@ func (self *WorktreeLoader) GetWorktrees() ([]*models.Worktree, error) { } } + wg := sync.WaitGroup{} + wg.Add(len(worktrees)) + for _, worktree := range worktrees { + worktree := worktree + + go utils.Safe(func() { + defer wg.Done() + + if worktree.IsPathMissing { + return + } + gitDir, err := callGitRevParseWithDir(self.cmd, self.version, worktree.Path, "--absolute-git-dir") + if err != nil { + self.Log.Warnf("Could not find git dir for worktree %s: %v", worktree.Path, err) + return + } + + worktree.GitDir = gitDir + }) + } + wg.Wait() + names := getUniqueNamesFromPaths(lo.Map(worktrees, func(worktree *models.Worktree, _ int) string { return worktree.Path })) diff --git a/pkg/commands/git_commands/worktree_loader_test.go b/pkg/commands/git_commands/worktree_loader_test.go index 12b30807496..02ed73e865b 100644 --- a/pkg/commands/git_commands/worktree_loader_test.go +++ b/pkg/commands/git_commands/worktree_loader_test.go @@ -14,7 +14,7 @@ func TestGetWorktrees(t *testing.T) { type scenario struct { testName string repoPaths *RepoPaths - before func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs) + before func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs, getRevParseArgs argFn) expectedWorktrees []*models.Worktree expectedErr string } @@ -26,7 +26,7 @@ func TestGetWorktrees(t *testing.T) { repoPath: "/path/to/repo", worktreePath: "/path/to/repo", }, - before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs) { + before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs, getRevParseArgs argFn) { runner.ExpectGitArgs([]string{"worktree", "list", "--porcelain"}, `worktree /path/to/repo HEAD d85cc9d281fa6ae1665c68365fc70e75e82a042d @@ -34,6 +34,8 @@ branch refs/heads/mybranch `, nil) + gitArgsMainWorktree := append(append([]string{"-C", "/path/to/repo"}, getRevParseArgs()...), "--absolute-git-dir") + runner.ExpectGitArgs(gitArgsMainWorktree, "/path/to/repo/.git", nil) _ = fs.MkdirAll("/path/to/repo/.git", 0o755) }, expectedWorktrees: []*models.Worktree{ @@ -55,7 +57,7 @@ branch refs/heads/mybranch repoPath: "/path/to/repo", worktreePath: "/path/to/repo", }, - before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs) { + before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs, getRevParseArgs argFn) { runner.ExpectGitArgs([]string{"worktree", "list", "--porcelain"}, `worktree /path/to/repo HEAD d85cc9d281fa6ae1665c68365fc70e75e82a042d @@ -66,6 +68,10 @@ HEAD 775955775e79b8f5b4c4b56f82fbf657e2d5e4de branch refs/heads/mybranch-worktree `, nil) + gitArgsMainWorktree := append(append([]string{"-C", "/path/to/repo"}, getRevParseArgs()...), "--absolute-git-dir") + runner.ExpectGitArgs(gitArgsMainWorktree, "/path/to/repo/.git", nil) + gitArgsLinkedWorktree := append(append([]string{"-C", "/path/to/repo-worktree"}, getRevParseArgs()...), "--absolute-git-dir") + runner.ExpectGitArgs(gitArgsLinkedWorktree, "/path/to/repo/.git/worktrees/repo-worktree", nil) _ = fs.MkdirAll("/path/to/repo/.git", 0o755) _ = fs.MkdirAll("/path/to/repo-worktree", 0o755) @@ -100,7 +106,7 @@ branch refs/heads/mybranch-worktree repoPath: "/path/to/repo", worktreePath: "/path/to/repo", }, - before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs) { + before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs, getRevParseArgs argFn) { runner.ExpectGitArgs([]string{"worktree", "list", "--porcelain"}, `worktree /path/to/worktree HEAD 775955775e79b8f5b4c4b56f82fbf657e2d5e4de @@ -129,7 +135,7 @@ branch refs/heads/missingbranch repoPath: "/path/to/repo", worktreePath: "/path/to/repo-worktree", }, - before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs) { + before: func(runner *oscommands.FakeCmdObjRunner, fs afero.Fs, getRevParseArgs argFn) { runner.ExpectGitArgs([]string{"worktree", "list", "--porcelain"}, `worktree /path/to/repo HEAD d85cc9d281fa6ae1665c68365fc70e75e82a042d @@ -140,6 +146,10 @@ HEAD 775955775e79b8f5b4c4b56f82fbf657e2d5e4de branch refs/heads/mybranch-worktree `, nil) + gitArgsMainWorktree := append(append([]string{"-C", "/path/to/repo"}, getRevParseArgs()...), "--absolute-git-dir") + runner.ExpectGitArgs(gitArgsMainWorktree, "/path/to/repo/.git", nil) + gitArgsLinkedWorktree := append(append([]string{"-C", "/path/to/repo-worktree"}, getRevParseArgs()...), "--absolute-git-dir") + runner.ExpectGitArgs(gitArgsLinkedWorktree, "/path/to/repo/.git/worktrees/repo-worktree", nil) _ = fs.MkdirAll("/path/to/repo/.git", 0o755) _ = fs.MkdirAll("/path/to/repo-worktree", 0o755) @@ -175,10 +185,23 @@ branch refs/heads/mybranch-worktree t.Run(s.testName, func(t *testing.T) { runner := oscommands.NewFakeRunner(t) fs := afero.NewMemMapFs() - s.before(runner, fs) + version, err := GetGitVersion(oscommands.NewDummyOSCommand()) + if err != nil { + t.Fatal(err) + } + + getRevParseArgs := func() []string { + args := []string{"rev-parse"} + if version.IsAtLeast(2, 31, 0) { + args = append(args, "--path-format=absolute") + } + return args + } + + s.before(runner, fs, getRevParseArgs) loader := &WorktreeLoader{ - GitCommon: buildGitCommon(commonDeps{runner: runner, fs: fs, repoPaths: s.repoPaths}), + GitCommon: buildGitCommon(commonDeps{runner: runner, fs: fs, repoPaths: s.repoPaths, gitVersion: version}), } worktrees, err := loader.GetWorktrees() diff --git a/pkg/commands/git_test.go b/pkg/commands/git_test.go deleted file mode 100644 index f4dc27dedbe..00000000000 --- a/pkg/commands/git_test.go +++ /dev/null @@ -1,74 +0,0 @@ -package commands - -import ( - "testing" - - "github.com/go-errors/errors" - "github.com/spf13/afero" - "github.com/stretchr/testify/assert" -) - -func TestFindWorktreeRoot(t *testing.T) { - type scenario struct { - testName string - currentPath string - before func(fs afero.Fs) - expectedPath string - expectedErr string - } - - scenarios := []scenario{ - { - testName: "at root of worktree", - currentPath: "/path/to/repo", - before: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git", 0o755) - }, - expectedPath: "/path/to/repo", - expectedErr: "", - }, - { - testName: "inside worktree", - currentPath: "/path/to/repo/subdir", - before: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/repo/.git", 0o755) - _ = fs.MkdirAll("/path/to/repo/subdir", 0o755) - }, - expectedPath: "/path/to/repo", - expectedErr: "", - }, - { - testName: "not in a git repo", - currentPath: "/path/to/dir", - before: func(fs afero.Fs) {}, - expectedPath: "", - expectedErr: "Must open lazygit in a git repository", - }, - { - testName: "In linked worktree", - currentPath: "/path/to/worktree", - before: func(fs afero.Fs) { - _ = fs.MkdirAll("/path/to/worktree", 0o755) - _ = afero.WriteFile(fs, "/path/to/worktree/.git", []byte("blah"), 0o755) - }, - expectedPath: "/path/to/worktree", - expectedErr: "", - }, - } - - for _, s := range scenarios { - s := s - t.Run(s.testName, func(t *testing.T) { - fs := afero.NewMemMapFs() - s.before(fs) - - root, err := findWorktreeRoot(fs, s.currentPath) - if s.expectedErr != "" { - assert.EqualError(t, errors.New(s.expectedErr), err.Error()) - } else { - assert.NoError(t, err) - assert.Equal(t, s.expectedPath, root) - } - }) - } -} diff --git a/pkg/commands/oscommands/cmd_obj_builder.go b/pkg/commands/oscommands/cmd_obj_builder.go index 77fb7e7c964..540487ed46e 100644 --- a/pkg/commands/oscommands/cmd_obj_builder.go +++ b/pkg/commands/oscommands/cmd_obj_builder.go @@ -27,8 +27,14 @@ type CmdObjBuilder struct { var _ ICmdObjBuilder = &CmdObjBuilder{} func (self *CmdObjBuilder) New(args []string) ICmdObj { + cmdObj := self.NewWithEnviron(args, os.Environ()) + return cmdObj +} + +// A command with explicit environment from env +func (self *CmdObjBuilder) NewWithEnviron(args []string, env []string) ICmdObj { cmd := exec.Command(args[0], args[1:]...) - cmd.Env = os.Environ() + cmd.Env = env return &CmdObj{ cmd: cmd, diff --git a/pkg/integration/components/env.go b/pkg/integration/components/env.go new file mode 100644 index 00000000000..d3cdf467f60 --- /dev/null +++ b/pkg/integration/components/env.go @@ -0,0 +1,65 @@ +package components + +import ( + "fmt" + "os" +) + +const ( + // These values will be passed to lazygit + LAZYGIT_ROOT_DIR = "LAZYGIT_ROOT_DIR" + SANDBOX_ENV_VAR = "SANDBOX" + TEST_NAME_ENV_VAR = "TEST_NAME" + WAIT_FOR_DEBUGGER_ENV_VAR = "WAIT_FOR_DEBUGGER" + + // These values will be passed to both lazygit and shell commands + GIT_CONFIG_GLOBAL_ENV_VAR = "GIT_CONFIG_GLOBAL" + // We pass PWD because if it's defined, Go will use it as the working directory + // rather than make a syscall to the OS, and that means symlinks won't be resolved, + // which is good to test for. + PWD = "PWD" + + // We set $HOME and $GIT_CONFIG_NOGLOBAL during integrationt tests so + // that older versions of git that don't respect $GIT_CONFIG_GLOBAL + // will find the correct global config file for testing + HOME = "HOME" + GIT_CONFIG_NOGLOBAL = "GIT_CONFIG_NOGLOBAL" + + // These values will be passed through to lazygit and shell commands, with their + // values inherited from the host environment + PATH = "PATH" + TERM = "TERM" +) + +// Tests will inherit these environment variables from the host environment, rather +// than the test runner deciding the values itself. +// All other environment variables present in the host environment will be ignored. +// Having such a minimal list ensures that lazygit behaves the same across different test environments. +var hostEnvironmentAllowlist = [...]string{ + PATH, + TERM, +} + +// Returns a copy of the environment filtered by +// hostEnvironmentAllowlist +func allowedHostEnvironment() []string { + env := []string{} + for _, envVar := range hostEnvironmentAllowlist { + env = append(env, fmt.Sprintf("%s=%s", envVar, os.Getenv(envVar))) + } + return env +} + +func NewTestEnvironment(rootDir string) []string { + env := allowedHostEnvironment() + + // Set $HOME to control the global git config location for git + // versions <= 2.31.8 + env = append(env, fmt.Sprintf("%s=%s", HOME, testPath(rootDir))) + + // $GIT_CONFIG_GLOBAL controls global git config location for git + // versions >= 2.32.0 + env = append(env, fmt.Sprintf("%s=%s", GIT_CONFIG_GLOBAL_ENV_VAR, globalGitConfigPath(rootDir))) + + return env +} diff --git a/pkg/integration/components/runner.go b/pkg/integration/components/runner.go index 064dd5c5b26..c148dc14163 100644 --- a/pkg/integration/components/runner.go +++ b/pkg/integration/components/runner.go @@ -13,14 +13,6 @@ import ( "github.com/samber/lo" ) -const ( - LAZYGIT_ROOT_DIR = "LAZYGIT_ROOT_DIR" - TEST_NAME_ENV_VAR = "TEST_NAME" - SANDBOX_ENV_VAR = "SANDBOX" - WAIT_FOR_DEBUGGER_ENV_VAR = "WAIT_FOR_DEBUGGER" - GIT_CONFIG_GLOBAL_ENV_VAR = "GIT_CONFIG_GLOBAL" -) - type RunTestArgs struct { Tests []*IntegrationTest Logf func(format string, formatArgs ...interface{}) @@ -161,18 +153,27 @@ func buildLazygit(testArgs RunTestArgs) error { // Sets up the fixture for test and returns the working directory to invoke // lazygit in. func createFixture(test *IntegrationTest, paths Paths, rootDir string) string { - shell := NewShell(paths.ActualRepo(), func(errorMsg string) { panic(errorMsg) }) + env := NewTestEnvironment(rootDir) + + env = append(env, fmt.Sprintf("%s=%s", PWD, paths.ActualRepo())) + shell := NewShell( + paths.ActualRepo(), + env, + func(errorMsg string) { panic(errorMsg) }, + ) shell.Init() - os.Setenv(GIT_CONFIG_GLOBAL_ENV_VAR, globalGitConfigPath(rootDir)) - test.SetupRepo(shell) return shell.dir } +func testPath(rootdir string) string { + return filepath.Join(rootdir, "test") +} + func globalGitConfigPath(rootDir string) string { - return filepath.Join(rootDir, "test", "global_git_config") + return filepath.Join(testPath(rootDir), "global_git_config") } func getGitVersion() (*git_commands.GitVersion, error) { @@ -215,9 +216,16 @@ func getLazygitCommand( }) cmdArgs = append(cmdArgs, resolvedExtraArgs...) - cmdObj := osCommand.Cmd.New(cmdArgs) + // Use a limited environment for test isolation, including pass through + // of just allowed host environment variables + cmdObj := osCommand.Cmd.NewWithEnviron(cmdArgs, NewTestEnvironment(rootDir)) + // Integration tests related to symlink behavior need a PWD that + // preserves symlinks. By default, SetWd will set a symlink-resolved + // value for PWD. Here, we override that with the path (that may) + // contain a symlink to simulate behavior in a user's shell correctly. cmdObj.SetWd(workingDir) + cmdObj.AddEnvVars(fmt.Sprintf("%s=%s", PWD, workingDir)) cmdObj.AddEnvVars(fmt.Sprintf("%s=%s", LAZYGIT_ROOT_DIR, rootDir)) diff --git a/pkg/integration/components/shell.go b/pkg/integration/components/shell.go index 5a3bbaa29a8..48ff3fdf734 100644 --- a/pkg/integration/components/shell.go +++ b/pkg/integration/components/shell.go @@ -19,6 +19,9 @@ import ( type Shell struct { // working directory the shell is invoked in dir string + // passed into each command + env []string + // when running the shell outside the gui we can directly panic on failure, // but inside the gui we need to close the gui before panicking fail func(string) @@ -26,14 +29,15 @@ type Shell struct { randomFileContentIndex int } -func NewShell(dir string, fail func(string)) *Shell { - return &Shell{dir: dir, fail: fail} +func NewShell(dir string, env []string, fail func(string)) *Shell { + return &Shell{dir: dir, env: env, fail: fail} } func (self *Shell) RunCommand(args []string) *Shell { return self.RunCommandWithEnv(args, []string{}) } +// Run a command with additional environment variables set func (self *Shell) RunCommandWithEnv(args []string, env []string) *Shell { output, err := self.runCommandWithOutputAndEnv(args, env) if err != nil { @@ -58,7 +62,7 @@ func (self *Shell) runCommandWithOutput(args []string) (string, error) { func (self *Shell) runCommandWithOutputAndEnv(args []string, env []string) (string, error) { cmd := exec.Command(args[0], args[1:]...) - cmd.Env = append(os.Environ(), env...) + cmd.Env = append(self.env, env...) cmd.Dir = self.dir output, err := cmd.CombinedOutput() @@ -461,8 +465,8 @@ func (self *Shell) CopyFile(source string, destination string) *Shell { return self } -// NOTE: this only takes effect before running the test; -// the test will still run in the original directory +// The final value passed to Chdir() during setup +// will be the directory the test is run from. func (self *Shell) Chdir(path string) *Shell { self.dir = filepath.Join(self.dir, path) diff --git a/pkg/integration/components/test.go b/pkg/integration/components/test.go index 203436ae755..6eab23ae04a 100644 --- a/pkg/integration/components/test.go +++ b/pkg/integration/components/test.go @@ -182,7 +182,13 @@ func (self *IntegrationTest) Run(gui integrationTypes.GuiDriver) { panic(err) } - shell := NewShell(pwd, func(errorMsg string) { gui.Fail(errorMsg) }) + shell := NewShell( + pwd, + // passing the full environment because it's already been filtered down + // in the parent process. + os.Environ(), + func(errorMsg string) { gui.Fail(errorMsg) }, + ) keys := gui.Keys() testDriver := NewTestDriver(gui, shell, keys, InputDelay()) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index f0437b30da6..1acc97fb4b2 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -274,13 +274,16 @@ var tests = []*components.IntegrationTest{ worktree.AssociateBranchBisect, worktree.AssociateBranchRebase, worktree.BareRepo, + worktree.BareRepoWorktreeConfig, worktree.Crud, worktree.CustomCommand, worktree.DetachWorktreeFromBranch, worktree.DotfileBareRepo, + worktree.DoubleNestedLinkedSubmodule, worktree.FastForwardWorktreeBranch, worktree.ForceRemoveWorktree, worktree.RemoveWorktreeFromBranch, worktree.ResetWindowTabs, + worktree.SymlinkIntoRepoSubdir, worktree.WorktreeInRepo, } diff --git a/pkg/integration/tests/worktree/bare_repo_worktree_config.go b/pkg/integration/tests/worktree/bare_repo_worktree_config.go new file mode 100644 index 00000000000..ae468145521 --- /dev/null +++ b/pkg/integration/tests/worktree/bare_repo_worktree_config.go @@ -0,0 +1,92 @@ +package worktree + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// This case is identical to dotfile_bare_repo.go, except +// that it invokes lazygit with $GIT_DIR set but not +// $GIT_WORK_TREE. Instead, the repo uses the core.worktree +// config to identify the main worktre. + +var BareRepoWorktreeConfig = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Open lazygit in the worktree of a vcsh-style bare repo and add a file and commit", + ExtraCmdArgs: []string{"--git-dir={{.actualPath}}/.bare"}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.UserConfig.Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + // we're going to have a directory structure like this: + // project + // - .bare + // - . (a worktree at the same path as .bare) + // + // + // 'repo' is the repository/directory that all lazygit tests start in + + shell.CreateFileAndAdd("a/b/c/blah", "blah\n") + shell.Commit("initial commit") + + shell.CreateFileAndAdd(".gitignore", ".bare/\n/repo\n") + shell.Commit("add .gitignore") + + shell.Chdir("..") + + // configure this "fake bare"" repo using the vcsh convention + // of core.bare=false and core.worktree set to the actual + // worktree path (a homedir root). This allows $GIT_DIR + // alone to make this repo "self worktree identifying" + shell.RunCommand([]string{"git", "--git-dir=./.bare", "init", "--shared=false"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "config", "core.bare", "false"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "config", "core.worktree", ".."}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "remote", "add", "origin", "./repo"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "checkout", "-b", "main"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "config", "branch.main.remote", "origin"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "config", "branch.main.merge", "refs/heads/master"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "fetch", "origin", "master"}) + shell.RunCommand([]string{"git", "--git-dir=./.bare", "-c", "merge.ff=true", "merge", "origin/master"}) + + // we no longer need the original repo so remove it + shell.DeleteFile("repo") + + shell.UpdateFile("a/b/c/blah", "updated content\n") + shell.Chdir("a/b/c") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Lines( + Contains("main"), + ) + + t.Views().Commits(). + Lines( + Contains("add .gitignore"), + Contains("initial commit"), + ) + + t.Views().Files(). + IsFocused(). + Lines( + Contains(" M a/b/c/blah"), // shows as modified + ). + PressPrimaryAction(). + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Type("Add blah"). + Confirm() + + t.Views().Files(). + IsEmpty() + + t.Views().Commits(). + Lines( + Contains("Add blah"), + Contains("add .gitignore"), + Contains("initial commit"), + ) + }, +}) diff --git a/pkg/integration/tests/worktree/double_nested_linked_submodule.go b/pkg/integration/tests/worktree/double_nested_linked_submodule.go new file mode 100644 index 00000000000..c964251a966 --- /dev/null +++ b/pkg/integration/tests/worktree/double_nested_linked_submodule.go @@ -0,0 +1,93 @@ +package worktree + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// Even though this involves submodules, it's a worktree test since +// it's really exercising lazygit's ability to correctly do pathfinding +// in a complex use case. +var DoubleNestedLinkedSubmodule = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Open lazygit in a link to a repo's double nested submodules", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.UserConfig.Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + // we're going to have a directory structure like this: + // project + // - repo/outerSubmodule/innerSubmodule/a/b/c + // - link (symlink to repo/outerSubmodule/innerSubmodule/a/b/c) + // + shell.CreateFileAndAdd("rootFile", "rootStuff") + shell.Commit("initial repo commit") + + shell.Chdir("..") + shell.CreateDir("innerSubmodule") + shell.Chdir("innerSubmodule") + shell.Init() + shell.CreateFileAndAdd("a/b/c/blah", "blah\n") + shell.Commit("initial inner commit") + + shell.Chdir("..") + shell.CreateDir("outerSubmodule") + shell.Chdir("outerSubmodule") + shell.Init() + shell.CreateFileAndAdd("foo", "foo") + shell.Commit("initial outer commit") + // the git config (-c) parameter below is required + // to let git create a file-protocol/path submodule + shell.RunCommand([]string{"git", "-c", "protocol.file.allow=always", "submodule", "add", "../innerSubmodule"}) + shell.Commit("add dependency as innerSubmodule") + + shell.Chdir("../repo") + shell.RunCommand([]string{"git", "-c", "protocol.file.allow=always", "submodule", "add", "../outerSubmodule"}) + shell.Commit("add dependency as outerSubmodule") + shell.Chdir("outerSubmodule") + shell.RunCommand([]string{"git", "-c", "protocol.file.allow=always", "submodule", "update", "--init", "--recursive"}) + + shell.Chdir("innerSubmodule") + shell.UpdateFile("a/b/c/blah", "updated content\n") + + shell.Chdir("../../..") + shell.RunCommand([]string{"ln", "-s", "repo/outerSubmodule/innerSubmodule/a/b/c", "link"}) + + shell.Chdir("link") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Lines( + Contains("HEAD detached"), + Contains("master"), + ) + + t.Views().Commits(). + Lines( + Contains("initial inner commit"), + ) + + t.Views().Files(). + IsFocused(). + Lines( + Contains(" M a/b/c/blah"), // shows as modified + ). + PressPrimaryAction(). + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Type("Update blah"). + Confirm() + + t.Views().Files(). + IsEmpty() + + t.Views().Commits(). + Lines( + Contains("Update blah"), + Contains("initial inner commit"), + ) + }, +}) diff --git a/pkg/integration/tests/worktree/symlink_into_repo_subdir.go b/pkg/integration/tests/worktree/symlink_into_repo_subdir.go new file mode 100644 index 00000000000..a77a95d72b5 --- /dev/null +++ b/pkg/integration/tests/worktree/symlink_into_repo_subdir.go @@ -0,0 +1,63 @@ +package worktree + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var SymlinkIntoRepoSubdir = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Open lazygit in a symlink into a repo's subdirectory", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + config.UserConfig.Gui.ShowFileTree = false + }, + SetupRepo: func(shell *Shell) { + // we're going to have a directory structure like this: + // project + // - repo/a/b/c (main worktree with subdirs) + // - link (symlink to repo/a/b/c) + // + shell.CreateFileAndAdd("a/b/c/blah", "blah\n") + shell.Commit("initial commit") + shell.UpdateFile("a/b/c/blah", "updated content\n") + + shell.Chdir("..") + shell.RunCommand([]string{"ln", "-s", "repo/a/b/c", "link"}) + + shell.Chdir("link") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Branches(). + Lines( + Contains("master"), + ) + + t.Views().Commits(). + Lines( + Contains("initial commit"), + ) + + t.Views().Files(). + IsFocused(). + Lines( + Contains(" M a/b/c/blah"), // shows as modified + ). + PressPrimaryAction(). + Press(keys.Files.CommitChanges) + + t.ExpectPopup().CommitMessagePanel(). + Title(Equals("Commit summary")). + Type("Add blah"). + Confirm() + + t.Views().Files(). + IsEmpty() + + t.Views().Commits(). + Lines( + Contains("Add blah"), + Contains("initial commit"), + ) + }, +}) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 88ce3cfcec5..202c3f23a68 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -271,7 +271,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p if err := cmd.Wait(); err != nil { // it's fine if we've killed this program ourselves if !strings.Contains(err.Error(), "signal: killed") { - self.Log.Errorf("Unexpected error when running cmd task: %v", err) + self.Log.Errorf("Unexpected error when running cmd task: %v; Failed command: %v %v", err, cmd.Path, cmd.Args) } } diff --git a/test/.gitconfig b/test/.gitconfig new file mode 120000 index 00000000000..b2405f3cd79 --- /dev/null +++ b/test/.gitconfig @@ -0,0 +1 @@ +global_git_config \ No newline at end of file