Print migration changes to the console when migrating config file

[stefanhaller]

• PR Description

This might be useful to see in general (users will normally only see it after they quit lazygit again, but still). But it is especially useful when writing back the config file fails; some users have their config file in a read-only location, so we had reports of lazygit no longer starting up when migration was necessary. #4210 was supposed to improve this a bit, but it didn't tell users what changes need to be made to the config file. Now we tell them, and users can then make these changes manually if they want.

We do this only at startup, when the GUI hasn't started yet. This is probably good enough, because it is much less likely that writing back a migrated repo-local config fails because it is not writeable.

Example output:

The user config file /Users/stk/Library/Application Support/lazygit/config.yml must be migrated. Attempting to do this automatically.
The following changes were made:

- Renamed 'gui.windowSize' to 'screenMode'
- Changed 'null' to '<disabled>' for keybinding 'keybinding.universal.confirmInEditor'
- Changed 'stream: true' to 'output: log' in custom command

Config file saved successfully to /Users/stk/Library/Application Support/lazygit/config.yml

The branch also contains a lot of code cleanups.

• 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

[stefanhaller]

@ChrisMcD1 You have been around in this code a lot, feel like reviewing this?

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 1fbfefa1	✅ 64.89%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (1fbfefa)	Report Missing	Report Missing	Report Missing
Head commit (cabcd54)	56435	49006	86.84%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4548)	94	61	64.89%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[ChrisMcD1]

What do you think about printing the entire file contents of the new file?
Could be useful if someone just wants to copy and paste that into whatever system sets up their config file.
Telling them what we are doing in text is surely helpful, but they'll still need to recreate those changes in order to launch lazygit. Might as well help them along that journey.

[stefanhaller]

What do you think about printing the entire file contents of the new file?

Hm, I don't know. My config file is pretty long, I don't think I would want it printed entirely. Especially if I then can't tell what changed. (And some people have copied the entire default config into their config file.)

Also, most people running into the issue with read-only config files seem to be using home-manager, and from what I understand a copy of the entire config doesn't help them, because they need to enter it in home-manager's own format. (I might have gotten that wrong though.)

At first I had toyed with the idea of printing a diff, and actually had that working, but I found that a context diff is less useful than I thought, because in longer sections you can't see which section you're in; so that's why I settled on the current approach of printing change instructions.

[ChrisMcD1]

If we are going to switch to returning nil in all cases when we don't make changes, could we just use a check against changedContent == nill instead?

No strong preference for that, I just thought of the idea

[stefanhaller]

Interesting, that's what I initially did. It has the theoretical problem that if we ever write a migrator that only removes a key without replacement, then you can't distinguish the case that nothing was done from the case that the config file only contained that key, and we removed it. (You might argue that the Yaml library will probably return an empty bytes slice instead of nil in that case, so we could still distinguish that, but I'm actually not sure if that's really the case, and I don't think we should rely on it.)

The extra bool is a bit clearer and safer.

[ChrisMcD1]

Seems like this library has support for iterators, so this could be more cleanly expressed as:

for change := range changes.KeysFromOldest() {
    changesText += fmt.Sprintf("- %s\n", change)
}

https://github.com/wk8/go-ordered-map?tab=readme-ov-file#iterator-support-go--123

[stefanhaller]

That's only available in master though, not in the latest release that we're using (which is from 2023). I didn't want to update to the master version.

It's becoming less of an issue now that we encapsulate this in the generics module, see below.

[ChrisMcD1]

Why do a bool instead of an struct{}? I don't know my idiomatic go that well, but I thought struct{} was how to say "I want a set, but that doesn't exist in go, so my values are empty"

[stefanhaller]

Good question; I simply modelled this after generics/set.Set without thinking much about it. I suppose in that case it was done with a bool because it makes checking for containment slightly easier (return set.m[value] rather than _, found := set.m[value]; return found).

Anyway, this discussion shows that we shouldn't use a map in client code, but provide a proper OrderedSet abstraction instead; I was just too lazy to do that initially. See jesseduffield/generics#2 (still using a bool there, simply for consistency with set, but it's now an implementation detail and doesn't matter), and 0138b9e.

[stefanhaller]

It looks like the discussion has stalled. @ChrisMcD1, I think your concerns above have all been addressed, is that true? Any further thoughts from your side?

@jesseduffield Are you interested in reviewing this too?

[ChrisMcD1]

Yep, looks good to me!

[jesseduffield]

LGTM