Use git rev-parse to obtain repository and worktree paths

[jwhitley]

• PR Description
  This changes GetRepoPaths() to pull information from git rev-parse instead of partially 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. I believe it also paves the way for lazygit to run from any subdirectory of a working tree with relatively minor changes.

Addresses #1294 and #3175.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[jwhitley]

A note on tests: some of tests in repo_path_tests.go didn't "survive" these changes as they no longer made sense. For example, "worktree with trailing separator in path" is transparently handled by git itself (I checked) so there's no reason to retest that git implements its own behavior.

I believe that the revised tests support the changed code reasonably well, but would appreciate feedback here due to the overall magnitude of the shift to the implementation.

[jwhitley]

One more thing. To support this work I created a shell script gist: repotest.zsh that uses git to replicate therepo_paths_test.go scenarios in actual git repos, and to vet out populating the RepoPaths struct via git rev-parse. Perhaps it would be helpful to inform eventual integation tests?

[stefanhaller]

Thanks, this looks awesome at first glance. I don't have time for an in-depth review today, but I'm also not sure I'm the best person to review this anyway. I'm not familiar with worktrees at all, Jesse wrote all that code, and I think it would be best to wait for him to be back.

[jwhitley]

Thanks. In the meantime it looks like code coverage CI failed because it didn’t run. Do you have any insight into that? If there are any coverage issues, it would be nice to sort those out ASAP.

[stefanhaller]

In the meantime it looks like code coverage CI failed because it didn’t run. Do you have any insight into that?

That's a know problem, see #3039 (comment) and #3039 (comment).

[jwhitley]

I ran the coverage manually, and there are two things that show up:

1. The current tests only hit the error case for the first git rev-parse call, but not the subsequent ones. This was a "known" choice, mostly as the realistic scenario is that git bails on making sense of the repo on the first call, but I'm open to revising it. For 100% coverage, there would need to be cases that failed each successive git call.
2. linkedWorktreePaths() only has partial coverage now, through the first error return, but missing the main loop. I suspect it was getting coverage in passing before, but now it's only called via worktree_loader.go. So direct test(s) may be in order.

[stefanhaller]

Don't sweat it too much, 100% test coverage is not a goal here. The decision whether a branch in your code deserves to be covered by a test is a judgement call, we don't want tools to dictate this.

We added the coverage check to CI mainly as an experiment to see if it's useful for catching the case that someone adds a whole new feature without tests. Personally I'm not convinced of that at all, this will always be caught by human reviews too. But the experiment is still ongoing.

[jwhitley]

Related to finding missed edge cases related to this PR, #1865 jumped out at me. The tl;dr of its scenario is: create a repo that has two-level nested submodules, then deep link into the "inner" submodule. cd into the link, and lazygit will show the wrong repo root, skipping the "outer" submodule. I updated the repotest.zsh script (see gist link above) to recreate this case. git, as expected, shows all of the correct values. i.e. this PR will correctly populate RepoPaths. So some other code is the problem.

Repro steps:

1. Grab the updated repotest.zsh code (2nd commit on that gist, at this writing).
2. run it via env DEBUG=1 ./repotest.zsh doubleLinkedSubmoduleCase. That will build the test repo under /tmp/lazygitRepoPath.XXXX ('XXXX' will be random; the script prints the actual path for you). The nested structure is repo/outerSubmodule/innerSubmodule, with a link in repo cLink@ -> outerSubmodule/innerSubmodule/a/b/c
3. cd into /tmp/lazygitRepoPath.XXXX/repo/cLink.
4. Run lazygit (built from the first commit on this PR).

lazygit will incorrectly show repo for the current repository, since it walked up /path/to/repo/cLink to /path/to/repo. It should show that it's in innerSubmodule.

The root cause (in git.go) is that NewGitCommand()'s calls findWorktreeRoot(), which walks up the directory tree looking for the worktree root without first calling filepath.EvalSymlinks(currentPath). A local patch that makes that call completely fixes #1865. i.e. In the current code symlinks can confuse the .git dir search, since they can skip nested submodule levels. That's the first-order bug. But I think there are deeper issues worth considering:

