Only perform checks on staged hunks

[Miljar]

I have been using GrumPHP for a while now, and one of the things that bothers me is that checks are performed on the entire file that is staged, rather than on the changes in that file.

Practical example: PHPCS reporting errors in parts of the code which are not touched in that commit.

An improvement would be to do the checks on the staged version of a file, and not on the working directory version. I have an example PHPCS pre-commit hook below, where this magic is demonstrated. Could this be incorporated in GrumPHP?

#!/bin/bash
# PHP CodeSniffer pre-commit hook for git
#
# @author Soenke Ruempler <soenke@ruempler.eu>
# @author Sebastian Kaspari <s.kaspari@googlemail.com>
#
# see the README

PHPCS_BIN=/usr/bin/phpcs
PHPCS_CODING_STANDARD=PEAR
PHPCS_IGNORE=
TMP_STAGING=".tmp_staging"

# parse config
CONFIG_FILE=$(dirname $0)/config
if [ -e $CONFIG_FILE ]; then
    . $CONFIG_FILE
fi

# simple check if code sniffer is set up correctly
if [ ! -x $PHPCS_BIN ]; then
    echo "PHP CodeSniffer bin not found or executable -> $PHPCS_BIN"
    exit 1
fi

# stolen from template file
if git rev-parse --verify HEAD
then
    against=HEAD
else
    # Initial commit: diff against an empty tree object
    against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

# this is the magic: 
# retrieve all files in staging area that are added, modified or renamed
# but no deletions etc
FILES=$(git diff-index --name-only --cached --diff-filter=ACMR $against -- )

if [ "$FILES" == "" ]; then
    exit 0
fi

# create temporary copy of staging area
if [ -e $TMP_STAGING ]; then
    rm -rf $TMP_STAGING
fi
mkdir $TMP_STAGING

# match files against whitelist
FILES_TO_CHECK=""
for FILE in $FILES
do
    echo "$FILE" | egrep -q "$PHPCS_FILE_PATTERN"
    RETVAL=$?
    if [ "$RETVAL" -eq "0" ]
    then
        FILES_TO_CHECK="$FILES_TO_CHECK $FILE"
    fi
done

if [ "$FILES_TO_CHECK" == "" ]; then
    exit 0
fi

# execute the code sniffer
if [ "$PHPCS_IGNORE" != "" ]; then
    IGNORE="--ignore=$PHPCS_IGNORE"
else
    IGNORE=""
fi

# Copy contents of staged version of files to temporary staging area
# because we only want the staged version that will be commited and not
# the version in the working directory
STAGED_FILES=""
for FILE in $FILES_TO_CHECK
do
  ID=$(git diff-index --cached HEAD $FILE | cut -d " " -f4)

  # create staged version of file in temporary staging area with the same
  # path as the original file so that the phpcs ignore filters can be applied
  mkdir -p "$TMP_STAGING/$(dirname $FILE)"
  git cat-file blob $ID > "$TMP_STAGING/$FILE"
  STAGED_FILES="$STAGED_FILES $TMP_STAGING/$FILE"
done

OUTPUT=$($PHPCS_BIN -n -s --standard=$PHPCS_CODING_STANDARD $IGNORE $TMP_STAGING)
#OUTPUT=$($PHPCS_BIN -n -s --standard=$PHPCS_CODING_STANDARD $IGNORE $STAGED_FILES)
RETVAL=$?

# delete temporary copy of staging area
rm -rf $TMP_STAGING

if [ $RETVAL -ne 0 ]; then
    echo "$OUTPUT" | less
fi

exit $RETVAL

[Miljar]

This does not solve the problem actually, but at least code that is not yet staged would not be checked, as it is not part of the staged version of the given file.

[veewee]

This feature has recently been developed, but hasn't been tagged yet.
Can you try this again with the current dev-master?
You can find more information on this issue in the tickets: #100, #103

[veewee]

@Miljar
This has been released in https://github.com/phpro/grumphp/releases/tag/v0.8.0
Can you verify this works for you?

[Miljar]

I have updated to v0.8.0 but I receive errors when I try to test this:

