PreserveWorkingTree not working inside docker

CircleCode commented 2 years ago

reproduction

see https://github.com/CircleCode/test-captainhook/tree/main for a minimal example showing the bug.

context

run-mode: docker
plugin \\CaptainHook\\App\\Plugin\\Hook\\PreserveWorkingTree enabled
a file is both staged and locally modified

bug

when running git commit -m test, the hook fails, and

shows no output
local modifications are lost

investigations

this seems to be related to \SebastianFeldmann\Git\Operator\Status::restoreWorkingTree calling git -C '/app/subdir/..' checkout --quiet -- '.' which in turns calls the post-checkout hook, which tries to run docker… from inside docker, returning .git/hooks/post-checkout: 5: docker: Permission denied

note: I know this subdir files structure is weird, but it serves as an illustration for #168

sebastianfeldmann commented 2 years ago

If you use the composer installed version of CaptainHook just run a composer update this should update the SebastianFeldmann\Git dependency to version 3.8.2 which will deactivate hooks for the restore command.

If you are using the PHAR version let me know, then I will generate a new PHAR release as well.

CircleCode commented 2 years ago

I won't be able to test until tomorrow, but composer version is enough, thanks for the quick fix. I'll report tomorrow if this closes this issue

CircleCode commented 2 years ago

Okay, this fixes my bug, thus marking it as resolved.

However, I still have another problem, and I'm not sure it is related: the hook works now correctly and prevent commit, but I don't see any output, I only now the exit status is 1

Do you have any indication of what could be happening ?

sebastianfeldmann commented 2 years ago

Just to be sure that I get the issue at hand.

This is what's happening

You have a file staged and modified
You execute a commit
The Cap'n is triggered and executed via Docker
The plugin cleans the working directory (modifications to the file are gone)
The Cap'n executes the Linter action
Linter action fails
Plugin restores modifications (does not trigger the post-checkout hook anymore :)
Commit fails (no output)

Is that about right? Not really sure what the "no output" issue could be. Sadly I'm not a Docker expert so I'm not sure if Docker is causing this or not. I'll try and reproduce the issue with a host executed Cap'n and see what happens.

CircleCode commented 2 years ago

I tried to figure it out, but I cannot find any explanation.

I simplified the case (a single file, only changed in index), and it can now be observed in https://github.com/CircleCode/test-captainhook/tree/main.

the short version is:

running the pre-commit hook directly outputs the errors
commiting does not output anything

i.e. with the following script

#!/bin/bash

docker compose run --rm --no-deps app composer install -q

git restore --staged src
git restore src

cat << 'EOF' > ./src/test.php
<?php

$a =        "foo"
$b =        "bar";
EOF

git add ./src/test.php

echo "----------------------------------------"
echo "manually running the pre-commit hook"
(cd .. && .git/hooks/pre-commit)

echo "----------------------------------------"
echo "commit"
git commit -m 'test'

I get the following output

❯ bash test.sh

----------------------------------------
manually running the pre-commit hook
pre-commit: 
 - \CaptainHook\App\Hook\PHP\Action\Linting                          : failed

Linting failed: PHP syntax errors in 1 file(s)

✘ subdir/src/test.php

----------------------------------------
commit
❯

Notes:

if I echo "foo" directly in the hook, before invoking captainhook, "foo" is printed, but not the output from cpatianhook, so this is not simply git dropping the output, this is only applicable to output from captainhook inside docker inside pre-commit hook.
if I attach to the running container during the hook execution, the attached terminal receives the output, while it is still not printed in the hook

sebastianfeldmann commented 2 years ago

The next idea would be to not echo "foo" directly in the hook script but to create an "echo-foo.php"

<?php
echo "foo php";
exit(1);

and run this from the hook script instead of captainhook. To make sure it's not a general "PHP issue". If that works, then it seems to be related how the Symphony Console component is handling the output.

CircleCode commented 2 years ago

You put me on the right way: this is indeed something related to php, but not to php alone.

tl;dr: Maybe it can be a good idea to mention it in the docker mode documentation: when using docker compose run, you have to use the --no-TTY flag

details:

I refined the issue a lot, and finally found the solution thanks to this pre-commit hook:

#!/bin/sh

echo '[pre-commit] [host]    [shell] [stdout] foo'
>&2 echo '[pre-commit] [host]    [shell] [stderr] foo'
php -r 'echo("[pre-commit] [host]    [php]   [stdout] foo\n");'
php -r 'fwrite(STDOUT, "[pre-commit] [host]    [php]   [stdout] foo via fwrite\n");'
php -r 'fwrite(STDERR, "[pre-commit] [host]    [php]   [stderr] foo via fwrite\n");'