1. If an ostensible goal of this PR is "delegate pathfinding policy to git", then NewGitCommand() should theoretically just call out to git rev-parse --show-toplevel. It's definitely another reimplementaton of that logic, which is one view on why there's a bug. But calling git from a command intended to call git would be a weird thing to do. And there's a lot of code in the world that walks up to the root dir looking for some config file/dir. Pragmatically, I think this is probably OK.
2. NewGitCommand() calls os.Chdir(currentPath). IMO, it's worth thinking about whether a "new" call should mutate app state like this, especially when git itself doesn't care because it always does this kind of path resolution. Some lazygit code may care, e.g. diffing or whatever, but that could theoretically just cd to the current repoPaths.worktreePath. (again pragmatically, the answer here might be "there's been too much pain playing current-directory-whack-a-mole, this code just normalizes the current directory 100% of the time".) I acknowledge that I don't have enough app-wide context here, but thought the question worth considering.

I'm going to submit the EvalSymlink patch as an additional commit to this PR shortly, since I've talked myself into thinking it's almost certainly the right thing to do at this moment.

[jwhitley]

Re: the prior comment, see also discussion on #3017.

I just submitted a commit that adds an EvalSymlinks call. Since afero MemMapFS doesn't support symlinks, I just skip symlink evaluation if MemMapFS is in use. The alternative would be to do what repo_paths.go used to do: use a symlink eval function which is replaced with a stub in test. If we want to go that way, I propose adding that into Common alongside afero Fs, so the two can be changed simultaneously as needed. Probably just add a new interface with associated implementations, ala:

type Symlinks interface {
  EvalSymlinks(path string) (string , error)
}

[jwhitley]

YES! I finally nailed what appears to be the last known issue with lazygit running from an arbitrary directory of the working tree: diff-ish git commands were being run from CWD, not from the root, but were being passed root-relative paths. Incoming commit will fix this by adding the Dir(self.repoPaths.worktreePath) directive to the relevant command builder invocations. Preliminary testing shows this working very nicely, including in "exotic" bare-repo versioned homedir cases. I need to fully inspect failing tests, mostly fixing up ones which aren't expecting the new -C args. Commit incoming soon...

I'll appreciate a little extra scrutiny on this specific change when it lands, since I'm not 100% sure I've hit every necessary invocation, nor am I positive that I know how to manually inspect each change via the UI just yet.

[jwhitley]

OK, I have nothing more incoming on this branch. I believe it fixes every known issue I've found on my own and by scraping the issues (not completely!!), involving repo variants (bare, "fake" bare, submodules, weird nested things, etc) that git itself handles but were lazygit edge cases. Further, lazygit is now also running cleanly from arbitrary subdirectories, including symlink behavior, and the above repo variants.

If anyone finds any misses on those goals, throw 'em my way and I'll crush 'em! 💪🏼

If it's helpful, I'd consider a follow-on PR that adds repo structure variants from the current version of repotest.zsh to the integration tests. High-level pointers there would be helpful (e.g. "look at these tests first for examples").

[jesseduffield]

This is awesome work. Leaving git to do the work is a much better option than doing it manually ourselves and this should make things much more reliable in the future.

I've left some comments around implementation. As for testing, it would be good to add an integration test for each issue that this PR intends to fix. Let me know if you need any guidance on that. An example integration test that deals with similar functionality is pkg/integration/tests/worktree/dotfile_bare_repo.go. You can also pass ExtraEnvVars to NewIntegrationTestArgs if your test depends on specific environment variables.

[jwhitley]

ARGH. This repo doesn't have a .editorconfig file. In principle gofumpt/gofmt should be picking up the slack here, but for some reason my local config isn't doing that correctly now. Either way, it's nice to have the default editing (indenting!) experience mostly match the formatting results. I'll add a minimal one (only for [*.go] files) to this branch; can remove it if not desired for some reason.

[jesseduffield]

I know you haven't requested a re-review yet but I thought I'd add some more comments on the back of your recent changes

[jwhitley]

@jesseduffield I've added the first integration test, aimed at capturing my real-world case that prompted #3175. The problem is that it doesn't fail on master or v0.40.2 (the latest release I have handy, via Homebrew).

EDIT: I did confirm that the test has UseCustomPath: true so that --work-tree won't be implicitly set.

I re-confirmed that v0.40.2 crashes on startup via the repro steps:

