GitStaged filter takes the working tree version instead of the staged version

[aadmathijssen]

Describe the bug
When using GitStaged filter option, phpcs checks the staged files instead of all files (which is good). However, instead of checking the staged version of each file, the working tree version is checked, which might be different. When using the GitStaged filter as a pre-commit hook, you want the staged version to be checked and not the working tree version to avoid accidentally committing files with phpcs violations.

Steps to reproduce
Take the following PHP file:

<?php

function convertStringToInt(string $x): int
{
    return (integer) $x;
}

Add it to the git staging area:

git add /path/to/file.php

Running phpcs with the GitStaged filter correctly returns 1 error:

$ vendor/bin/phpcs --standard=PSR12 --filter=GitStaged .

FILE: /path/to/file.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 5 | ERROR | [x] Short form type keywords must be used. Found: (integer)
---------------------------------------------------------------------------

Now change the file as follows:

-    return (integer) $x;
+    return intval($x);

This fixes the issue in the working tree, but for some reason you might not want to commit this change yet (for instance because you want to test the performance impact of using intval instead of casting).

Now when I run phpcs with the GitStaged filter it returns 0 errors:

$ vendor/bin/phpcs --standard=PSR12 --filter=GitStaged .

This is unexpected, because I am not committing the version in my working tree but in staging.

The same happens when I delete the file in my working tree:

$ rm /path/to/file.php
$ vendor/bin/phpcs --standard=PSR12 --filter=GitStaged .

Now the file that I am about to commit is not taken into account in the analysis by the phpcs tool.

Expected behavior

A would have expected the phpcs checks to be performed on the version in staging instead of the working tree version.

Versions:

• OS: any
• PHP: any
• PHPCS: 3.5.0 and up
• Standard: any

Additional context

This Github Gist provides a pre-commit hook that resolves this issue by creating a temporary working tree containing only the staged versions of the staged files and runs phpcs on that folder instead of the current working tree.

Possible solution using git stash

It seems to be possible to create a git pre-commit hook that employs the gitstaged filter while disregarding the changes in the working tree:

1. Stash all working directory changes including untracked files, except for the staging area:

git stash push --keep-index --include-untracked -m "phpcs pre-commit stash"

2. Run phpcs with the gitstaged filter and store the exit code in a variable:

phpcs --filter=GitStaged
result=$?

3. Restore to the previous configuration:

git reset --hard
git stash pop --index

4. Use the stored phpcs exit code as exit code.

exit ${result}

This particular use of git stash push, git reset and git stash pop avoids potential merge conflicts. See the following StackOverflow post for details: https://stackoverflow.com/questions/26379637/whats-the-index-option-for-git-stash-pop-for

[jrfnl]

@aadmathijssen Thanks for reporting this. This behaviour is by design. This was discussed extensively when the feature was added: #2137

[aadmathijssen]

Hi @jrfnl, thanks for the reply. I think it is best to document this somewhere; adding a section on the --filter option to the Advanced Usage wiki page looks like an appropriate place to me.

In the original PR I also found a reference to the diff-sniffer tool by @morozov, which seems to provide a solution to the issue I reported.

[aadmathijssen]

I think I found a way to create a git pre-commit hook that employs the GitStaged filter while disregarding the changes in the working tree. I included this solution at the end of the issue description.

I also created a GitHub gist for easier copy-pasting: https://gist.github.com/aadmathijssen/e539b808a5da9a7e8f74244610554111

[jrfnl]

@aadmathijssen That looks about right to me. This was one of the things discussed in the original ticket. It is, however, not currently possible to do this from within PHPCS as there is no "undo changes made by a filter after a run" functionality/hook.

All the same, the script in the gist is already much simpler than your original script, isn't it ? 😉

[gsherwood]

I'm going to close this issue as it looks like the conversation has resolved. Thanks @jrfnl.