phpro / grumphp

A PHP code-quality tool
MIT License
4.11k stars 430 forks source link

Git branch pattern whitelist works as "all" instead of "any" #1013

Closed jigarius closed 1 year ago

jigarius commented 2 years ago
Q A
Version GrumPHP 0.18.1
Bug? yes
New feature? no
Question? yes
Documentation? no
Related tickets None

My configuration

# grumphp.yml
tasks:
  git_branch_name:
    whitelist:
      - master
      - '/^release\/\d+\.\d+(\.x)?(-dev)?$/'
      - '/^(dev|feature|bugfix|hotfix)\/\d+-[\-|a-z|0-9]+$/'

Steps to reproduce:

git checkout -b dev/0-branch-naming
git commit -am "My commit."

Result:

Whitelist rule not matched: master
Whitelist rule not matched: /^release\/\d+\.\d+(\.x)?(-dev)?$/
To skip commit checks, add -n or --no-verify flag to commit command
Failed to execute command vendor/bin/grumphp git:pre-commit --skip-success-output: exit status 1

The first pattern in the whitelist matches, but grumphp still fails because the other 2 don't match. Am I doing something wrong? Also, in the long term, I'd suggest renaming whitelist and blacklist to allow and deny.

veewee commented 1 year ago

Hello,

It should pass if one of the lines matches according to the implementation and testsuite. Can you provide a failing unit test? https://github.com/phpro/grumphp/blob/master/test/Unit/Task/Git/BranchNameTest.php#L96-L105

Thanks for the suggestion as well. It's on the radar - yet a lot of work with a lot of breaking changes for everyone. So it will probably be something for v2 - if I ever find the time :)

techdaddies-kevin commented 1 year ago

The issue is that in the regex rules, you need to double escape your forward slash (\/) so that what is passed to preg_match() is a single escaped string (\/)

jigarius commented 1 year ago

I'll close it for now because I think the fix suggested in the previous comment makes sense. I'll reopen it if doesn't work.