phpro / grumphp

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

Pre commit hook showing entire git diff and no errors when there are #992

Closed pottink closed 2 years ago

pottink commented 2 years ago

image

When I execute my pre-commit-hook to run the GrumPHP tests, it always shows me all the git diff output and if there are errors, it just quits with no message or errors shown.

Anyone else having this issue?

When I manually run the GrumPHP it does everything as wanted.

PHP: 8.0.12 GrumPHP Shim: 1.8.1

pre-commit:

#!/bin/sh

#
# Run the hook command.
# Note: this will be replaced by the real command during copy.
#

# Fetch the GIT diff and format it as command input:
DIFF=$(git -c diff.mnemonicprefix=false -c diff.noprefix=false --no-pager diff -r -p -m -M --full-index --no-color --staged | cat)

# Grumphp env vars

export GRUMPHP_GIT_WORKING_DIR="$(git rev-parse --show-toplevel)"

# Run GrumPHP
(cd "./" && printf "%s\n" "${DIFF}" | kevin app 'vendor/phpro/grumphp-shim/grumphp' 'git:pre-commit' '--skip-success-output')
veewee commented 2 years ago

Spent the afternoon investigating this issue - since members on our team have similar issues. The error occurs on docker-compose v2.3.3 It works on docker-compose 2.2.1 So the error must be introduced somewhere in between

In docker-compose v2.2.3, This PR got merged: Do not try to guess when to allocate a TTY and keep it as default Before, it tries guessing wether the TTY should be enabled or not. After this PR, TTY is enabled by default and you need to explicitly disable it. Since GrumPHP passes in the diff through STDIN, adding the -T option to docker-comoser run is required.

So the fix is to change the command to something like:

EXEC_GRUMPHP_COMMAND: docker-compose run -T --rm --no-deps application

I've added this info to the docs as well: https://github.com/phpro/grumphp/blob/master/doc/parameters.md -> git_hook_variables


If that does not work, these might work for you as a temporary solution:

garciasdos commented 2 years ago

Same error happening here, solved with the temporary solution provided by @veewee. But hoping this is fixed in a future version 🙏🏼

veewee commented 2 years ago

@garciasdos The problem is not in GrumPHP itself, but in docker-compose. The permanent solution is to use the -T flag inside the commit hook. Been using this solution since previous week without any issues.

EXEC_GRUMPHP_COMMAND: docker-compose run -T [YOUROTHERPARAMS]

(when changing this env var, you'll need to rerun grumphp git:init in order for the changes to be written to the hook file)


As mentioned above: previously docker-compose did a check if it is running in a TTY console or not. This check got removed. If you are using some custom tooling like @pottink's kevin, you can add a check to see if your running in a TTY or pipe yourself. In bash, this would look like this:

USETTY=""

# Check if stdin is attached a pipe or a redirect
if [[ -p /dev/stdin ]] || [[ ! -t 0 && ! -p /dev/stdin ]]; then
  USETTY="-T"
fi

# Check if stdout is attached a pipe or a redirect
if [[ -p /dev/stdout ]] || [[ ! -t 1 && ! -p /dev/stdout ]]; then
  USETTY="-T"
fi

docker-compose run $USETTY --rm "${APP_NAME}" "$@"