Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

ChrisMcD1 · 2025-02-28T22:31:17Z

PR Description

Their issue where the default allBranchesLogCmd default remains present is because we just do a lo.Uniq(lo.WithoutEmpty()) on the combined list of allBranchesLogCmd and allBranchesLogCmds.

At the point of this code, it is not possible to tell whether the value present in allBranchesLogCmd is user-provided or not. We have already merged the config with the default config, so the user not setting anything, and the user explicitly setting "Yes, I want the default", are indistinguishable.

Based on that bug report, I'm assuming that users that have not set anything for allBranchesLogCmd, but have set something for allBranchesLogCmds, just want the list they have specified in the plural version. Some users have likely figured out they can explicitly set allBranchesLogCmd: "" to get this behavior, but most would not.

To achieve this desired behavior, I figure it is easiest to just migrate all user config to allBranchesLogCmds. If they have explicitly set a non-empty value in allBranchesLogCmd, it will be pulled over. If they set an empty string, it will be excluded.

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)
If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
Docs have been updated if necessary
You've read through your own file changes for silly mistakes etc

ChrisMcD1 · 2025-04-27T20:46:48Z

@stefanhaller Could you give this a look?

Also, is there somewhere I should include a notice of breaking changes on re-write? Last time we did this there were some nix people using immutable config files that ran into issues because they don't let LazyGit do write-back.

stefanhaller

Great work! Looks mostly good, just my usual bunch of minor nitpicks below.

And sorry for taking so long to review.

stefanhaller · 2025-04-29T08:44:25Z

pkg/config/app_config.go

+
+		cmdsKeyNode, cmdsValueNode := yaml_utils.LookupKey(gitNode, "allBranchesLogCmds")
+		if cmdsKeyNode == nil {
+			// Create dummy node and attach it onto the root git node


What's "dummy" about this? I find this comment more confusing than helpful, and I think we might just delete it.

Added an additional comment to clarify what I meant.
For me, it feels a bit odd to create the empty sequence, which would be nonsensical to do if we didn't then prepend the element onto it. So I'm just calling out that odd implementation.

If you still think the comment is confusing, or the implementation is trivial, I'm happy to remove.

(And we do technically support the weird case where the user has just removed the default allBranchesLogCmd by setting it equal to "", and then done nothing else. The clearing behavior of setting it to "" is why we check if the value is an empty string further down)

It was really just the "dummy" that I found confusing. The new comment is great. (Personally I still think it isn't needed; it's clear enough what the code is doing. But if you find it helpful, no objections.)

stefanhaller · 2025-04-29T08:48:58Z

pkg/config/app_config.go

+			if cmdsValueNode.Kind != yaml.SequenceNode {
+				return fmt.Errorf("You should have an allBranchesLogCmds defined as a sequence!")
+			}


I find it slightly confusing to do this here, where in half of the cases we have just created this ourselves and know it's the right type. I would move this up to an else block after line 370. (Yes, it then runs unnecessarily even when cmdValueNode is empty and there's nothing to migrate, but it doesn't hurt to throw the error in that case too; loading the non-migrated config file would have failed anyway.)

Sure, if that feels more coherent to you. My thought was just to keep it local to the moment that stuff would break should it not be defined as a sequence node.

Yes, I like this better, thanks for changing. It's really minor though.

I pushed one more fixup to replace the unneeded fmt.Errorf with errors.New (7067885); I missed this the first time around. (I thought we had a linter check for this? Seems not.)

stefanhaller · 2025-04-29T08:49:11Z

pkg/config/app_config.go

+				return fmt.Errorf("You should have an allBranchesLogCmds defined as a sequence!")
+			}
+			// Prepending the individual element to make it show up first in the list, which was prior behavior
+			cmdsValueNode.Content = append([]*yaml.Node{{Kind: yaml.ScalarNode, Value: cmdValueNode.Value}}, cmdsValueNode.Content...)


We have utils.Prepend for this.

