phpro / grumphp

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

Too many arguments when space in the Git user name #883

Closed yguedidi closed 3 years ago

yguedidi commented 3 years ago
Q A
Version 1.3.1
Bug? yes
New feature? no
Question? no
Documentation? no
Related tickets N/A

It seems that GrumPHP is not working with git user name that contains a space.

Steps to reproduce:

# Set a git username with a space
mkdir tmp
cd tmp
git init
echo "vendor" > .gitignore
pbpaste > grumphp.yml
composer require --dev phpro/grumphp

# Your actions
# Please add the steps on how to reproduce the issue here.
git config --global user.name "Foo Bar"

# Run GrumPHP:
git add -A && git commit -m"Test"
# or
./vendor/bin/grumphp run

Result:


  Too many arguments, expected arguments "command" "commit-msg-file".  

git:commit-msg [--git-user GIT-USER] [--git-email GIT-EMAIL] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-c|--config CONFIG] [--] <command> <commit-msg-file>

exit status 1

Early investigation By adding var_dump($_SERVER['argv']); at the beginning of the grumphp file, we have:

array(6) {
  [0]=>
  string(32) "vendor/phpro/grumphp/bin/grumphp"
  [1]=>
  string(14) "git:commit-msg"
  [2]=>
  string(15) "--git-user='Foo"
  [3]=>
  string(4) "Bar'"
  [4]=>
  string(33) "--git-email='foo@bar.com'"
  [5]=>
  string(19) ".git/COMMIT_EDITMSG"
}

This explain the "Too many arguments" error.

Sadly I'm not an expert in shell, I tried to edit the .git/hooks/commit-msg file to play with the quoting, but without luck

veewee commented 3 years ago

That's a strange error. What hook are you using? Can you paste what the generated hook file looks like?

The regular hook should work. At least here it does with a space in it.

As you can see, it is escaped: https://github.com/phpro/grumphp/blob/master/resources/hooks/local/commit-msg#L20

yguedidi commented 3 years ago

Indeed it's strange.. I'm using that exact same hook, I'm on the latest released version 1.3.1, and I did run git:deinit and git:init to be sure that I have latest hooks. I tried on Bash and Zsh

veewee commented 3 years ago

Can you show your grumphp.yaml file and the content of the hook?

yguedidi commented 3 years ago

Sure!

grumphp.yaml:

grumphp:
    stop_on_failure: true
    process_timeout: 120
    ascii:
        failed: ~
        succeeded: ~
    fixer:
        enabled: false
    tasks:
        phpcs: ~
        phpcsfixer2:
            config: .php_cs
            diff: true

content of the hook (.git/hooks/commit-msg):

#!/bin/sh

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

GIT_USER=$(git config user.name)
GIT_EMAIL=$(git config user.email)
COMMIT_MSG_FILE=$1

# 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}" | exec 'vendor/phpro/grumphp/bin/grumphp' 'git:commit-msg' "--git-user='$GIT_USER'" "--git-email='$GIT_EMAIL'" "$COMMIT_MSG_FILE")
veewee commented 3 years ago

@yguedidi, Are you still having this issue.

I made some time for investigating this since we had some similar issues in our docker setup. The issue in there was related to an entrypoint that did not escape input arguments: exec $@ instead of exec "$@".

To start debugging I added set -x on top of the hook. This displays the commands that are being executed. You could do the same in your commit-msg hook.

One thing I noticed is that this setup is adding additional quotes around the --git-user and --git-email argument. This results in Grumphp containing quotes for those specific fields.

Does changing these lines work for you?

GIT_USER="$(git config user.name)"
GIT_EMAIL="$(git config user.email)"

 .... --git-user="$GIT_USER" --git-email="$GIT_EMAIL" ....

I think the solution above is better than the one that is in place.

I've also created a PR here : https://github.com/phpro/grumphp/pull/903. This way you can test it out through composer:

composer config repositories.grumphp vcs https://github.com/veewee/grumphp.git
composer require phpro/grumphp:dev-improve-commit-msg
yguedidi commented 3 years ago

Hello @veewee, thank you for the time you spend investigating this!

I sadly still have the issue, even with your suggested changes :confused: I made them directly on the files.

Thanks to set +x, I can say that with the changes it shows:

vendor/phpro/grumphp/bin/grumphp git:commit-msg --git-user=Foo Bar --git-email=foo@bar.com .git/COMMIT_EDITMSG

so no escaping around the name. and with my var_dump($_SERVER['argv']); trick I get:

array(6) {
  [0]=>
  string(32) "vendor/phpro/grumphp/bin/grumphp"
  [1]=>
  string(14) "git:commit-msg"
  [2]=>
  string(18) "--git-user=Foo"
  [3]=>
  string(7) "Bar"
  [4]=>
  string(31) "--git-email=foo@bar.com"
  [5]=>
  string(19) ".git/COMMIT_EDITMSG"
}

I tried to change the run command to ... '--git-user="'"$GIT_USER"'"' '--git-email="'"$GIT_EMAIL"'"' ... in order to add double quotes around it and this output:

vendor/phpro/grumphp/bin/grumphp git:commit-msg --git-user="Foo Bar" --git-email="foo@bar.com" diff --git a/src/myfile.php b/src/myfile.php

and with var_dump I get:

array(6) {
  [0]=>
  string(32) "vendor/phpro/grumphp/bin/grumphp"
  [1]=>
  string(14) "git:commit-msg"
  [2]=>
  string(19) "--git-user="Foo"
  [3]=>
  string(8) "Bar""
  [4]=>
  string(33) "--git-email="foo@bar.com""
  [5]=>
  string(19) ".git/COMMIT_EDITMSG"
}

so the git user is still splitted in 2 parts... Same results by wrapping with single quotes with ... --git-user="'"$GIT_USER"'" --git-email="'"$GIT_EMAIL"'" ...

Maybe it's a PHP argument parsing issue in the end? In few days I'll completely reinstall my Ubuntu to install the new version, maybe it will end up being an issue with my current setup

yguedidi commented 3 years ago

@veewee I found the issue!! It was indeed due to my setup, something I completely forgot about... :sweat:

You're right about the issue! It's indeed something about escaping. I setup while ago a php shell script that wraps the normal php command by the Symfony CLI wrapper:

# ~/bin/php

#!/usr/bin/bash
symfony php $@

I updated it to be (adding double quotes around $@):

# ~/bin/php

#!/usr/bin/bash
symfony php "$@"

and now it works!! :tada:

I'm really sorry for all the time you spent investigating this issue that in the end comes from my special setup (that I forgot about :confused:) Thank you!

veewee commented 3 years ago

No worries @yguedidi, we were seeing similar issues with our docker setup at the same time. That was the reason why I investigated it.

Looks like you had exactly the same issue though - but whilst passing it to symfony's php version.

Glad we found it!