1. Download the current version of repotest.zsh
2. Invoke it as: env DEBUG=1 ./repotest.zsh fakeBareRepoCase
   a. The last line of output shows the temp directory used for this case.
3. cd to the temp directory in step 2a.
4. run export GIT_DIR=`realpath ./bareRepo.git`
5. lazygit

Observe a crash ala:

>> /opt/homebrew/bin/lazygit                                                                                                                                                                                                                18.17.1
2024/01/11 14:33:18 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues

*errors.errorString Error getting repo paths: failed to get worktree git dir path: stat /tmp/lazygitRepoPath.J8eOWO/a/b/c/.git: no such file or directory
github.com/jesseduffield/lazygit/pkg/commands/git.go:100 (0x100eef2b8)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:271 (0x1010152ac)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:656 (0x101017c00)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:675 (0x101018200)
github.com/jesseduffield/lazygit/pkg/utils/utils.go:108 (0x100d92960)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:674 (0x10101816c)
github.com/jesseduffield/lazygit/pkg/app/app.go:263 (0x1010404bc)
github.com/jesseduffield/lazygit/pkg/app/app.go:48 (0x101040481)
github.com/jesseduffield/lazygit/pkg/app/entry_point.go:150 (0x10104208c)
github.com/jesseduffield/lazygit/main.go:23 (0x101043858)
runtime/internal/atomic/types.go:194 (0x100a1eafc)
runtime/asm_arm64.s:1197 (0x100a50434)

AFAICT, the test in the latest commit should correctly replicate the fakeBareRepoCase setup. I suspect that something about the test environment may be preventing the crash. Do you have any insight on that?

[jwhitley]

For clarity, here's my plan for new integration tests.

Issues fixed:

#1294 - dotfile bare repo "omnibus" issue
#1865 - two-level nested submodules
#3015 - fixed since git itself evaluates symlinks in git rev-parse (here for --git-dir)
#3144 - lg errors when opened from symlink'ed path (outside of repo) to dir inside of repo
#3175 - Similar dotfile case, $GIT_DIR is set (via --git-dir arg) but --work-tree/$GIT_WORK_TREE is unset

New lazygit integration tests

(EDIT: GitHub's long-form autolinking inside (check)lists was making this unreadable; switched to manual links. –J)

• #3175 - Similar to bare_repo.go, but --git-dir is set, --work-tree is unset, but core.worktree on the bare repo is set.
  • #1294 isn't really an issue with lazygit (see my comment), but various commenters have bare-repo issues covered by #3175
• #3144 - Setup repo with subdir, symlink into that subdir, run lg from it
  • #3015 is a dupe of #3144
• #1865 - integration test matching repotest.zsh's doubleLinkedSubmoduleCase (discussed in detail above)

[jwhitley]

Learning about the integration test setup: The crash below is from 0.40.2 run from the repo constructed by the test, with GIT_DIR set as per the test. The branch build works as expected. Stronger confirmation of some misalignment of the test environment vs. the built application. I've been digging through {test,runner,shell}.go, but no obvious causes have popped up.

>> /opt/homebrew/bin/lazygit
2024/01/11 16:37:28 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues

*errors.errorString Error getting repo paths: failed to get worktree git dir path: stat /Users/whitley/src/3p/lazygit/1/test/_results/worktree/bare_repo_worktree_config/actual/a/b/c/.git: no such file or directory
github.com/jesseduffield/lazygit/pkg/commands/git.go:100 (0x10519f2b8)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:271 (0x1052c52ac)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:656 (0x1052c7c00)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:675 (0x1052c8200)
github.com/jesseduffield/lazygit/pkg/utils/utils.go:108 (0x105042960)
github.com/jesseduffield/lazygit/pkg/gui/gui.go:674 (0x1052c816c)
github.com/jesseduffield/lazygit/pkg/app/app.go:263 (0x1052f04bc)
github.com/jesseduffield/lazygit/pkg/app/app.go:48 (0x1052f0481)
github.com/jesseduffield/lazygit/pkg/app/entry_point.go:150 (0x1052f208c)
github.com/jesseduffield/lazygit/main.go:23 (0x1052f3858)
runtime/internal/atomic/types.go:194 (0x104cceafc)
runtime/asm_arm64.s:1197 (0x104d00434)

[jesseduffield]

I spent some time looking into this and it turns out that the difference between the manual and the automated test is that the current working directory is not set by the automated test, meaning it defaults to the lazygit root directory.

Regrettably, we actually depend on this working tree being set so that it can look up the test to run internally. Specifically, in GetTests() in pkg/integration/tests/tests.go we call utils.GetLazyRootDirectory() to find the lazygit root in order to find the tests. And that function (defined in vendor/github.com/jesseduffield/lazycore/pkg/utils/utils.go) just looks for a .git directory (as opposed to looking for a folder called 'lazygit' for example)!

This is why we're passing '--path' instead of setting the working directory.

I've got a PR up to fix this: #3215

If you add that to your branch, your test should fail properly on master (tested this myself locally). You'll need to remove the UseCustomPath part from the test as that's no longer a thing.

[jwhitley]

Awesome, thanks @jesseduffield. I rebased my local branch and confirmed that I get the expected failure on master with this test.

Next wrinkle: this test starts lazygit in a subdir of the repo (a/b/c) then tries to add (a/b/c/blah) to pick up an update. This operation fails because lazygit isn't accounting for CWD being below the repo-root when it invokes git. (Clearly my manual testing was trash .. I would have sworn I'd exercised this manually. :-pppp mumbles in "why test plans exist") A generalized sequence of events:

1. cd /path/to/repo/a/b/c
2. modify a/b/c/blah
3. start lazygit (from this branch)
4. Tap space to add blah

A warning dialog like the one below appears, and the command window shows that we're trying to git add -- a/b/c (while CWD is already a/b/c). Note that the 'a' (add all) command works fine because it's not passed any file paths.

I'll chew on this one today, but it means that there's a whole class of issues I clearly missed w.r.t. subdirectory correctness. lazygit is always passing single files as root-relative paths. That means, one way or another, git needs to know where the repo root is:

1. either because paths are always relative to CWD (how a human on the CLI often works)
2. because CWD or -C options tell git where the root is.

It feels like a big change to plumb -C to every relevant git invocation. An alternative is for lazygit to explicitly manage CWD state by always cd'ing to RepoPaths.worktreePath for the current worktree. Any other required cd operations must then save/restore that state. That sets environmental context for all git commands, which should be a smaller code change but still a "big" operational change.

As I said, I'll think on this one today. If you have feelings about the best path forward here, (including "put it off to a follow-on PR"), I'd love to hear.

[jesseduffield]

It feels like a big change to plumb -C to every relevant git invocation. An alternative is for lazygit to explicitly manage CWD state by always cd'ing to RepoPaths.worktreePath for the current worktree. Any other required cd operations must then save/restore that state. That sets environmental context for all git commands, which should be a smaller code change but still a "big" operational change.

As I said, I'll think on this one today. If you have feelings about the best path forward here, (including "put it off to a follow-on PR"), I'd love to hear.

Hmm. So in pkg/app/entry_point.go we currently set the CWD if --path or --work-tree is passed, but in this case only --git-dir is being passed so we leave the CWD to be whatever lazygit is opened in. So I think we should just also change the CWD to the worktree root if --git-dir is passed. I don't expect that to be a big change at all, given that we're already doing it with the other cases. The tricky part is picking the right place to do this: I suspect pkg/app/entry_point.go is the wrong place given that we don't obtain the repo paths until later. How about in the first leg of this if statement in NewGitCommand?

    // pkg/commands/git.go
	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)
		}
	}

	repoPaths, err := git_commands.GetRepoPaths(cmn.Fs, currentPath)
	if err != nil {
		return nil, errors.Errorf("Error getting repo paths: %v", err)
	}

[jwhitley]

That's hugely helpful. It turns out there's an even simpler approach with the new GetRepoPaths(). That whole chunk of code in NewGitCommand() reduces to the snippet below. The principle is the same behind the GetRepoPaths() refactor: git handles all of this logic for us already, so just let it. With one additional (completely orthogonal) change below, all unit and integration tests (and the new test) pass:

	// 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)

	repoPaths, err := git_commands.GetRepoPaths(osCommand.Cmd, currentPath)
	if err != nil {
		return nil, errors.Errorf("Error getting repo paths: %v", err)
	}
	currentPath = repoPaths.WorktreePath()

	err = os.Chdir(currentPath)
	if err != nil {
		return nil, utils.WrapError(err)
	}