Okay, I have used it!

Oh.... it actually triggers a package import loop :(

# github.com/jesseduffield/lazygit/pkg/utils package github.com/jesseduffield/lazygit imports github.com/jesseduffield/lazygit/pkg/app from main.go imports github.com/jesseduffield/lazygit/pkg/app/daemon from entry_point.go imports github.com/jesseduffield/lazygit/pkg/commands/models from rebase.go imports github.com/jesseduffield/lazygit/pkg/utils from commit.go imports github.com/jesseduffield/lazygit/pkg/common from dummies.go imports github.com/jesseduffield/lazygit/pkg/config from common.go imports github.com/jesseduffield/lazygit/pkg/utils from app_config.go: import cycle not allowed

Is that worth trying to tease apart, or does it not matter? It's a one-liner even inside the utils, so I don't feel the benefit is very high

Oh wow, that's annoying. Sure, for this PR it's totally ok to leave it as is, but in the future we should look into fixing this; it should be possible to depend on something as simple as utils/slice from anywhere else.

I opened #4538 to fix this.

Migrated to utils.Prepend after rebasing off master.

stefanhaller · 2025-04-29T08:52:22Z

pkg/config/app_config_test.go

+			if err != nil {
+				t.Error(err)
+			}


There's assert.NoError for this.

Used!

The godocs example had it being used in an if guard, so I did that too

Good. I have to say I'm torn about using the if guard; we don't do this anywhere else in our code. I get its purpose, but I wonder if it's the right tradeoff; it helps making the test output potentially less confusing in the case that the test does fail with an error, but how often is that going to happen? And it comes at the cost of making the test a little harder to read, which I find more important here, so I'd vote for removing the condition.

Okay, removed the condition, I have no particular affinity for it.

Maybe going around and making all the tests use the condition would be a good small task for me to do? I view it sort of like an:

if err == nil { assertMoreStuff }

so it doesn't feel really out of place to me (although I wish it would follow the more normal err != nil convention)

I still prefer not to do this. I like the simplicity of just listing all test expectations without conditions, this makes the tests very clear and easy to read. As I said, the downside is that you might get two failed expectations instead of just one if a test fails, but that's not a big deal, and it will happen very rarely anyway.

stefanhaller · 2025-04-29T08:54:38Z

pkg/utils/yaml_utils/yaml_utils.go

@@ -100,6 +100,19 @@ func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) {
 	return nil, nil
 }

