give docker a tty output when expecting color

[glehmann]

this makes the behavior more consistent with the system language
and would help the executable run in a docker container to
produce a colored output.

[glehmann]

I haven't looked at the tests (quite obviously!) — I'm waiting on a opinion on this change before taking care of that :-)

[asottile]

this won't work because one can enable color without a tty and docker will crash

[glehmann]

never heard of docker crashing because of that

I ran a few tests to be sure:

~/s/pre-commit (docker-tty|✔) $ docker run --tty --rm debian grep --color Debian /etc/issue 2>&1
Debian GNU/Linux 12 \n \l
~/s/pre-commit (docker-tty|✔) $ docker run --tty --rm debian grep --color Debian /etc/issue 2>&1 | cat
Debian GNU/Linux 12 \n \l

Debian is displayed in red in both cases. No crash.

~/s/pre-commit (docker-tty|✔) $ docker run --rm debian grep --color Debian /etc/issue 2>&1 | cat
Debian GNU/Linux 12 \n \l

no color in that case

and just to make sure that the | cat drops the tty ability:

~/s/pre-commit (docker-tty|✔) $ python -c "import sys; print(sys.stdout.isatty())" 2>&1 | cat
False
~/s/pre-commit (docker-tty|✔) $ python -c "import sys; print(sys.stdout.isatty())" 2>&1
True
~/s/pre-commit (docker-tty|✔) $ python -c "import sys; print(sys.stderr.isatty())" 2>&1 | cat
False
~/s/pre-commit (docker-tty|✔) $ python -c "import sys; print(sys.stderr.isatty())" 2>&1
True

I may be missing something though. Could you be more explicit on how docker will be crashing?

[asottile]

it drops the tty ability on stdout but not on stdin

[glehmann]

~/s/pre-commit (docker-tty|✔) $ echo | docker run --tty --rm debian grep --color Debian /etc/issue 2>&1 | cat
Debian GNU/Linux 12 \n \l

still no crash (but some color)

~/s/pre-commit (docker-tty|✔) $ echo | python -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1 | cat
False
False
False

and no tty anywhere

docker makes the program running in the container that there is a tty though

~/s/pre-commit (docker-tty|✔) $ echo | docker run --rm python python3 -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1 | cat
False
False
False
~/s/pre-commit (docker-tty|✔) $ docker run --rm python python3 -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1
False
False
False
~/s/pre-commit (docker-tty|✔) $ docker run --tty --rm python python3 -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1
True
True
True
~/s/pre-commit (docker-tty|✔) $ echo | docker run --rm python python3 -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1 | cat
False
False
False
~/s/pre-commit (docker-tty|✔) $ echo | docker run --tty --rm python python3 -c "import sys; print(sys.stdin.isatty()); print(sys.stdout.isatty()); print(sys.stderr.isatty()); " 2>&1 | cat
True
True
True

which is good to have some color. It may cause some problems with the programs that expect some interactivity, but I think pre-commit forbid that in the hooks, so it shouldn't matter much

[asottile]

hmm perhaps we can try this then. there's definitely easily reproducible cases where -i without a tty causes docker to halt, I recall some with --tty as well but I'll just revert this if there's a problem found later on. I don't personally use language: docker so if it breaks shrugs.

[glehmann]

I've been using it for two weeks and haven't seen any problem. Does it look good enough for you to merge it?

[asottile]

needs a test demonstrating your change

[asottile]

rather than use + it would probably be clearer if you used something like get_docker_user above -- maybe write a little function for that?

[asottile]

[glehmann]

Thanks!