Checking unstaged files on git commit -a

[kvdnberg]

Hello!

We are implementing GrumPHP as a trail, currently only running phpcs, in our company and hope to have it adopted soon but I am running into a problem.

Maybe I'm missing something but is there no way to have GrumPHP check a git commit -a?

Unstaged files are ignored when GrumPHP is run, meaning a git commit -a where no files are staged will always pass. I thought there might be a way to have the unstaged files checked as well but from what I can find in the documentation and code is there is only a way to have the unstaged stuff stashed and unstashed upon running.

The whole reason we want to implement GrumPHP is to have all commits checked to prevent silly/preventable mistakes from being committed, and a reviewer having to pick them out manually. If this is circumvented by running git commit -a it kind of defeats the purpose.

[veewee]

Hello @kvdnberg,

The problem with git commit -a is that the files aren't staged before the pre-commit hook ran. We are fetching the changes with the command:

git diff -r -p -m -M --full-index --staged

This results in a list of all changed files that are staged in GIT. But since the changed file isn't staged yet, GrumPHP won't detect the file. I've looked around in the git-diff documentation and in some other projects, but I currently haven't found any good solution for this problem. Maybe you've got an idea ?

Also note that it is always possible to commit with the -n flag which will skip all commit hooks. The user can also disable or remove the git hook. So there is always a workaround for someone who doesn't want GrumPHP to validate the commit. This tool can be very handy, when used right. You will have to inform your team that it is not done to bypass the tests.

If you ALWAYS want to enforce the validation of the commit, it is also possible to run GrumPHP on a continious integration platform like Travis or Jenkins. This is the only way to make sure that no commit breaks the rules you defined as a team.

[kvdnberg]

Thanks @veewee for your feedback.

I know that using -n will circumvent grumphp, and that would be a way for a dev to get around it, but with git commit -a the circumvention is not intentional, particularly because GrumPHP will show the "All good" message, the dev will assume the commit is checked.

I will see if I can figure out a way to select the files properly on a -a commit.

[kvdnberg]

Hi @veewee,

There is something strange about the way GrumPHP executes the git diff it would seem. If I run the exact same diff command in the pre-commit hook script itself, the changes are there on a git commit -a. Any ideas why this might be?

./.git/hooks/pre-commit:

git diff -r -p -m -M --full-index --staged

[veewee]

The difference is that the diff runs in PHP.

I just tried the hook and opened another terminal screen where I entered git status. The changes are still not staged in the second terminal. So my best guess is that git is adding the changes in the cache of the process that is running at that moment.

In the git hook a PHP command is triggered which will create a separate process through the Symfony ProcessBuilder. This means that the command acts like it runs in the second terminal screen.

It sure is strange thing! :)

[kvdnberg]

I have been searching a lot for a way to at least identify that we are dealing with a git commit -a instead of a git commit inside the pre-commit hook but it haven't been able to find anything yet. I figured this would be the easiest way to let GrumPHP know that in this case the unstaged files needs to be taken into account. An alternative would be passing the diff from the pre-commit hook script but this would be quite messy I guess. I still want to try and figure something out to fix this but at the moment I am letting it rest while working on other things, so if anyone has insights, ideas, .....

[veewee]

I'm starting to think that passing the diff is the only solution for this problem. Instead of passing the content of the diff as an argument, it is possible to pipe it to the command like this:

# Short version:
git diff -r -p -m -M --full-index --staged | php test.php

# More readable version:
DIFF=$(git diff -r -p -m -M --full-index --staged | cat)
echo $DIFF | php test.php

In the command we could write something like this:

if (ftell(STDIN) === 0) {
    $diff = '';
    while (!feof(STDIN)) {
        $diff .= fread(STDIN, 1024);
    }
}

(This logic could also be placed in the ConsoleIO)

Next we'll have to change the logic of the PreCommitCommand::getCommittedFiles() so that the diff from the user input is being used when it's present. When no diff is present in the STDIN, it will fall back to the logic that is allready present in the library.

(This logic could also be placed in the ChangedFiles locator or in a separate ChangedFilesFromInputDiff locator)

I think this is the most elegant solution we can implement. What do you think about this?