feat: Support multiple commit prefixes #4261

ChrisMcD1 · 2025-02-14T01:37:29Z

PR Description
This implementation, unlike that proposed in Allow multiple commit prefixes #4253 keeps the yaml schema easy, and does a migration from the single elements to a sequence of elements.

Addresses #4194

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-02-14T01:42:48Z

docs/Config.md

-  # See https://github.com/jesseduffield/lazygit/blob/master/docs/Config.md#predefined-commit-message-prefix
-  commitPrefix:
-    # pattern to match on. E.g. for 'feature/AB-123' to match on the AB-123 use "^\\w+\\/(\\w+-\\w+).*"
-    pattern: ""
-
-    # Replace directive. E.g. for 'feature/AB-123' to start the commit message with 'AB-123 ' use "[$1] "
-    replace: ""
-


Once again, go generate removed this. I guess it just doesn't support sequences?

Yes, that's true. The config doc is generated from the default values specified in the schema, and arrays don't have the concept of a default value.

It's actually a good thing that this is removed from the docs. We had several cases of people copying the entire defaults section from the docs to their config file, and then it doesn't work as expected; see here for more information.

ChrisMcD1 · 2025-02-14T14:50:06Z

The failed integration test custom_commands/suggestions_preset seems flaky. I'm unable to reproduce it locally on this branch, but it fails intermittently on a different branch I'm working on. Is that a known issue?

stefanhaller

This is really nice, great work. I like the added tests. Just two minor things below.

stefanhaller · 2025-02-16T10:23:44Z

docs/Config.md

-  # See https://github.com/jesseduffield/lazygit/blob/master/docs/Config.md#predefined-commit-message-prefix
-  commitPrefix:
-    # pattern to match on. E.g. for 'feature/AB-123' to match on the AB-123 use "^\\w+\\/(\\w+-\\w+).*"
-    pattern: ""
-
-    # Replace directive. E.g. for 'feature/AB-123' to start the commit message with 'AB-123 ' use "[$1] "
-    replace: ""
-


Yes, that's true. The config doc is generated from the default values specified in the schema, and arrays don't have the concept of a default value.

It's actually a good thing that this is removed from the docs. We had several cases of people copying the entire defaults section from the docs to their config file, and then it doesn't work as expected; see here for more information.

stefanhaller · 2025-02-16T10:26:08Z

docs/Config.md

-    pattern: "^\\w+\\/(\\w+-\\w+).*"
-    replace: '[$1] '
+    - pattern: "^\\w+\\/(\\w+-\\w+).*"
+      replace: '[$1] '


It would be good to have an example with multiple entries here (like you have for commitPrefixes below), and maybe say explicitly that they are tried in order, first match wins.

The reason why I prefer to have this here rather than below is that I'd like to deprecate (and eventually remove) commitPrefixes; now that we have repo-local config files, it's no longer needed.

Added an example with multiple prefixes!

I left the example down below as well, want me to remove?

No, it's fine to keep them. We'll remove that section at some point anyway.

stefanhaller · 2025-02-16T10:43:36Z

pkg/utils/yaml_utils/yaml_utils.go

 	return nil, nil
 }

