Validate Git branch naming convention #328 #329

ademarco · 2017-03-09T14:15:41Z

Q	A
Branch	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Documented?	yes
Fixed tickets	#328

This PR adds branch name checking allowing to define branch names as follows:

parameters:
  tasks:
      git_branch_name:
        matchers:
          - '/^issue-\d+-[a-z-]+/'
        case_insensitive: false

New Task Checklist:

ademarco · 2017-03-09T14:19:37Z

Here a first draft, still need to provide tests.

I'm not sure how to answer to these:

 Is the run() method readable?
 Is the run() method using the configuration correctly?

ademarco · 2017-03-12T19:03:41Z

@veewee does the PR look OK or should I polish it a little more?

veewee

Hi @ademarco,

Thanks for the PR! It took me some time to get to the reviewing part. I've added some remarks to the code. Can you take a look at them?

veewee · 2017-03-14T06:58:53Z

doc/tasks.md

@@ -19,6 +19,7 @@ parameters:
        gherkin: ~
        git_blacklist: ~
        git_commit_message: ~
+        git_branch_name: ~


This list should be ordered alfabetical.

Done, also task link below in the same file.

veewee · 2017-03-14T06:58:58Z

README.md

@@ -102,6 +102,7 @@ parameters:
        gherkin: ~
        git_blacklist: ~
        git_commit_message: ~
+        git_branch_name: ~


This list should be ordered alfabetical.

veewee · 2017-03-14T07:01:41Z

src/Configuration/GrumPHP.php

@@ -41,6 +41,14 @@ public function getGitDir()
    }

    /**
+     * @return \Gitonomy\Git\Repository
+     */
+    public function getGitRepository()


It's better to inject the repository directly in the task constructor instead of using the configuration as additional service container. Can you remove this method?

Yep, sorry! Removed, I'm using dependency injection now.

veewee · 2017-03-14T07:03:51Z

src/Task/Git/AbstractRegex.php

+use GrumPHP\Task\TaskInterface;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+
+abstract class AbstractRegex implements TaskInterface


Currently I don't think this abstract class is the way to go. There are some people working on the git_commit_message task in e.g. #319. This will surely break the implementation and doesn't add much value since it only contains the configurable options. I think it's better to drop this class and create a separate implementation.

veewee · 2017-03-14T07:05:06Z

src/Task/Git/AbstractRegex.php