---

Separately, I noted that pkg/integration/tests/commit/commit_switch_to_editor.go was failing for me. It turns out that $GIT_EDITOR overrides core.editor, causing this test to invoke my neovim setup rather than the stubbed-in test "editor". I've added a commit to fix that by outright unsetting GIT_EDITOR in cmd/integration_test/main.go.

[jesseduffield]

Great work, that's much simpler.

As for GIT_EDITOR, I think we should be fixing this issue at the source, which is that in getLazygitCommand in pkg/integration/components/runner.go we instantiate the command object like cmdObj := osCommand.Cmd.New(cmdArgs) and New does cmd.Env = os.Environ(). So lazygit is inheriting all the env vars from the environment. I think that in pkg/commands/oscommands/cmd_obj_builder.go we should have:

func (self *CmdObjBuilder) New(args []string) ICmdObj {
	cmdObj := self.NewWithoutEnviron(args)
	cmdObj.GetCmd().Env = os.Environ()
	return cmdObj
}

func (self *CmdObjBuilder) NewWithoutEnviron(args []string) ICmdObj {
	cmd := exec.Command(args[0], args[1:]...)

	return &CmdObj{
		cmd:    cmd,
		runner: self.runner,
	}
}

And then in pkg/integration/components/runner.go we should go:

cmdObj := osCommand.Cmd.NewWithoutEnviron(cmdArgs)

That way, we'll have full control over what environment variables are set in the test lazygit process, and there won't be any difference between dev machines or CI.

[jesseduffield]

Whoops my bad, I missed that part of the code. Not able to look closely right now but I assume this is the crux of it:

func (self *WorktreeHelper) GetLinkedWorktreeName() string {
	worktrees := self.c.Model().Worktrees
	if len(worktrees) == 0 {
		return ""
	}

	// worktrees always have the current worktree on top
	currentWorktree := worktrees[0]
	if currentWorktree.IsMain {
		return ""
	}

	return currentWorktree.Name
}

[jwhitley]

Thanks Jesse. I skimmed around through the code with that as a starting point, but nothing obvious popped up. I'll have a go at debugging into it in the morning.

[jwhitley]

OK, I just goofed the recent change to apply filepath.Abs in GetRepoPaths – used it on the wrong field. <facepalm.gif> That's fixed in the latest change, and integration tests now pass locally on git 2.20.0 and 2.43.0. I didn't add a unit test for this specific case. It would require passing a mock function for filepath.Abs during test since it turns out that filepath.Abs will always hit the real filesystem if given a relative path (but not for an already-absolute path). Let me know what you all think about unit coverage for this.

[stefanhaller]

All tests are green! Nice. There's one linter error though, please have a look.

I don't have an opinion about the unit test question.

[jwhitley]

There's one linter error though, please have a look.

Good grief, missed some newly-dead code when removing currentPath from RepoPaths. Fixed in latest commit.

[jwhitley]

Allright, I also fixed up some default-disabled analyses in gopls that were default re-enabled by my Neovim gopls/LSP plugin. That was producing a lot of noise, so I wasn't always paying as much attention to diagnostics (e.g. that lint error) as I should.

[jesseduffield]

One last thing before merging: any chance you're a windows user @jwhitley ? Cos we wanna ensure this works on windows (regrettably we have yet to get integration tests working on windows in CI)

[jwhitley]

One last thing before merging: any chance you're a windows user @jwhitley ? Cos we wanna ensure this works on windows (regrettably we have yet to get integration tests working on windows in CI)

I'll see what I can do. I have a Windows 10 VM that should work for this, but it's never been setup for anything like dev work.

[jwhitley]

Hmm.. I think something about my setup is incomplete or borked. It's looking like the integration tests all fail with the same error:

go run .\cmd\integration_test\main.go cli .\pkg\integration\tests\bisect\skip.go
panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
github.com/jesseduffield/lazygit/pkg/integration/components.TestNameFromFilePath({0x40000c6500?, 0x42?})
        C:/Users/whitley/src/lazygit/pkg/integration/components/test.go:230 +0x68