docker run --rm -v "$(pwd)/../:/app" -w '/app/subdir/' php:7.4-cli echo '[pre-commit] [docker]  [shell] [stdout] foo'
docker run --rm -v "$(pwd)/../:/app" -w '/app/subdir/' php:7.4-cli php -r 'echo("[pre-commit] [docker]  [php]   [stdout] foo\n");'
docker run --rm -v "$(pwd)/../:/app" -w '/app/subdir/' php:7.4-cli php -r 'fwrite(STDOUT, "[pre-commit] [docker]  [php]   [stdout] foo via fwrite\n");'
docker run --rm -v "$(pwd)/../:/app" -w '/app/subdir/' php:7.4-cli php -r 'fwrite(STDERR, "[pre-commit] [docker]  [php]   [stderr] foo via fwrite\n");'

docker compose -f subdir/docker-compose.yaml run -T --rm --no-deps php echo '[pre-commit] [compose] [shell] [stdout] foo'
docker compose -f subdir/docker-compose.yaml run --rm --no-deps php php -r 'echo("[pre-commit] [compose] [php]   [stdout] foo\n");'
docker compose -f subdir/docker-compose.yaml run --rm --no-deps php php -r 'fwrite(STDOUT, "[pre-commit] [compose] [php]   [stdout] foo via fwrite\n");'
docker compose -f subdir/docker-compose.yaml run --rm --no-deps php php -r 'fwrite(STDERR, "[pre-commit] [compose] [php]   [stderr] foo via fwrite\n");'

docker compose -f subdir/docker-compose.yaml run -T --rm --no-deps php echo '[pre-commit] [compose] [shell] [stdout] [no-tty] foo'
docker compose -f subdir/docker-compose.yaml run -T --rm --no-deps php php -r 'echo("[pre-commit] [compose] [php]   [stdout] [no-tty] foo\n");'
docker compose -f subdir/docker-compose.yaml run -T --rm --no-deps php php -r 'fwrite(STDOUT, "[pre-commit] [compose] [php]   [stdout] [no-tty] foo via fwrite\n");'
docker compose -f subdir/docker-compose.yaml run -T --rm --no-deps php php -r 'fwrite(STDERR, "[pre-commit] [compose] [php]   [stderr] [no-tty] foo via fwrite\n");'

The expected output is

[pre-commit] [host]    [shell] [stdout] foo
[pre-commit] [host]    [shell] [stderr] foo
[pre-commit] [host]    [php]   [stdout] foo
[pre-commit] [host]    [php]   [stdout] foo via fwrite
[pre-commit] [host]    [php]   [stderr] foo via fwrite
[pre-commit] [docker]  [shell] [stdout] foo
[pre-commit] [docker]  [php]   [stdout] foo
[pre-commit] [docker]  [php]   [stdout] foo via fwrite
[pre-commit] [docker]  [php]   [stderr] foo via fwrite
[pre-commit] [compose] [shell] [stdout] foo
[pre-commit] [compose] [php]   [stdout] foo
[pre-commit] [compose] [php]   [stdout] foo via fwrite
[pre-commit] [compose] [php]   [stderr] foo via fwrite
[pre-commit] [compose] [shell] [stdout] [no-tty] foo
[pre-commit] [compose] [php]   [stdout] [no-tty] foo
[pre-commit] [compose] [php]   [stdout] [no-tty] foo via fwrite
[pre-commit] [compose] [php]   [stderr] [no-tty] foo via fwrite

The resulting output is

[pre-commit] [host]    [shell] [stdout] foo
[pre-commit] [host]    [shell] [stderr] foo
[pre-commit] [host]    [php]   [stdout] foo
[pre-commit] [host]    [php]   [stdout] foo via fwrite
[pre-commit] [host]    [php]   [stderr] foo via fwrite
[pre-commit] [docker]  [shell] [stdout] foo
[pre-commit] [docker]  [php]   [stdout] foo
[pre-commit] [docker]  [php]   [stdout] foo via fwrite
[pre-commit] [docker]  [php]   [stderr] foo via fwrite
[pre-commit] [compose] [shell] [stdout] foo
[pre-commit] [compose] [shell] [stdout] [no-tty] foo
[pre-commit] [compose] [php]   [stdout] [no-tty] foo
[pre-commit] [compose] [php]   [stdout] [no-tty] foo via fwrite
[pre-commit] [compose] [php]   [stderr] [no-tty] foo via fwrite

i.e: docker compose runs php in TTY mode, and it seems that in this case, php does not output text. To be totally honest, I have no idea why, and I do not even know if this is expected, or an issue; and in this case, is it a php or a docker compose one?

CircleCode commented 2 years ago

for the record, now that I have the explanation, and the good keywords on google, it seems I am not the only one who have faced this problem, and this would not be a bug: according to https://stackoverflow.com/a/68943553, it is simply that git does not give an interactive terminal, so you have to indicate this to docker compose.

sebastianfeldmann commented 2 years ago

Funny how things come together, I just merged a pull request a couple of days ago where exactly this behavior was mentioned. I didn't fully understand it at that time but now it all makes sense.

https://github.com/captainhookphp/captainhook/pull/169

Thanks for the thorough investigation and explanation. I think I will make this topic even more prominent in the documentation, now that I have a better grasp of its severity.

Again, thanks a lot.

captainhookphp / captainhook