Don't rewrite config file unnecessarily when it contains commitPrefixes #4311

ChrisMcD1 · 2025-02-23T01:40:07Z

PR Description
The issue statement 'commitPrefixes' config triggers 'deprecated config' warning #4310 is exactly right, that the commitPrefixes element improperly claims that it has modified the yaml whenever it exists, even if it does not need to do changes.

Now, we initialize it to false, only set it to true inside our modification section of the for loop.
Tests updated to add one that would have failed prior to this change. The syntax change to use named struct fields instead of positional fields felt nice since I wanted to just define on a single one of them the assertAsString field.

The reason that field is required at all is the 2nd complaint on the linked issue about the formatting change, is I don't believe is something that is trivial to fix. I observed on existing migrations before I wrote this one. But if it is easy to wrap up into this, let me know!

Also, how do we normally backport things into previous releases? We'll probably want this to make it into a 0.47.2.

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
[X]You've read through your own file changes for silly mistakes etc

stefanhaller

LGTM, just two small things below.

I agree it would be good to release a 0.47.2 with this. There's no need for a backporting process, we usually just cut a new release from master and don't care that it also contains a few other changes.

stefanhaller · 2025-02-23T10:38:49Z

pkg/config/app_config_test.go

+    foo:
+      - pattern: "^\\w+-\\w+.*"
+        replace: '[JIRA $0] '`,
+			expected: `


This test is relying on the knowledge that our yaml rewriting will change the indentation to 4. If we change that as proposed in #4312, this test will no longer catch the error but succeed either way.

I think I would use some other indentation (e.g. 3 at top level, 1 at the second level) to make this more robust.

stefanhaller · 2025-02-23T10:42:08Z

pkg/config/app_config_test.go

+				assert.Equal(t, s.expected, string(actualBytes))
+			} else {
+				assert.Equal(t, expectedConfig, actualConfig)
+			}


I'm not sure this change is necessary. If we do it, then we should also get rid of the indentation changes in many of the tests (and the comments "indentation is not preserved etc"). I got rid of many of those already in #4312 already though, so I feel we can just leave this as is.

If this change is not necessary, are we fine without any tests in this PR? I'm fine with that, if so.

That's not what I meant, but I misread the code. I have mistaken this for the tests in yaml_utils_test.go, which always do string comparisons. I thought this one would do that too, before your change here; I didn't realize it always did yaml node comparisons.

So I guess what I would suggest is to change this test to work in a similar way as the ones in yaml_utils_test.go. I pushed a WIP commit that makes this change, but it would of course be nicer to have this before the other commit in the history. Happy to make this change if you agree, and if you don't enjoy history cleanup work (I do 😄).

ChrisMcD1 · 2025-02-23T17:39:34Z

@stefanhaller Whoops! I was doing the rebase at the same time as you!

stefanhaller · 2025-02-23T17:40:49Z

No worries, feel free to force-push over my version, it wasn't much work.

If you want to pick up my changes, it would be easy to cherry-pick that commit.

ChrisMcD1 · 2025-02-23T17:40:50Z

I restored to your commit

stefanhaller · 2025-02-23T17:43:01Z

It's a bad habit of mine to rebase contributor's branches and force-push them, I should do that less often, or at least ask for an ok before I do that. I'm sometimes impatient and don't want to wait for the communication roundtrips...

ChrisMcD1 · 2025-02-23T17:45:18Z

I understand you there!

Just removed the assertAsString field since it is now useless. I'm happy with the tests like this if you want to squash and merge

This makes the tests a little bit easier to read, the multi-line string literals make this otherwise a little difficult.

…ment

stefanhaller · 2025-02-23T18:03:19Z

Ok. I force-pushed again with a cleaned-up commit history, I also changed the indentation of the added test as suggested here to make it more robust.

Wanna have a last look before I merge?

ChrisMcD1