+    {
+        $resolver = new OptionsResolver();
+        $resolver->setDefaults([
+          'case_insensitive' => true,


I don't know if the case_insensitive and addition_modifiers are the way to go. Maybe it's better to enforce strict modifiers in the pattern. This was also mentioned in #319.

Ok, removed in favour of already existing additional_modifiers option

veewee · 2017-03-14T07:05:34Z

src/Task/Git/BranchName.php

+     *
+     * @throws RuntimeException
+     */
+    protected function runMatcher(array $config, $name, $rule, $ruleName)


You can mark this one as private

veewee · 2017-03-14T07:05:56Z

src/Task/Git/BranchName.php

+     *
+     * @return string
+     */
+    public function getCurrentBranchName()


You can mark this one as private

veewee · 2017-03-14T07:16:39Z

src/Task/Git/BranchName.php

+    public function getCurrentBranchName()
+    {
+        $gitRepository = $this->grumPHP->getGitRepository();
+        $branch = new Branch($gitRepository, $gitRepository->getHead()->getRevision());


I am not sure this will work in all cases. A commit can be in multiple branches. How does this code handle these?
Maybe it's better to execute the git symbolic-ref --short HEAD command through the Repository::run() command?

Yep, removed that in favour of ::run() method

veewee · 2017-03-14T07:17:21Z

src/Task/Git/CommitMessage.php


 /**
 * Git CommitMessage Task
 */
-class CommitMessage implements TaskInterface
+class CommitMessage extends AbstractRegex


Can you revert this class to the old version?

veewee

Thanks for the quick update! I've added 2 more remarks.

veewee · 2017-03-14T08:34:22Z

src/Task/Git/BranchName.php

+     */
+    public function canRunInContext(ContextInterface $context)
+    {
+        return $context instanceof RunContext;


Shouldn't this also run in pre-commit context? Now it will not be triggered during a git commit.

Ok done, I've added the pre commit context too.

Great, can you add a test for this as well?

veewee · 2017-03-14T08:34:49Z

src/Task/Git/BranchName.php

+    {
+        $regex = new Regex($rule);
+
+        $additionalModifiersArray = array_filter(str_split((string) $config['additional_modifiers']));


The additional modifiers will always be a string since it is enforce by the OptionsResolver.

veewee · 2017-03-14T14:15:54Z

src/Task/Git/BranchName.php

+     */
+    public function run(ContextInterface $context)
+    {
+        $name = trim($this->repository->run('symbolic-ref', ['HEAD', '--short']));


When the current revision is a non symbolic reference, the command will throw an exception like:

Error code: 128
Message: fatal: ref HEAD is not a symbolic ref

This can happen in detached head or if you checked out a specific commit.
This will throw a Gitonomy\Git\Exception\ProcessException.

How would you like to handle this issue on the task? Should it be configurable to allow non symbolic references or not? We might want to fetch the error message and display a user-friendly error message instead?

More info: https://git-scm.com/docs/git-symbolic-ref

Maybe we should not check for branch naming convention when you are on a detached head or you checkout a specific commit? We could just catch the exception and skip the step by returning a TaskResult::createSkipped(), does that make sense?

Maybe it's best to make this configurable through an extra configuration flag?
Some people might want to allow it, others may have something against it.

Rules:

If you are allowed to commit on non symbolic references: task passes

If you are not allowed to commit on non symbolic references: task fails with a decent error message.

Ok shall we call the boolean option allow_symbolic_references? Should it default to true?

something like 4f95d27 ?

Looks good. I've added some remarks.
Can you add documentation and tests for this new flag?

veewee · 2017-03-15T05:49:53Z

src/Task/Git/BranchName.php

        ]);

        $resolver->addAllowedTypes('matchers', ['array']);
        $resolver->addAllowedTypes('additional_modifiers', ['string']);
+        $resolver->addAllowedTypes('allow_symbolic_references', ['boolean']);


You are trying to search the branch name based on the symbolic reference 'HEAD'. Since in detached mode there is no symbolic reference HEAD, you retrieve the error. So I don't think the naming is correct here.
Maybe something like allow_detached_head is easier to understand?

Ok I'll go for that one.

veewee · 2017-03-15T05:55:13Z

src/Task/Git/BranchName.php

+            if ($config['allow_symbolic_references'] === false) {
+                $message = "Branch naming convention task is not allowed to run on symbolic references.";
+                return TaskResult::createFailed($this, $context, $message);
+            }


If the command fails and the detached head is allowed, you'll have to return a success result. Otherwise the matchers will fail.
If the detached head is not allowed, you can return the error message. Maybe it should be a bit more clear for less experienced users?

veewee · 2017-03-16T06:46:49Z

Thanks for your work!

drupol · 2017-03-16T06:50:54Z

👍

ademarco · 2017-03-16T07:09:51Z

Thanks for merging @veewee !

jfhovinne · 2017-03-16T07:20:22Z

👍

netlooker · 2017-03-16T08:19:54Z

👍 Massive feature :D

Add initial code for Git branch name task phpro#328

a6bc7b8

ademarco added 4 commits March 9, 2017 15:26

Fix PHPCS issue phpro#328

47ee534

Add tests phpro#328

dea3a2e

Update README phpro#328

015ed01

Fix README.md phpro#328

464e565

veewee requested changes Mar 14, 2017

View reviewed changes

Refactor after mintainer review phpro#328

8593c73

veewee reviewed Mar 14, 2017

View reviewed changes

Make sure task runs in GitPreCommitContext too phpro#328

7429d11

veewee added the new feature label Mar 14, 2017

veewee added this to the Version 0.11.4 milestone Mar 14, 2017

Add test for GitPreCommitContext phpro#328

e6b1846

veewee reviewed Mar 14, 2017

View reviewed changes

First attempt on symbolic references handling phpro#328

4f95d27

veewee reviewed Mar 15, 2017

View reviewed changes

ademarco added 2 commits March 15, 2017 12:04

Add documentation and test phpro#328

e48687f

Make PhpParser happy phpro#328

194ae38

veewee approved these changes Mar 16, 2017

View reviewed changes

veewee merged commit 6fac607 into phpro:master Mar 16, 2017

veewee mentioned this pull request Mar 16, 2017

Validate Git branch naming convention #328

Closed

ademarco deleted the 328-branch-names branch March 16, 2017 07:11

Validate Git branch naming convention #328 #329

Validate Git branch naming convention #328 #329

Conversation

ademarco commented Mar 9, 2017 • edited Loading

New Task Checklist:

ademarco commented Mar 9, 2017

ademarco commented Mar 12, 2017

veewee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veewee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veewee Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ademarco Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veewee commented Mar 16, 2017

drupol commented Mar 16, 2017

ademarco commented Mar 16, 2017

jfhovinne commented Mar 16, 2017

netlooker commented Mar 16, 2017

ademarco commented Mar 9, 2017 •

edited

Loading

veewee Mar 14, 2017 •

edited

Loading

ademarco Mar 14, 2017 •

edited

Loading