Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChrisMcD1
commented
Feb 26, 2025
| }, | ||
| self.UserConfig().Git.AllBranchesLogCmds..., | ||
| ))) | ||
| candidates := lo.Uniq(lo.WithoutEmpty(self.UserConfig().Git.AllBranchesLogCmds)) |
There was a problem hiding this comment.
We could potentially move this logic into the migration, but I didn't feel like doing so. It would mean we would technically want to edit the AllBranchesLogCmds, even if there was no AllBranchesLogCmd, which just adds another code path for little gain.
ChrisMcD1
force-pushed
the
3961-all-branches-log
branch
from
d59eb2f to
da22805
Compare
ChrisMcD1
force-pushed
the
refactor-migrations
branch
from
264b998 to
532de87
Compare
ChrisMcD1
force-pushed
the
3961-all-branches-log
branch
2 times, most recently
from
53440ae to
7f3e0ee
Compare
7 tasks
ChrisMcD1
force-pushed
the
refactor-migrations
branch
2 times, most recently
from
f2bb202 to
d39f883
Compare
- **PR Description** In the solving of jesseduffield#4194 we had some discussion about the repeated marshalling and unmarshalling of the content in our migrations. jesseduffield#4194 (comment). I was feeling the itch to see if there was a meaningful performance impact to that, and this is the results! I added a benchmark first that uses the entirety of `Config.md`. This ensures a worst-case scenario for the repeated parsing, and probably at least 1 user has done this for their config. Here were the benchmark results prior and after this PR: ## Before ``` $ go test ./pkg/config/ -bench=. -benchmem -count=10 goos: linux goarch: amd64 pkg: github.com/jesseduffield/lazygit/pkg/config cpu: Intel(R) Core(TM) i7-8705G CPU @ 3.10GHz BenchmarkMigrationOnLargeConfiguration-8 85 18165916 ns/op 1501442 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 50 21024442 ns/op 1501473 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 75 18814769 ns/op 1501509 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 60 17886812 ns/op 1501434 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 97 17767498 ns/op 1501448 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 62 18749923 ns/op 1501478 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 62 19248265 ns/op 1501467 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 92 14771199 ns/op 1501423 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 67 16345152 ns/op 1501440 B/op 19316 allocs/op BenchmarkMigrationOnLargeConfiguration-8 98 16785737 ns/op 1501472 B/op 19316 allocs/op PASS ok github.com/jesseduffield/lazygit/pkg/config 14.474s ``` so somewhere in the range of 14 to 21 ms added to startup time. ## After ``` $ go test ./pkg/config/ -bench=. -benchmem -count=10 goos: linux goarch: amd64 pkg: github.com/jesseduffield/lazygit/pkg/config cpu: Intel(R) Core(TM) i7-8705G CPU @ 3.10GHz BenchmarkMigrationOnLargeConfiguration-8 440 2936145 ns/op 265800 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 428 3099183 ns/op 265787 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 355 3074069 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 478 3031144 ns/op 265792 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 375 2795470 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 406 2688491 ns/op 265791 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 375 3092990 ns/op 265791 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 379 2789022 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 417 3108530 ns/op 265785 B/op 3928 allocs/op BenchmarkMigrationOnLargeConfiguration-8 487 2848502 ns/op 265788 B/op 3928 allocs/op PASS ok github.com/jesseduffield/lazygit/pkg/config 15.352s ``` So somewhere between 2.6 and 3.1 ms. Is that a meaningful speedup? I'm not sure, but it's something! I had to add the bit of complexity mentioned in the comment to track whether we had made any changes, but that doesn't feel fundamentally much more complex to me, since we were already implicitly tracking that fact with whether the string file changed. - **Please check if the PR fulfills these requirements** * [X] Cheatsheets are up-to-date (run `go generate ./...`) * [X] Code has been formatted (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting)) * [ ] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [ ] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [ ] If a new UserConfig entry was added, make sure it can be hot-reloaded (see [here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig)) * [ ] Docs have been updated if necessary * [X] You've read through your own file changes for silly mistakes etc
This was backwards; we renamed Sha to Hash, so Sha is now deprecated, not Hash.
It has been 'e' instead of 'o' for quite a while now.
It is false by default. This way there's one less change to make in the next commit.
This allows us to tell whether they appear in the user's config file, which we will need later in this branch.
We'll reuse it in the next commit.
- **PR Description** I want to be able to configure custom commands that I don't need very often; I don't want these to pollute the global keybindings menu, and I don't want to assign globally unique keybindings to them (because there are only so many of these available, and also because I wouldn't be able to remember them, because the commands are not used often). However, I still want to invoke them through keybindings somehow. I find that the perfect solution for this is to configure a menu that contains custom commands. I can pop open the menu using only one key that I need to remember, but I can access the individual custom commands inside using keys that don't need to be unique with the rest of the global keybindings. In this PR we achieve this by adding an optional `subCommands` property to customCommand that can be used instead of the other properties like `command`, etc. This is an alternative approach to jesseduffield#4276, which added a new top-level property for custom command menus. Potentially closes jesseduffield#3799. - **Please check if the PR fulfills these requirements** * [x] Cheatsheets are up-to-date (run `go generate ./...`) * [x] Code has been formatted (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting)) * [x] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [x] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [x] If a new UserConfig entry was added, make sure it can be hot-reloaded (see [here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig)) * [x] Docs have been updated if necessary * [x] You've read through your own file changes for silly mistakes etc
ChrisMcD1
force-pushed
the
3961-all-branches-log
branch
from
7f3e0ee to
1ce80ef
Compare
|
I have learned that you can't move stacked PRs across repos, so this is closed in favor of the real one jesseduffield#4345 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A stacked PR on top of Refactor migrations to only marshall yaml twice jesseduffield/lazygit#4318. Will be changed into a cross-repository PR once that once merges. (The N+1 cost of adding a migration is lower now that I just have to modify a yaml.Node, and that's it!)
Solves the linked issue by migrating
allBranchesLogCmdinto the preferredallBranchesLogCmds. This would be breaking behavior if somebody was relying on the behavior in jesseduffield#3961, where they did not specify aallBranchesLogCmdelement, and added some additionalallBranchesLogCmdselements. Such a behavior was never documented, and relying on a deprecated element seems like user error. I would assume most people are like the one in the issue, where they assumed that they should put all of the items they want to cycle through in theallBranchesLogCmds, if they defined it.Fixes jesseduffield#3961
go generate ./...)