Looks Good To Me!

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | patch | `v0.47.1` -> `v0.47.2` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jesseduffield/lazygit (jesseduffield/lazygit)</summary> ### [`v0.47.2`](https://github.com/jesseduffield/lazygit/releases/tag/v0.47.2) [Compare Source](jesseduffield/lazygit@v0.47.1...v0.47.2)  Small patch release for you all. This is mainly to fix an issue with v0.47.1 which erroneously re-indented users' lazygit config files on startup. Shout-out to [@karimkhaleel](https://github.com/karimkhaleel) for his MR with some gnarly yaml-handling code. And a special shout-out to [@ChrisMcD1](https://github.com/ChrisMcD1) who has been pumping out many great contributions lately. Great to have you aboard. #### What's Changed ##### Enhancements 🔥 - Skip post-checkout hook when discarding changes by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4313 ##### Fixes 🔧 - fix: Disable global keybinds when confirmation is active by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4284 - Don't rewrite config file unnecessarily when it contains commitPrefixes by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4311 - Change side panel width calculation to work for larger numbers by [@ChrisMcD1](https://github.com/ChrisMcD1) in jesseduffield/lazygit#4287 ##### Maintenance ⚙️ - Fix auto-release schedule by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4308 - Use indentation of 2 when rewriting auto-migrated config file by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4312 - Use refs in jsonschema by [@karimkhaleel](https://github.com/karimkhaleel) in jesseduffield/lazygit#4309 - Improve release workflow by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4307 - Filter out dev comments from schema by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4319 - Fix release script by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4322 - Fix release script once again by [@jesseduffield](https://github.com/jesseduffield) in jesseduffield/lazygit#4323 ##### Docs 📖 - Improve the error message when users have gpg signing turned on by [@stefanhaller](https://github.com/stefanhaller) in jesseduffield/lazygit#4296 **Full Changelog**: jesseduffield/lazygit@v0.47.1...v0.47.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

stefanhaller reviewed Feb 23, 2025

View reviewed changes

stefanhaller force-pushed the 4310-fix-commit-prefixes-deprecated branch from 1723b86 to d83208d Compare February 23, 2025 17:38

ChrisMcD1 force-pushed the 4310-fix-commit-prefixes-deprecated branch from d83208d to 35f8c32 Compare February 23, 2025 17:39

ChrisMcD1 force-pushed the 4310-fix-commit-prefixes-deprecated branch from 35f8c32 to d83208d Compare February 23, 2025 17:40

stefanhaller and others added 3 commits February 23, 2025 18:59

Change test to use named struct fields instead of positional fields

4b30bc6

This makes the tests a little bit easier to read, the multi-line string literals make this otherwise a little difficult.

Change TestCommitPrefixMigrations to compare only strings

38ab7eb

Make commit prefixes migration only return true if it enters if state…

72b9e83

…ment

stefanhaller force-pushed the 4310-fix-commit-prefixes-deprecated branch from 7620855 to 72b9e83 Compare February 23, 2025 18:01

stefanhaller added the bug Something isn't working label Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

View reviewed changes

stefanhaller changed the title ~~Make commit prefixes migration only return true if it enters if statement~~ Don't rewrite config file unnecessarily when it contains commitPrefixes Feb 23, 2025

stefanhaller merged commit 62c6ba7 into jesseduffield:master Feb 23, 2025
15 checks passed

stefanhaller mentioned this pull request Feb 23, 2025

'commitPrefixes' config triggers 'deprecated config' warning #4310

Closed

jesseduffield mentioned this pull request Feb 25, 2025

lazygit 0.47.2 Homebrew/homebrew-core#208878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't rewrite config file unnecessarily when it contains commitPrefixes #4311

Don't rewrite config file unnecessarily when it contains commitPrefixes #4311

ChrisMcD1 commented Feb 23, 2025 •

edited

Loading

stefanhaller left a comment

stefanhaller Feb 23, 2025

stefanhaller Feb 23, 2025

ChrisMcD1 Feb 23, 2025

stefanhaller Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 left a comment

Don't rewrite config file unnecessarily when it contains commitPrefixes #4311

Don't rewrite config file unnecessarily when it contains commitPrefixes #4311

Conversation

ChrisMcD1 commented Feb 23, 2025 • edited Loading

stefanhaller left a comment

Choose a reason for hiding this comment

stefanhaller Feb 23, 2025

Choose a reason for hiding this comment

stefanhaller Feb 23, 2025

Choose a reason for hiding this comment

ChrisMcD1 Feb 23, 2025

Choose a reason for hiding this comment

stefanhaller Feb 23, 2025

Choose a reason for hiding this comment

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 commented Feb 23, 2025

stefanhaller commented Feb 23, 2025

ChrisMcD1 left a comment

Choose a reason for hiding this comment

ChrisMcD1 commented Feb 23, 2025 •

edited

Loading