Reapplying unstaged changes from stash.
The stashed changes could not be applied. Please run `git stash pop` manually!More info: exception 'Gitonomy\Git\Exception\ProcessException' with message 'Error while running git command:
'git' '--git-dir' '/path/to/my/project/.git' '--work-tree' '/path/to/my/project' 'stash' 'pop' '--quiet'

In my source code, I then get git markers for merge conflicts:

<<<<<<< Updated upstream
=======
    }

    public function tralala()
    {
>>>>>>> Stashed changes

This is how I tested:

• Added file in git (add & commit)
• updated file: wrote method which is supposed to pass PHPCS and one which should not pass it
• git add -p and only add the hunks for the passing method
• git commit -m "Test commit"

[veewee]

We didn't really test git addin patch mode.
After some investigation it turns out that this one will always fail.
Looks like an issue in git:

For example:

# create the file:
echo "<?php" > test.php
git add test.php
echo 'echo "stage me";' >> test.php
echo 'echo "unstage me";' >> test.php

# Run patch add:
git add -p test.php
# edit the hunk and delete the unstage me line

# You can check what is staged and what is not:
git status
git diff --cached
git diff

# Create stash:
git stash save --keep-index
git diff --cached
git stash pop

Result:

Auto-merging test.php
CONFLICT (add/add): Merge conflict in test.php

<?php
echo "stage me";
<<<<<<< Updated upstream
=======
echo "unstage me";
>>>>>>> Stashed changes

As you can see, this will always result in an error in GIT.
Maybe this is something that needs to be reported in GIT instead of fixed in GrumPHP?
What do you think abou this @aderuwe ?

[Miljar]

Why mess with my workspace? When stashing, the checks will still happen on new unstaged files, since these are not stashed.
Another solution would be to checkout the changes in another folder and perform the checks on those files, like in the script from my original post:

# this is the magic: 
# retrieve all files in staging area that are added, modified or renamed
# but no deletions etc
FILES=$(git diff-index --name-only --cached --diff-filter=ACMR $against -- )

Then loop over those files and check those files against the checkers.

[veewee]

No, only the staged files are being validated. This could also be fixed with the stash --all flag, but isn't necesarry at the moment.

A copy is another option, but there is much more that can go wrong inside of GrumPHP.
In very big projects the copy might also be slow.

[veewee]

Did some additional research last night and found the following discussion:
http://git.661346.n2.nabble.com/stash-refuses-to-pop-td7453780.html

So basically the patched stash can only be applied again when the staged changes are committed.

[Miljar]

@veewee I have thought about it, and I don't think that the current solution is good enough. At the very least, there should be an option to turn it off, because I don't like it when tools mess with my workspace by stash popping & pushing. If something goes wrong, who knows what I have to do to restore the situation.

You worry that in big projects the copying of the files will be slow. I can only see that as a problem when you try to commit a lot of big blobs. Like a folder with a database dump or a folder with images. I have not really tested this situation so I cannot confirm nor deny this. However, you could make this smart by allowing to add a global ignore pattern for GrumPHP where you could add *.jpg, *.sql, *.json, *.bson and such. Only files not matching the patterns should be checked.

[veewee]

@Miljar
That was also our concern while developing this feature. In the end we decided to turn it on by default, but maybe it was a better idea to turn it off.
This feature is configurable. You can turn it of with the ignore_unstaged_changes parameter.
More information: https://github.com/phpro/grumphp/blob/master/doc/parameters.md

About the copy: we will certainly have to look into that one. My biggest concern is how GrumPHP will handle these files / tasks when they run in a separate folder. So mostly about: autoloading, relative paths, location of the tools, location of the vendor libraries, .... But then again: this needs to be investigated first.

[Miljar]

It's a tough one :)
For stuff like linters and phpcs, phpmd, the copy approach should work fairly well, without many problems.
But things like phpunit/behat/phpspec would indeed be more difficult because of the autoloading and dependencies on vendors.

[veewee]

We changed the default value of ignore_unstaged_changes to false in PR #135. This way the stash functionallity isn't triggered by default. You can enable it in a local grumphp.yml file and use it with care. Currently there is no other solution for the problem we found above, so I will close this one until we find a better solution.