github.com/jesseduffield/lazygit/pkg/integration/tests.GetTests.func2({0x40000c6500, 0x42}, {0x7ff6a22f3b28?, 0x4000171a40?}, {0x40000c17a8?, 0x7ff6a1f73dbc?})
        C:/Users/whitley/src/lazygit/pkg/integration/tests/tests.go:46 +0x208
path/filepath.walk({0x40000c6500, 0x42}, {0x7ff6a22f3b28, 0x4000171a40}, 0x40000c1ab0)
        C:/Program Files/Go/src/path/filepath/path.go:477 +0xc8
path/filepath.walk({0x40000c8840, 0x39}, {0x7ff6a22f3b28, 0x40001719d0}, 0x40000c1ab0)
        C:/Program Files/Go/src/path/filepath/path.go:501 +0x1d4
path/filepath.walk({0x40000c87c0, 0x32}, {0x7ff6a22f3b28, 0x4000171960}, 0x40000c1ab0)
        C:/Program Files/Go/src/path/filepath/path.go:501 +0x1d4
path/filepath.Walk({0x40000c87c0, 0x32}, 0x40000c1ab0)
        C:/Program Files/Go/src/path/filepath/path.go:572 +0x6c
github.com/jesseduffield/lazygit/pkg/integration/tests.GetTests({0x40000a2aa0, 0x1c})
        C:/Users/whitley/src/lazygit/pkg/integration/tests/tests.go:29 +0x174
github.com/jesseduffield/lazygit/pkg/integration/clients.getTestsToRun({0x40000a6060, 0x1, 0x2})
        C:/Users/whitley/src/lazygit/pkg/integration/clients/cli.go:57 +0x30
github.com/jesseduffield/lazygit/pkg/integration/clients.RunCLI({0x40000a6060, 0x1, 0x2}, 0x0, 0x0, 0x0, 0x0)
        C:/Users/whitley/src/lazygit/pkg/integration/clients/cli.go:34 +0x7c
main.main()
        C:/Users/whitley/src/lazygit/cmd/integration_test/main.go:71 +0x484
exit status 2

FWIW this is arm64 Windows 11 Pro running go 1.21.6 for win arm64. Is there a canonical dev setup on Windows I should be using? Right now, I have a minimal setup of just go and git for windows installed.

Unit tests also show some errors:

> go test ./... -short
?       github.com/jesseduffield/lazygit        [no test files]
?       github.com/jesseduffield/lazygit/cmd/integration_test   [no test files]
?       github.com/jesseduffield/lazygit/pkg/app        [no test files]
?       github.com/jesseduffield/lazygit/pkg/app/daemon [no test files]
?       github.com/jesseduffield/lazygit/pkg/app/types  [no test files]
?       github.com/jesseduffield/lazygit/pkg/commands   [no test files]
ok      github.com/jesseduffield/lazygit/pkg/cheatsheet 0.107s
?       github.com/jesseduffield/lazygit/pkg/commands/models    [no test files]
?       github.com/jesseduffield/lazygit/pkg/commands/types/enums       [no test files]
?       github.com/jesseduffield/lazygit/pkg/common     [no test files]
?       github.com/jesseduffield/lazygit/pkg/constants  [no test files]
?       github.com/jesseduffield/lazygit/pkg/fakes      [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui        [no test files]
?       github.com/jesseduffield/lazygit/pkg/env        [no test files]
ok      github.com/jesseduffield/lazygit/pkg/commands/git_commands      0.628s
ok      github.com/jesseduffield/lazygit/pkg/commands/git_config        0.099s
ok      github.com/jesseduffield/lazygit/pkg/commands/hosting_service   0.109s
--- FAIL: TestOSCommandRun (0.00s)
    os_test.go:21:
                Error Trace:    C:\Users\whitley\src\lazygit\pkg\commands\oscommands\os_test.go:21
                                                        C:\Users\whitley\src\lazygit\pkg\commands\oscommands\os_test.go:28
                Error:          Expect "exec: "rmdir": executable file not found in %PATH%" to match "rmdir.*unexisting-folder.*"
                Test:           TestOSCommandRun
FAIL
FAIL    github.com/jesseduffield/lazygit/pkg/commands/oscommands        0.113s
ok      github.com/jesseduffield/lazygit/pkg/commands/patch     0.102s
ok      github.com/jesseduffield/lazygit/pkg/config     0.076s
?       github.com/jesseduffield/lazygit/pkg/gui/context/traits [no test files]
ok      github.com/jesseduffield/lazygit/pkg/gui/context        0.103s
ok      github.com/jesseduffield/lazygit/pkg/gui/controllers    0.104s
ok      github.com/jesseduffield/lazygit/pkg/gui/controllers/helpers    0.110s
?       github.com/jesseduffield/lazygit/pkg/gui/keybindings    [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui/modes/cherrypicking    [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui/modes/filtering        [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui/modes/marked_base_commit       [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui/modes/diffing  [no test files]
ok      github.com/jesseduffield/lazygit/pkg/gui/filetree       0.100s
ok      github.com/jesseduffield/lazygit/pkg/gui/mergeconflicts 0.101s
?       github.com/jesseduffield/lazygit/pkg/gui/popup  [no test files]
ok      github.com/jesseduffield/lazygit/pkg/gui/patch_exploring        0.104s
--- FAIL: TestGetCommitListDisplayStrings (0.00s)
    --- FAIL: TestGetCommitListDisplayStrings/custom_time_format (0.00s)
        commits_test.go:427:
            sha1 2019-12-31 Jesse Duffield    commit1
            sha2 2019-12-20 Jesse Duffield    commit2
        commits_test.go:429:
                Error Trace:    C:\Users\whitley\src\lazygit\pkg\gui\presentation\commits_test.go:429
                Error:          Not equal:
                                expected: "sha1 2:03AM     Jesse Duffield    commit1\nsha2 2019-12-20 Jesse Duffield    commit2"
                                actual  : "sha1 2019-12-31 Jesse Duffield    commit1\nsha2 2019-12-20 Jesse Duffield    commit2"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,2 +1,2 @@
                                -sha1 2:03AM     Jesse Duffield    commit1
                                +sha1 2019-12-31 Jesse Duffield    commit1
                                 sha2 2019-12-20 Jesse Duffield    commit2
                Test:           TestGetCommitListDisplayStrings/custom_time_format
FAIL
FAIL    github.com/jesseduffield/lazygit/pkg/gui/presentation   0.113s
ok      github.com/jesseduffield/lazygit/pkg/gui/presentation/authors   0.093s
?       github.com/jesseduffield/lazygit/pkg/gui/presentation/icons     [no test files]
ok      github.com/jesseduffield/lazygit/pkg/gui/presentation/graph     0.098s
?       github.com/jesseduffield/lazygit/pkg/gui/status [no test files]
?       github.com/jesseduffield/lazygit/pkg/gui/types  [no test files]
ok      github.com/jesseduffield/lazygit/pkg/gui/services/custom_commands       0.153s
ok      github.com/jesseduffield/lazygit/pkg/gui/style  0.098s
?       github.com/jesseduffield/lazygit/pkg/integration/clients        [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/clients/injector       [no test files]
ok      github.com/jesseduffield/lazygit/pkg/i18n       0.076s
?       github.com/jesseduffield/lazygit/pkg/integration/tests  [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/cherry_pick      [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/commit   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/config   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/conflicts        [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/custom_commands  [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/demo     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/diff     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/file     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/filter_and_search        [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/filter_by_path   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/interactive_rebase       [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/misc     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/patch_building   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/reflog   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/shared   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/staging  [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/stash    [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/submodule        [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/sync     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/tag      [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/ui       [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/undo     [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/worktree [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/types  [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/bisect   [no test files]
?       github.com/jesseduffield/lazygit/pkg/integration/tests/branch   [no test files]
ok      github.com/jesseduffield/lazygit/pkg/integration/components     0.105s
?       github.com/jesseduffield/lazygit/pkg/jsonschema [no test files]
?       github.com/jesseduffield/lazygit/pkg/logs       [no test files]
?       github.com/jesseduffield/lazygit/pkg/logs/tail  [no test files]
ok      github.com/jesseduffield/lazygit/pkg/snake      0.066s
?       github.com/jesseduffield/lazygit/pkg/updates    [no test files]
ok      github.com/jesseduffield/lazygit/pkg/tasks      0.884s
ok      github.com/jesseduffield/lazygit/pkg/theme      0.106s
ok      github.com/jesseduffield/lazygit/pkg/utils      0.105s
ok      github.com/jesseduffield/lazygit/pkg/utils/yaml_utils   0.069s
FAIL

[jwhitley]

OH, it's dying in TestNameFromFilePath:

func TestNameFromFilePath(path string) string {
	name := strings.Split(path, "integration/tests/")[1]

	return name[:len(name)-len(".go")]
}

Blame says that code is pretty old, so this clearly must have worked. I've tried running from cmd shell and powershell, same failures. Do the integration tests need to run from WSL?

[stefanhaller]

As far as I know, integration tests have never worked on Windows. It would be great to fix this, but I wouldn't try to do that as part of this PR, which is big enough already.

I can't speak for Jesse, but I understand his request to test this on Windows more like testing it manually to see if everything seems to work. I just a tried a little bit, and starting up lazygit works fine, also in a subfolder of a repo, or in a subfolder of a submodule. I'm not sure what else to test, as I didn't pay much attention to what all the various special situations are that were fixed by this PR. 😅 (And I don't use workspaces.)

[stefanhaller]

One more thing: I'm a bit unhappy about the commit history (40 commits, yikes!). I find it basically unreadable, but there are also commits in there that don't even compile or fail tests, which is bad for bisecting (in future).

It's probably too late and way too much work to clean it up now, and I don't want to request that; however, I do wonder if it would be better to squash it down to a single commit. (If there's a change that can easily be broken out into a separate commit from there, all the better. I haven't checked.) Let me know what you think.

[jwhitley]

As far as I know, integration tests have never worked on Windows. It would be great to fix this, but I wouldn't try to do that as part of this PR, which is big enough already.

I can't speak for Jesse, but I understand his request to test this on Windows more like testing it manually to see if everything seems to work. I just a tried a little bit, and starting up lazygit works fine, also in a subfolder of a repo, or in a subfolder of a submodule. I'm not sure what else to test, as I didn't pay much attention to what all the various special situations are that were fixed by this PR. 😅 (And I don't use workspaces.)

Brilliant, thanks Stefan. I'll kick the tires myself today as well. Also, I'll squash the heck out of this mess. I suspect it'll just be One Big Commit, but if I can sensibly break it up into a set of test-clean commits I will do so.

[jwhitley]

OK, One Big Commit has been pushed. I winnowed down the commit message heavily, focusing on the notable functional changes, since there was a lot of implementation/fixup churn. For reference, I made a copy of the original branch at jwhitley/work-tree-from-git-orig.

[stefanhaller]

Nice. Maybe I'll even review that commit now. 😄 (Not before tomorrow though.)

Meanwhile I made a bit of progress towards getting the integration tests to run on Windows, see #3236. I didn't get far enough to have them run on CI, but I can run them locally at least. They succeed on master, but fail if I cherry-pick your commit on top. I don't have more time to look into this now, but I feel we should figure this out before we merge your PR. Feel free to play with this if you want.

[jwhitley]

What are next step(s) to getting this PR to land? On one hand, both Stefan and I have done some light ad-hoc testing. On the other the hopes for some quick progress on Windows tests in #3236 appear to have been dashed. That feels like another extended project, given the current state of things. A lot of what I call "time to first test" infrastructure work.

My thinking is: I'd like to get this PR merged, either now or after some additional to-be-defined due diligence, then focus on getting the Windows unit and integration tests running. At very least, pre-merge and pre-release due diligence would then be able to catch Windows test failures.

[jesseduffield]

I'm happy to merge this now, @stefanhaller ?

[stefanhaller]

Yep, me too. Thanks again for the great work John!

[jesseduffield]

Great work on this @jwhitley !

[jwhitley]

🎊 awesome, thank you both again for all of your help getting this one landed! 🎊

[jwhitley]

I don't believe I'm actually closing this browser tab. 😂

[jwhitley]

OH! Don't forget to close out all of the issues covered by this PR. This comment should be the canonical reference, at least of the stuff I found, confirmed fixed, and used as the baseline cases for integration test coverage. See my notes at that comment for details.