jesseduffield
Change RepoPaths to be acquired via RepoPathCache
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
- [x] Cheatsheets are up-to-date (run
go generate ./...) - [x] Code has been formatted (see here)
- [x] Tests have been added/updated (see here for the integration test guide)
- [x] Text is internationalised (see here)
- [x] Docs (specifically
docs/Config.md) have been updated if necessary - [x] You've read through your own file changes for silly mistakes etc
~~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.)
Bonus points: the second commit adds a new VSCode launch config that one-click debugs the unit test in the current tab.
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.
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.
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?
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).
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.
@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:
- obtaining RepoPaths early in the startup process that we only need to call git rev-parse once during startup
- 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?
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?
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.
Fair points. I'll change this to just eliminate the extra call per previous discussion.
@jesseduffield Whups, yes. Let me dig in right now and refresh myself as to what the proposed modification to this PR would look like.