Change RepoPaths to be acquired via RepoPathCache

[jwhitley]

PR Description

In order to optimize the number of git calls made this change implements a RepoPathCache as the API by which RepoPaths instances are acquired. This primarily affects app startup and worktree enumeration.

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

Fixes #3227.

Please check if the PR fulfills these requirements

• 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]

This PR is WIP since there's an annoying panic in worktree_loader_test.go, almost certainly related to how I'm setting up GitCommon's RepoPathCache instance for testing. (EDIT: now fixed, ready for review.) Other unit tests pass, as do all integration tests. I didn't implement a separate unit or integration test related to repo_path_cache.go. Virtually every existing integration test will hit the cache API because it's so central. Likewise, a unit test didn't seem relevant, but I'm open to discussion on that point. (EDIT: Of note, worktree_loader_test.go already exercises this code. See updates to that test file in this PR.)

[jwhitley]

Bonus points: the second commit adds a new VSCode launch config that one-click debugs the unit test in the current tab.

[stefanhaller]

Bonus points: the second commit adds a new VSCode launch config that one-click debugs the unit test in the current tab.

I recommend to use VS Code's builtin test browser for that, it's pretty good. You don't need a launch config for this. It should discover go tests automatically (at least I don't remember having to configure anything).

Running the test under the cursor can be done with Command-; C, debugging the test under the cursor with Command-; Command-C.

[stefanhaller]

Hm, don't you have the go extension installed, by any chance? It shows code lenses for running and debugging tests, I find it hard to believe that you missed those.

[jwhitley]

Hm, don't you have the go extension installed, by any chance? It shows code lenses for running and debugging tests, I find it hard to believe that you missed those.

lol, I absolutely missed those. 🙄 In my defense, my web searching for debugging go unit tests in VSCode didn't point me at either the codelenses OR the hindsight-super-obvious "look in the command palette!", always towards launch config stuff. Also, debugging integration tests previously via the launch config pointed me in exactly the wrong direction... ah well.

Shall I just drop that commit, since it's redundant?

[stefanhaller]

Shall I just drop that commit, since it's redundant?

Yes, I would say so. The debug configurations menu is already too crowded (I'm also not sure I'm happy with the recent change of unhiding some configs that you normally don't use on their own).

[jwhitley]

Thanks Stefan. Redundant debug config dropped.

[jwhitley]

@jesseduffield Any feedback on this PR?

[jesseduffield]

@jwhitley sorry I've been busy lately so haven't had much time for reviewing. Looking at this code, it seems we're doing two things:

1. obtaining RepoPaths early in the startup process that we only need to call git rev-parse once during startup
2. caching all calls to GetRepoPaths globally based on the directory

These are two separate things and although I think the first part is great because it reduces startup time, I'm less sure about the second part. Caching the calls will speed things up, but at the cost of code complexity and the cache potentially becoming stale. How it could become stale, I'm not actually sure. Maybe the user tweaks some stuff with a worktree outside of lazygit and then expects the RepoPaths for that worktree to be reflected when they come back to lazygit and refresh? What do you think?

Also keen to get @stefanhaller 's thoughts

[jesseduffield]

exposing GetGitVersion on repoPathCache is a little too opportunistic for my liking: the fact that repo paths knows about the git version doesn't mean we should rely on it for obtaining the git version. I would instead pass that through to NewGitCommand explicitly as we did before.

[jwhitley]

Good catch. To be honest, I found a bit of "architectural pull" towards something which is really a "Git service", but explicitly tried to avoid in this PR(**). Looks like some of that slipped through here.

**: Use cases that motivate that are things like this comment from Stefan, specifically needing to cd into the repo consistently as part of a git operation. One could envision a first class GitRepo entity (basically what we know as RepoPaths), but with an upgraded API to consolidate repo handling logic. The code logic shifts to asking "perform this git operation in this Repo". The cache is essentially an implementation detail of the repo accessor API at that point. I'm specifically not saying we should go there at this time, just that it's something to chew on if/when more use cases accumulate that support such a direction.

[stefanhaller]

That comment was specific to those few submodule operations though, and those can be dealt with locally in pkg/commands/git_commands/submodule.go (I'll have some code to look at shortly). I don't see where else we have the need to perform a git operation in a repo other than the currently open one.

[stefanhaller]

Also keen to get @stefanhaller 's thoughts

I don't have much of an opinion here, as I haven't been following the prior work in this area very closely, and I'm also very unfamiliar with worktrees as I don't use them.

[stefanhaller]

@jwhitley sorry I've been busy lately so haven't had much time for reviewing. Looking at this code, it seems we're doing two things:

1. obtaining RepoPaths early in the startup process that we only need to call git rev-parse once during startup
2. caching all calls to GetRepoPaths globally based on the directory

These are two separate things and although I think the first part is great because it reduces startup time, I'm less sure about the second part.

Discussed in person: Jesse and I agree that 1. is useful, but 2. is too risky and maybe not useful enough. @jwhitley, would you be ok with dropping the cache again?

[jwhitley]

I'd be fine with that. I'd like to propose another option for consideration, first. go-memoize allows automatic time-based cache expiry. I didn't use that as my starting point, but a more conservative approach might be:

• Use automatic cache expiry with a time (if we could agree that such exists) that's "long enough to be useful, short enough to be safe".
• Optionally, allow that time to be configured by the user.

I've also thought about using explicit cache invalidation, but haven't been clear where or when that would be useful. E.g. as a user exposed "reload" command is the best I've come up with, given that the cases that invalidate this data are pretty much wholly external to lazygit.

Thoughts?

[stefanhaller]

I really think none of this is worth it. It's great that we reduce the startup time by combining several git rev-parse calls to one; but that's all we need to do. The term "cache invalidation" alone makes my alarm bells go off; the only benefit of this cache would be that we reduce the time to switch back and forth between two repos by 10ms, which just isn't worth it.

[jwhitley]

Fair points. I'll change this to just eliminate the extra call per previous discussion.