phpro / grumphp

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

git_conflict rules crashes on latest version #340

Closed Anahkiasen closed 7 years ago

Anahkiasen commented 7 years ago
Q A
Version 0.11.4
Bug? yes
New feature? no
Question? no
Documentation? no
Related tickets

My configuration

parameters:
    git_dir: .
    bin_dir: ./vendor/bin
    tasks:
        git_conflict: ~

Result:

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function GrumPHP\Task\AbstractExternalTask::__construct(), 1 passed and exactly 3 expected in /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/phpro/grumphp/src/Task/AbstractExternalTask.php:31
Stack trace:
#0 [internal function]: GrumPHP\Task\AbstractExternalTask->__construct(Object(GrumPHP\Configuration\GrumPHP))
#1 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(900): ReflectionClass->newInstanceArgs(Array)
#2 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(451): Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object(Symfony\Component\DependencyInjection\Definition), 'task.git.confli...')
#3 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(957): Symfony\Component\DependencyInjection\ContainerBuilder->get('task.git.c in /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/phpro/grumphp/src/Task/AbstractExternalTask.php on line 31

Fatal error: Uncaught ArgumentCountError: Too few arguments to function GrumPHP\Task\AbstractExternalTask::__construct(), 1 passed and exactly 3 expected in /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/phpro/grumphp/src/Task/AbstractExternalTask.php:31
Stack trace:
#0 [internal function]: GrumPHP\Task\AbstractExternalTask->__construct(Object(GrumPHP\Configuration\GrumPHP))
#1 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(900): ReflectionClass->newInstanceArgs(Array)
#2 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(451): Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object(Symfony\Component\DependencyInjection\Definition), 'task.git.confli...')
#3 /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/symfony/dependency-injection/ContainerBuilder.php(957): Symfony\Component\DependencyInjection\ContainerBuilder->get('task.git.c in /Users/anahkiasen/Sites/stappler/aromatech-extranet-reactor/vendor/phpro/grumphp/src/Task/AbstractExternalTask.php on line 31

Removing the task makes the error go away

veewee commented 7 years ago

It looks like the internal task config is invalid. It should look something like this:

services:
    task.git.conflict:
        class: GrumPHP\Task\Git\Conflict
        arguments:
            - '@config'
            - '@process_builder'
            - '@formatter.raw_process'
        tags:
            - {name: grumphp.task, config: git_conflict}

Can you validate if this works for you?

Anahkiasen commented 7 years ago

Seems like it does yeah

veewee commented 7 years ago

Merged in #341.

veewee commented 7 years ago

It seems like there is another logic error in the task (https://github.com/phpro/grumphp/blob/master/src/Task/Git/Conflict.php#L60-L66). I've fixed it locally but was wondering why this task exitst: Git does this out of the box:

error: commit is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.
fatal: Exiting because of an unresolved conflict.

AND

fatal: cannot do a partial commit during a merge.

GrumPHP is never triggered. Maybe it's better to drop the task completely. It looks like it never worked. Maybe I am doing something wrong. All opinions are welcome :)

cafferata commented 7 years ago

I had the same feelings when I was going trough the documentation and found this task. I was also trying to figure out how this task could help us. Without success! Maybe my old @ceesvanegmond colleague/friend (builder of this task) can help us out?

Git does this out of the box

For others who like to reproduce this, here is an sample (copied from stackoverflow by Mu):

# Note: commands below in format `CUURENT_WORKING_DIRECTORY $ command params`
Desktop $ cd test

First, let us create the repository structure

test $ mkdir repo && cd repo && git init && touch file && git add file && git commit -m "msg"
repo $ cd .. && git clone repo repo_clone && cd repo_clone
repo_clone $ echo "text2" >> file && git add file && git commit -m "msg" && cd ../repo
repo $ echo "text1" >> file && git add file && git commit -m "msg" && cd ../repo_clone

Now we are in repo_clone, and if you do a git pull, it will throw up conflicts

repo_clone $ git pull origin master
remote: Counting objects: 5, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From /home/anshulgoyal/Desktop/test/test/repo
 * branch            master     -> FETCH_HEAD
   24d5b2e..1a1aa70  master     -> origin/master
Auto-merging file
CONFLICT (content): Merge conflict in file
Automatic merge failed; fix conflicts and then commit the result.

If we ignore the conflicts in the clone, and make more commits in the original repo now,

repo_clone $ cd ../repo
repo $ echo "text1" >> file && git add file && git commit -m "msg" && cd ../repo_clone

And then we do a git pull, we get

repo_clone $ git pull
U   file
Pull is not possible because you have unmerged files.
Please, fix them up in the work tree, and then use 'git add/rm <file>'
as appropriate to mark resolution, or use 'git commit -a'.

Note that the file now is in an unmerged state and if we do a git status, we can clearly see the same:

repo_clone $ git status
On branch master
Your branch and 'origin/master' have diverged,
and have 1 and 1 different commit each, respectively.
  (use "git pull" to merge the remote branch into yours)

You have unmerged paths.
  (fix conflicts and run "git commit")

Unmerged paths:
  (use "git add <file>..." to mark resolution)

        both modified:      file

So, to resolve this, we first need to resolve the merge conflict we ignored earlier

repo_clone $ vi file

and set its contents to

text2
text1
text1

and then add it and commit the changes

repo_clone $ git add file && git commit -m "resolved merge conflicts"
[master 39c3ba1] resolved merge conflicts
veewee commented 7 years ago

Since the git_conflict task does not add any value, I've removed it.

ceesvanegmond commented 7 years ago

Maybe it's because of upgrade to GIT 2.x? This task had worked here, it's in several projects and is working fine. Sure GIT does this 'merge notification' out of the box, but not when using an UI like SourceTree for example. I need to dive in the specific cause of this, @veewee is this something you still would like to have in the GrumPHP repository? Or maybe because of GIT 2.x it isn't needed anymore?

veewee commented 7 years ago

Hi @ceesvanegmond,

The task never worked since the task configuration was not added to the project and an error in the if statement. I haven't used SourceTree myself, but it is using the git cli internally I assume it also cannot commit these files. Git 2.X is available since June 2014. It might be time to upgrade your git if you haven't done this aleady :)

Since it never worked and since GIT does this be default, we removed this task. If you can show me a valid use case with a working task, it might be adding value to the project again. But in the meantime, I decided to remove it.

ceesvanegmond commented 7 years ago

Hi @veewee,

Did some research, and you're 100% right. There wan't any config for that specific task, so it never dit anything, which is kinda weird...

@cafferata do you still have an specific use case for this problem?

cafferata commented 7 years ago

For me there is no reason to bringing back the old git_conflict tasks. Thanks for asking @ceesvanegmond!