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

[ginolegigot]

Is your feature request related to a problem? Please describe.
Hello! Would it be possible to set commitPrefix as a list so we can change/transform several branch patterns into several commit message prefix pattern ?

Describe the solution you'd like
For example something like this:

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

Currently we have:

git:
  commitPrefix:
    pattern: "^\\w+-\\w+.*"
    replace: '[JIRA $0] '

Thanks for your reading !

[jesseduffield]

Is the idea that different repos use different patterns, or would you want multiple patterns used on the one repo? Cos if it's the former, the proper fix is to just support per-repo configuration (which I am pretty sure we are yet to support)

[ginolegigot]

Hello!
Thanks for your reply, yes the wish is to have multiple patterns used on one repo.
If i take the same example of the doc, if i have a branch feature/AB-123 for features, but if i also have branches like run/FixIssue for run/patches, it would be cool to automate the commit message creation following these two different patterns

[jesseduffield]

I see, and would you like the first matching pattern to be the one that's used?

[ginolegigot]

yes could be as simple as that, if the one is not matched lets try the second on etc.

[jesseduffield]

Okay I'm cool with that. Should be easy enough to implement. Would you be up for raising a PR @ginolegigot ?

[ginolegigot]

i can barely write a print in go but can try. If someone feels ready tho, feel free to take it

[ChrisMcD1]

I've got a local branch setting up this config with a slice of elements in the config, but that is a breaking change. There doesn't appear to be a free way to communicate to yaml that it should unmarahall as a slice, or a slice of length 1. go-yaml/yaml#100

Would you rather I:

1. Make it a breaking schema change and add a migration for it?
2. Write a bit of custom unmarahalling code to handle both schemas

[ChrisMcD1]

Got a PR for this! #4253 Chose option 2 of the above list

[stefanhaller]

I'm not excited about option 2, since your custom unmarshalling code then doesn't match the generated schema, and that's a problem. Not just for generating Config.md (as you found out in your PR), but also for validation in the editor for those that support it (e.g. VS Code with Redhead's YAML extension). This is a great and important feature that we shouldn't break. Now, theoretically it might be possible to get it to work using jsonschema's oneOf construct, which allows specifying two possible types for a key, but we tried that before for other cases and couldn't get it to work.

My suggestion would be to add a new config option commitPrefixes that is an array. Deprecate the old one (by adding an entry to the BreakingChangesByVersion map in english.go). Read both properties normally. At runtime, use the new config if it has any entries, and fall back to the old one otherwise. After a year or so we can remove the old one.

This is similar to what we did for the branch colors in #4130.

Actually, in this case it seems that we could do better and auto-migrate the config from commitPrefix to commitPrefixes at load time; no need for a deprecation warning. We couldn't do that for the branch color patterns because in that case the semantics of the values had changed too, but in this case it looks like it would be possible.

[ChrisMcD1]

I couldn't migrate the name to commitPrefixes because that name was already used for the per-repo map of prefixes. But it does auto-migrate!

I noticed a decent chunk of similar code in the migrations. Each possible migration unmarshalls, does its checks, and then re-marshalls. It has to check for empty documents and such every time. Would you be open to a refactoring PR that parses the document into a single *yaml.Node and then acts on that for the duration of the migrations?

[stefanhaller]

I noticed a decent chunk of similar code in the migrations. Each possible migration unmarshalls, does its checks, and then re-marshalls. It has to check for empty documents and such every time. Would you be open to a refactoring PR that parses the document into a single *yaml.Node and then acts on that for the duration of the migrations?

I thought about that back when I wrote the code, and I swear I had a reason for doing it this way. If I could only remember what the reason was...

I think the reason might have been that it's more robust this way to decide whether we have to write the file back; with your approach, each migration needs an extra bool return value that communicates back whether something was changed. Not sure that's a good reason though. (Alternatively, we could make a deep copy of the yaml node and then a DeepEquals afterwards to see if something changed; also awkward.)

So yes, I think I wouldn't be opposed to refactoring this if you find it important enough.

[stefanhaller]

Fixed by #4261.