+func TransformNode(yamlBytes []byte, path []string, transform func(node *yaml.Node) (bool, error)) ([]byte, error) {


Some brief documentation would be nice; e.g. what the path parameter means, and what the bool return value of the callback means.

Would it maybe be useful to add a test with some simple transformation like converting int values to string values to illustrate the usage?

Added some docs, and a test. The test didn't end up being as simple as I would like because of the complexity of expressing the conversion of int values to string, but it is a test!

Very nice. I pushed a fixup (1ea96eb) to address a linter error; yaml.Node.Value is a string no matter whether the type is int or str, so it's unnecessary to sprintf it again.

stefanhaller · 2025-02-17T18:57:55Z

Cool, happy with this now. I had to push one more fixup (d81cf52) to remove some trailing whitespace that triggered my OCD. 😄

Merging.

This implementation, unlike that proposed in jesseduffield#4253 keeps the yaml schema easy, and does a migration from the single elements to a sequence of elements.

Before we changed the commitPrefix config to a list in #4261, we had this bug where the defaults section in Config.md would erroneously list the default for commitPrefix as git: commitPrefix: pattern: "" replace: "" This was not correct, commitPrefix was a pointer, and the default for that was nil, which is not the same. Now, some people copied and pasted the entire defaults section into their config files, setting the commitPrefix to an empty pattern (which is not the same as not setting it at all). And this caused the branch name to be filled in to the subject field for every commit; see for example #3632. New users copying the defaults section into their config file in the current version no longer have this problem because now that commitPrefix is a list, it is no longer included in the defaults section. However, the migration that we added in #4261 would happily carry over those empty strings into a list entry, so people migrating from an older version still have the broken config in their config files. To work around the issue, ignore commit prefix settings whose pattern is an empty string. I can't imagine a valid reason why people would actually want to set the pattern to an empty string, so I assume this only comes from the broken defaults problem described above.

Before we changed the commitPrefix config to a list in #4261, we had this bug where the defaults section in `Config.md` would erroneously list the default for commitPrefix as ```yml git: commitPrefix: pattern: "" replace: "" ``` This was not correct, commitPrefix was a pointer, and the default for that was nil, which is not the same. Now, some people copied and pasted the entire defaults section into their config files, setting the commitPrefix to an empty pattern (which is not the same as not setting it at all). And this caused the branch name to be filled in to the subject field for every commit; see for example #3632. New users copying the defaults section into their config file in the current version no longer have this problem because now that commitPrefix is a list, it is no longer included in the defaults section. However, the migration that we added in #4261 would happily carry over those empty strings into a list entry, so people migrating from an older version still have the broken config in their config files. To work around the issue, ignore commit prefix settings whose pattern is an empty string. I can't imagine a valid reason why people would actually want to set the pattern to an empty string, so I assume this only comes from the broken defaults problem described above.

ChrisMcD1 mentioned this pull request Feb 14, 2025

Allow multiple commit prefixes #4253

Closed

7 tasks

ChrisMcD1 marked this pull request as ready for review February 14, 2025 01:38

ChrisMcD1 commented Feb 14, 2025

View reviewed changes

ChrisMcD1 mentioned this pull request Feb 15, 2025

Race Condition in Branch Focus Rendering #4267

Closed

stefanhaller reviewed Feb 16, 2025

View reviewed changes

ChrisMcD1 force-pushed the multiple-commit-prefixes-auto-migrations branch 2 times, most recently from 8ab32f2 to c9d6f2f Compare February 16, 2025 21:24

stefanhaller force-pushed the multiple-commit-prefixes-auto-migrations branch from c9d6f2f to 1ea96eb Compare February 17, 2025 18:55

feat: Support multiple commit prefixes

2fa4ee2

This implementation, unlike that proposed in jesseduffield#4253 keeps the yaml schema easy, and does a migration from the single elements to a sequence of elements.

stefanhaller force-pushed the multiple-commit-prefixes-auto-migrations branch from 1ea96eb to 2fa4ee2 Compare February 17, 2025 18:58

stefanhaller enabled auto-merge February 17, 2025 18:58

stefanhaller added the enhancement New feature or request label Feb 17, 2025

stefanhaller merged commit 0d155e1 into jesseduffield:master Feb 17, 2025
14 of 15 checks passed

stefanhaller mentioned this pull request Feb 17, 2025

Set git.commitPrefix as a list instead of a dict to change several branch pattern into different commit messages prefixes #4194

Closed

abidkyo mentioned this pull request Feb 22, 2025

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

Closed

ChrisMcD1 mentioned this pull request Feb 28, 2025

lazygit cannot be opened with read-only config files #4337

Closed

stefanhaller mentioned this pull request Mar 9, 2025

Fix commitPrefix setting with empty pattern #4381

Merged

Uh oh!

feat: Support multiple commit prefixes #4261

feat: Support multiple commit prefixes #4261

Uh oh!

Conversation

ChrisMcD1 commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisMcD1 commented Feb 14, 2025

Uh oh!

stefanhaller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanhaller commented Feb 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChrisMcD1 commented Feb 14, 2025 •

edited

Loading