Add ability to configure branch color patterns using regex

[mtrajano]

• PR Description

Add ability to specify color patterns in the branchColorPatterns config using regex, ex. JIRA-\d+ would match all branch names in the form JIRA-456.

Example config:

gui:
  branchColorPatterns:
    'docs/.+': 'black' # make all branches prefixed with docs/ have a black color
    'feature/collapse-all': 'red' # make a specfic branch name red
    'IDEA-\d+': 'blue' # make all branches with the prefix `IDEA-` followed by a digit, blue

• 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]

This only seems to support a * at the end, and manually matches prefixes before it. If we want to do pattern matching with wildcards in this way, we should use the Glob package to support the full globbing syntax.

However, I'm unsure if globbing is the best solution here. I'd rather thought we'd use regular expressions; they are more powerful and flexible, and we already use them for other things in the user config, e.g. commit prefixes.

With regular expressions, however, it's a bit harder to maintain backward compatibility transparently. I think for clarity we should add a new config (e.g. BranchColorPatterns) and use that instead of the old one as soon as it has at least one entry. Note that Nikita's branch here doesn't do that quite the way I think it should be: he first checks if there is a match with the new config, and if there isn't, he still falls back to the old config. That's not how I would do it: check the new config (and only the new one) if it has any entries, and only fall back to the old one if it doesn't. This way we can properly deprecate the old one and eventually remove it after some time.

[castlele]

I'm agree that it would be better to use regular expressions here. On my current place of work we have strict rules about branch naming. So, with power of regular expressions I achieved behavior, when only correctly named branches are highlighted:

gui:
  branchColorPatterns:
    "IDS+-[0-9]+-[^']+$": '#29d1f7'

[mtrajano]

Thank you for the feedback @stefanhaller @castlele I'll adjust the pr accordingly. I agree that using regex will give the user a better ability on how to specify the branch rules.

[mtrajano]

Since the new config is replacing the old one I needed a way to know whether a regex pattern was passed or wether I should attempt to match using the old way (by branchType). Once the old method of matching is deprecated this can be removed

[mtrajano]

keeping the old default behavior

[stefanhaller]

@jesseduffield How do you feel about removing this default behavior? I find it surprising and annoying. I don't want it, but it's not obvious how to disable it; previously I had to use this to disable it:

gui:
  branchColors:
    feature: default
    bugfix: default
    hotfix: default

but if I want to use the new regex patterns now, I have to say

gui:
  branchColorPatterns:
    "mypattern": "#ff0000"
    "^feature/": default
    "^bugfix/": default
    "^hotfix/": default

This is very non-obvious, and I feel like I shouldn't have to do that.

[mtrajano]

Had some free time today and updated this pr, let me know if this is closer to what you were imagining @stefanhaller. Given that we have to account for both the new behavior while keeping the old way unchanged it might be a little messy until we deprecate the old behavior. Feel free to suggest any changes, thanks! Here is an image of what it looks like with a sample config:

[jesseduffield]

For what it's worth, that approach looks good to me (code just needs some more documentation, and to be explicit about the old option being deprecated)

[mtrajano]

@jesseduffield Added some more documentation and deprecation warnings, let me know if you think anything is missing clarity

[stefanhaller]

Great work @mtrajano. A few thoughts below, looks good to me otherwise.

[stefanhaller]

@jesseduffield How do you feel about removing this default behavior? I find it surprising and annoying. I don't want it, but it's not obvious how to disable it; previously I had to use this to disable it:

gui:
  branchColors:
    feature: default
    bugfix: default
    hotfix: default

but if I want to use the new regex patterns now, I have to say

gui:
  branchColorPatterns:
    "mypattern": "#ff0000"
    "^feature/": default
    "^bugfix/": default
    "^hotfix/": default

This is very non-obvious, and I feel like I shouldn't have to do that.

[jesseduffield]

@stefanhaller Those feature/bugfix/hotfix things are based on 'gitflow' which is now considered legacy by many, so I'm happy to remove those defaults.

[stefanhaller]

Nice. I took the liberty of separating the removal of the default coloring into a commit of its own, and I added a commit that adds a "breaking changes" popup for the deprecation of the old config and the removal of the default coloring.

This looks good to me now; @jesseduffield Any thoughts about deprecation comments?

[stefanhaller]

I added another fixup (e6fa89c) to fix a test failure. I'm not sure this is the best way to fix it; alternatively we could put an if statement into GetBranchTextStyle to gracefully return DefaultTextColor if no matcher has been set. @jesseduffield Any preference?

