Fail when file is both in staging and not

[keradus]

Hi. First of all - nice tool ! ;)

Maybe it's a matter of configuration, but please consider the following scenario:

• configure tool to use PHP CS Fixer, init git hooks
• create foo.php file with CS violations
• git add foo.php, git commit -m "foo" -> fails due to PHP CS Fixer violations
• fix violations without adding the file (so git status will show unfixed-files in stage and fixed one in non-stage)
• git commit -m "foo" will proceed, even if the file we are commiting is not valid

Please, could you help?

[veewee]

Hello @keradus,

Glad you enjoy our tool! About your problem: this has been reported before. Please take a look at issue: https://github.com/phpro/grumphp/issues/67

This is not something that can be fixed, since phpcsfixer is running on the actual file. At the point of the last commit, the file has no CS issues and therefor you are able to commit.

[keradus]

2 (alternative) hints then:

• fail PHP CS Fixer task for file if file is in both stage and unstage status
• pass content of staged version to stdin (PHP CS Fixer could work on stdin, no need to work on drive)

[veewee]

The first hint is a bit strange. Both the staged as unstaged changes can succeed or fail. The second one will be pretty hard. php-cs-fixer supports stdin, but other tools don't.

Maybe it's a better option to create a new task that can be enabled that checks files that are both in staged as non staged status. This way you can tell GrumPHP that an error can be triggered. If you want it to just notify when a file exists in both statuses, you can disable the future blocking flag. (see: https://github.com/phpro/grumphp/issues/17)

What do you think of this solution?

(any other ideas @aderuwe ?)

[keradus]

Does this behaviour occur on every task that run on files, like phpmd, composer etc?

---

Notify is not enough for me, and new task could do the trick, but wouldn't be the prettiest solution.

[keradus]

Oh, and then... what about:

• git stash
• run tasks
• git stash pop

?

[veewee]

Yes, the behaviour applies to most tasks. The stash idea is actually pretty nice.

I expect that the changes only have to be stashed during pre-commit and not at commit-msg level. Not sure if stash works as expected during pre-commit. I will have to investigate this a bit further. If it works, it's pretty easy to implement in the actual git-hook. (However, this means that the grumphp git:pre-commit cli tool will pass in your case...)

Thanks for the hint :)

[keradus]

Yeah, I have no idea would it work on commit hooks, but it would be extra cool ;) If not then indeed the stage/unstage task will help as last solution.

Thank you for your support ;)

[aderuwe]

in the case of stash and pop, we should probably either document it well or make it opt-in - if grumphp tasks take a long time to complete, this actually changes what users see in their working dir for some time?

[veewee]

yes, that is indeed another consideration we'll have to make. Good point! I think it's best to first find out if this stash idea will work. If it works we can add a well documented flag like ignore_unstaged_changes. This means that the logic has to be added to the git:pre-commit command.

If the stash idea doesn't work, an additional task as described above would be a solution.

Sounds pretty good for now right?

[aderuwe]

Yup, I prefer the stash solution over the task, too.

[keradus]

works in pre-commit hook

git stash -q --keep-index
call-tools
git stash pop -q

[veewee]

Great, I just started creating an event subscriber for this functionality. Stay tuned :)

[veewee]

Question: should this feature be enabled by default ? It comes with some risks ... When PHP crashes in one of the tasks e.g., the changes are stored in git stash but the user might think that they are lost ... Maybe it's best to disable it by default?

[veewee]

I created a first version. All remarks more then welcome!

[keradus]

For me having it disabled by default is risky - tool say everything is fine, and push the changes, and then see that i have sth not staged. But its to late, the damages were done.

you just run separated process for external tool CLI, if it will blow out you will know it from process informations

[veewee]

I understand that this is strange behaviour. But when we just start to stash changes and something goes wrong, the user might think that his code is gone. That's not something that we want to happen.

Not all tasks run in a separated process. For example: the linters run in the actual GrumPHP process. So if for example you try to validate an XML of X MB, it might be possible that you run into a fatal error because of a memory limit or something.

It might be an idea to register a shutdown function in the listener to make sure that the popStash() method will be called. Not a big fan of those shutdown methods, but in this case it might be the solution?

[veewee]

Fixed in #103. Thanks @keradus for providing the git stash solution!

[keradus]

Great to hear @veewee ! Thank you for all the work ;) I will update my grumphp and work on new version from now, so I will report any issues, if they occur ;)