+// Returns the key and value if they were present
+func RemoveKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) {


I think it would make sense to add a test for this, we have tests for most other things in yaml_utils.

@ChrisMcD1 I had a need for this function in a PR of mine (#4525), so I stole this function from your PR and added tests for it. (Would also appreciate a review of the rest of the branch if you feel like it.)

stefanhaller · 2025-04-29T09:01:16Z

Also, is there somewhere I should include a notice of breaking changes on re-write? Last time we did this there were some nix people using immutable config files that ran into issues because they don't let LazyGit do write-back.

It would be good if we could do one or both of the suggestions from #4210 (comment). Would you be up for giving that a try? I'll hold off merging this one before next Saturday's release to give us a bit more time for that.

ChrisMcD1 · 2025-05-05T00:54:06Z

@stefanhaller Comments addressed!

Got a load of garbage commits to just show you how I was thinking through your suggestions. Will squash them after review prior to merge.

Will remove RemoveKey helper once your PR that also uses it is merged and I rebase off of master.

stefanhaller · 2025-05-05T16:21:01Z

Got a load of garbage commits to just show you how I was thinking through your suggestions.

No worries, but just out of curiosity: why didn't you continue to use fixup commits for these? I like doing that and putting a bit of explanation into the body of the commit messages, seems like the best of both worlds to me.

ChrisMcD1 · 2025-05-05T18:01:33Z

No worries, but just out of curiosity: why didn't you continue to use fixup commits for these? I like doing that and putting a bit of explanation into the body of the commit messages, seems like the best of both worlds to me.

I was not aware until right now that you could do a body on a fixup commit, I only ever create them with F in Lazygit while selecting the commit I'm targeting. What is your flow where you add a body?

stefanhaller · 2025-05-05T20:26:58Z

I was not aware until right now that you could do a body on a fixup commit,

Yes, for the auto-squash functionality only the subject is used, the body can be anything you want. Of course it's lost when squashing, so it's only useful during review.

I only ever create them with F in Lazygit while selecting the commit I'm targeting. What is your flow where you add a body?

I also use F to create them, and then I just reword them afterwards.

This solves jesseduffield#3961 because we no longer have a combination of the default and the user defined list. We just have the user defined list.

stefanhaller · 2025-05-07T08:12:23Z

LGTM!

ChrisMcD1 marked this pull request as ready for review February 28, 2025 22:31

ChrisMcD1 mentioned this pull request Feb 28, 2025

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds ChrisMcD1/lazygit#1

Closed

7 tasks

ChrisMcD1 force-pushed the 3961-all-branches-log branch from 1ce80ef to aefa6e5 Compare February 28, 2025 22:32

ChrisMcD1 force-pushed the 3961-all-branches-log branch from aefa6e5 to f0c591e Compare April 10, 2025 03:42

ChrisMcD1 mentioned this pull request Apr 10, 2025

Status panel cycles through log commands when switching panels #4469

Open

stefanhaller added the bug Something isn't working label Apr 29, 2025

stefanhaller reviewed Apr 29, 2025

View reviewed changes

ChrisMcD1 force-pushed the 3961-all-branches-log branch 2 times, most recently from 0d38051 to 5b964f8 Compare May 5, 2025 00:52

ChrisMcD1 force-pushed the 3961-all-branches-log branch from 7067885 to 9df51c8 Compare May 7, 2025 03:30

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds

1028f8e

This solves jesseduffield#3961 because we no longer have a combination of the default and the user defined list. We just have the user defined list.

ChrisMcD1 force-pushed the 3961-all-branches-log branch from 9df51c8 to 1028f8e Compare May 7, 2025 03:34

stefanhaller merged commit e6bd9d0 into jesseduffield:master May 7, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

ChrisMcD1 commented Feb 28, 2025 •

edited

Loading

ChrisMcD1 commented Apr 27, 2025

stefanhaller left a comment

stefanhaller Apr 29, 2025

ChrisMcD1 May 5, 2025

stefanhaller May 5, 2025

stefanhaller Apr 29, 2025

ChrisMcD1 May 5, 2025

stefanhaller May 5, 2025

stefanhaller Apr 29, 2025

ChrisMcD1 May 5, 2025

ChrisMcD1 May 5, 2025

stefanhaller May 5, 2025

stefanhaller May 6, 2025

ChrisMcD1 May 7, 2025

stefanhaller Apr 29, 2025

ChrisMcD1 May 5, 2025

stefanhaller May 5, 2025

ChrisMcD1 May 7, 2025

stefanhaller May 7, 2025

stefanhaller Apr 29, 2025

stefanhaller Apr 30, 2025

stefanhaller commented Apr 29, 2025

ChrisMcD1 commented May 5, 2025

stefanhaller commented May 5, 2025

ChrisMcD1 commented May 5, 2025

stefanhaller commented May 5, 2025

stefanhaller commented May 7, 2025

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

Migrate deprecated AllBranchesLogCmd to AllBranchesLogCmds #4345

Conversation

ChrisMcD1 commented Feb 28, 2025 • edited Loading

ChrisMcD1 commented Apr 27, 2025

stefanhaller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhaller commented Apr 29, 2025

ChrisMcD1 commented May 5, 2025

stefanhaller commented May 5, 2025

ChrisMcD1 commented May 5, 2025

stefanhaller commented May 5, 2025

stefanhaller commented May 7, 2025

ChrisMcD1 commented Feb 28, 2025 •

edited

Loading