@mtrajano This makes me realize that branchPrefixColorCache is not really a cache any more (you could argue that it wasn't before, either), and talking about "prefix" is also wrong now. Should we rename it to something like branchColorMatcher?

[stefanhaller]

@mtrajano Oh, and could you update the PR description please? It is out of date.

[mtrajano]

@mtrajano This makes me realize that branchPrefixColorCache is not really a cache any more (you could argue that it wasn't before, either), and talking about "prefix" is also wrong now. Should we rename it to something like branchColorMatcher?

Yes I also thought the naming choice was weird (nothing to do with caching) but kept it the same since it has the same behavior as before. I can rename this to colorMatcher similar to the struct name or colorPatterns, that to me is more descriptive of what it does, thoughts?

[stefanhaller]

Works for me.

[mtrajano]

Updated the PR description and renamed the variable let me know if that looks good

[mtrajano]

I added another fixup (e6fa89c) to fix a test failure. I'm not sure this is the best way to fix it; alternatively we could put an if statement into GetBranchTextStyle to gracefully return DefaultTextColor if no matcher has been set. @jesseduffield Any preference?

I'm sorry it's possible I overwrote this commit with my last push, let me see if I can add the same

[mtrajano]

Was there anything else you changed or just the test?

[stefanhaller]

You lost all my work, which is a bit annoying to be honest. I did this to save time, not to waste time. 😉

Maybe it's unexpected for contributors to get stuff pushed to their branch by maintainers, but we actually do this all the time, so you need to be careful not to blindly force-push.

I'll recover my things from the reflog and force-push again, hang on a minute...

[stefanhaller]

Ok, I force-pushed again, please pull.

My changes are:

• created a separate commit for the removal of the default branch colors (cdc324f)
• added a fixup for fixing (2d6315a) a test crash (with the caveat above that this might not be the best fix)
• added a "breaking changes" popup for the deprecation of the old config and the removal of the default coloring (ac71ae4)

(I mentioned all of these above...)

[mtrajano]

Looks good now, thankfully wasn't a huge set of changes but yes reflog always does the trick 🙏 Let me know if there's anything else needed to get this merged, thanks for the helpful comments

[stefanhaller]

I added another fixup for updating the config schema (CI failed because of this).

One thing I'm wondering is whether we should explicitly mention that the regex patterns are not anchored, and that users need to do that themselves if they want an exact match for example.

[mtrajano]

One thing I'm wondering is whether we should explicitly mention that the regex patterns are not anchored, and that users need to do that themselves if they want an exact match for example.

Yes I think it's worth mentioning, this behavior felt counter intuitive to me as mentioned here #4130 (comment).

[jesseduffield]

Everything here looks good to me except for the tense of 'deprecation': the comments say that the old config will be deprecated in the future but in reality it's deprecated as of right now: it will be removed in the future.

As an aside, I wonder if it would be good to rename BranchColors to DeprecatedBranchColors (and do the same in future for other deprecated config values) so that it's borderline impossible for a contributor to accidentally use the wrong one. Consider that in gui.go in this PR, the comment wouldn't be necessary if it was called DeprecatedBranchColors

[mtrajano]

@jesseduffield I don't think the config rename is a good idea, it will break a bunch of users configs unexpectedly with a simple commit change. They can keep using the current version until a major version is released in which case the old config value can be just be removed entirely. IMO having the notice as part of a changelog or some sort of upgrade guide is enough and is usually the standard other applications follow. Thoughts @stefanhaller ?

[jesseduffield]

To be clear I'm not suggesting that we change the name that the user uses (i.e. yaml:"branchColors"): I'm suggesting we change the name in our code, which has no impact on user configs.

[mtrajano]

Oh yes in that case it can make sense to serve as a reminder for the maintainers in the future 👍

[stefanhaller]

I'm mildly opposed to renaming the field. It's always slightly confusing when a config variable has a name that's different from the user-facing yaml name, and I would only do that if there's a really strong reason for it. In this case, I don't see a big risk that future contributors will continue to use the old field for new code; I'm having a hard time imagining what kind of code that would be.

Not a super strong opinion, so if you both think we should rename it, I'd be fine with that.

[jesseduffield]

I don't have a strong opinion, so happy to leave the code as-is

[mtrajano]

I just rebased this branch again since it was out of date. Anything else I'm missing to get this one merged?

[stefanhaller]

I was meaning to add a few more documentation tweaks, but didn't get around to it yet. I hope to find some time for this tomorrow.

[stefanhaller]

I tweaked the documentation again to fix the tense of 'deprecation' as @jesseduffield asked for above, and to make it consistent with other deprecated configs. I also removed the documentation of the old config from Config.md; I don't think we need to document deprecated configs.

@jesseduffield Wanna have a final look before I merge?

[jesseduffield]

@stefanhaller I've pushed another commit updating the tense in a couple other places. LGTM

[stefanhaller]

@jesseduffield Thanks, squashed it in