Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tty settings for git hooks do not match git on other platforms #2914

Closed
1 task done
asottile opened this issue Nov 27, 2020 · 17 comments
Closed
1 task done

tty settings for git hooks do not match git on other platforms #2914

asottile opened this issue Nov 27, 2020 · 17 comments
Labels

Comments

@asottile
Copy link

  • I was not able to find an open or closed issue matching what I'm seeing

Setup

  • Which version of Git for Windows are you using? Is it 32-bit or 64-bit?
$ git --version --build-options
git version 2.29.2.windows.2
cpu: x86_64
built from commit: 3464b98ce6803c98bf8fb34390cd150d66e4a0d3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
  • Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19041.630]
  • What options did you set as part of the installation? Or did you choose the
    defaults?
> cat "C:\Program Files\Git\etc\install-options.txt"
Editor Option: Nano
Custom Editor Path:
Default Branch Option:
Path Option: CmdTools
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Merge
Use Credential Manager: Core
Performance Tweaks FSCache: Enabled
Enable Symlinks: Enabled
  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

pretty bog standard, I've seen this on other installations on other machines as well

Details

  • Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

Git Bash + cmd.exe both exhibit the same behaviour

  1. create .git/hooks/pre-commit with these contents (may need python3 on posix)
#!/usr/bin/env python
import sys
print('sys.stdin.isatty()', sys.stdin.isatty())
print('sys.stdout.isatty()', sys.stdout.isatty())
print('sys.stderr.isatty()', sys.stderr.isatty())
sys.exit(1)
  1. chmod +x .git/hooks/pre-commit
  2. git commit --allow-empty -m foo
  • What did you expect to occur after running these commands?

Here's the output on posix:

$ git commit --allow-empty -m foo
sys.stdin.isatty() False
sys.stdout.isatty() True
sys.stderr.isatty() True
  • What actually happened instead?

Here's the output on windows:

> git commit --allow-empty -m foo
sys.stdin.isatty() True
sys.stdout.isatty() False
sys.stderr.isatty() True
  • If the problem was occurring with a specific repository, can you provide the
    URL to that repository to help us with testing?

N/A (a blank repository reproduces)


background on why this matters: I'm using the tty attribute of stdout to determine if I should produce ansi color escape sequences

I don't particularly care about the incorrect (?) stdin being a tty, I'm more concerned about stdout being correct

@rimrul
Copy link
Member

rimrul commented Nov 27, 2020

python

We don't ship Python, so you're probably using a Python for Windows from python.org or the Microsoft app store. And that's likely the cause of your issue. You'll probably need to use MSYS2 Python to get the desired behaviour. If you use a MSYS2 Python you probably want to disable the python alias you likely have.

@rimrul rimrul changed the title tty settings for git hooks do not match git on other platforms Python tty settings for git hooks do not match git on other platforms Nov 27, 2020
@asottile
Copy link
Author

surely you'll believe me if I use bash then:

#!/usr/bin/env bash
[ -t 0 ] && echo 'stdin is' || echo 'stdin is not'
[ -t 1 ] && echo 'stdout is' || echo 'stdout is not'
[ -t 2 ] && echo 'stderr is' || echo 'stderr is not'
exit 1
> git commit -m wat --allow-empty
stdin is not
stdout is not
stderr is

@asottile asottile changed the title Python tty settings for git hooks do not match git on other platforms tty settings for git hooks do not match git on other platforms Nov 27, 2020
@dscho
Copy link
Member

dscho commented Nov 27, 2020

Interesting.

First of all, that your idea was probably to ask isatty() whether we're currently connected to an interactive terminal, right?

This is the correct call to make on Linux, macOS and other Unix-y systems. However, on Windows, the semantics are slightly different. Just enough to confuse you:

_isatty returns a nonzero value if the descriptor is associated with a character device. Otherwise, _isatty returns 0.

Note how it talks about a character device? Win32 Consoles are character devices, but so is NUL (see also the commit message of cbb3f3c that goes into a bit more detail on that).

And NUL (via the Unix-y equivalent /dev/tty that is transmogrified automatically on Windows) is precisely what that no_stdin flag uses that is set by run_hook_ve().

Therefore, opening NUL and then calling isatty() on the file descriptor will return a non-zero value. Even if it is not an interactive console. Confusing, amirite?

Since Python (I assume you mean the version that can be installed via the Windows Store) seems to simply pass through to the isatty() function of the Operating System, instead of trying to match the Unix behavior of isatty() (as cbb3f3c tries to do), and since the Bash we use is the MSYS2 Bash, which very much does try to match Unix behavior, you see different results between the Bash and the Python version of the hook.

Git Bash + cmd.exe both exhibit the same behaviour

I actually see vastly different behavior in Git Bash than in Git CMD. In the following table, /Bash refers to the Bash version of the hook, and /Python refers to the Python version (run through the Windows Store version of Python):

handle Git Bash/Bash Git Bash/Python Git CMD/Bash Git CMD/Python
stdin False True False True
stdout True True False False
stderr True True True True

As you can see, the stdin behavior is well in line with my explanations above.

So what about that stdout issue? That is not a Bash vs Python issue, it is a Git Bash vs Git CMD issue.

Alas, hooks are executed with their stdout redirected to stderr. This is accomplished by calling dup2(). My guess is that this does not duplicate the underlying HANDLE faithfully enough for _isatty() to report the same value (you can see that it does report the expected value in the stderr line, which shows no difference between the different columns).

Further, I guess that the MSYS2 Bash tries a little harder and still figures out that the duplicated handle redirects to an interactive console. Hence the difference in the stdout row.

This is only a guess at this point, though, but I already spent more than half an hour to write up this explanation and need to go back to taking care of more pressing things. @asottile maybe you can continue the investigation at this point?

@dscho
Copy link
Member

dscho commented Nov 27, 2020

I'm using the tty attribute of stdout to determine if I should produce ansi color escape sequences

Maybe you need to do something more along the lines of calling _get_osfhandle() to get a HANDLE, and then use GetConsoleScreenBufferInfo() to see whether it is attached to a console. This example should be helpful.

@asottile
Copy link
Author

I'm actually using the python.org installers, the windows store ones are fraught with weird behaviours / edge cases that I'd rather not deal with until they get ironed out there

I still think there is a problem with git as redirection in cmd works correctly:

> python -c "import sys; print(sys.stdout.isatty()); print(sys.stderr.isatty())"
True
True

> python -c "import sys; print(sys.stdout.isatty()); print(sys.stderr.isatty())" 1>&2
True
True

> python -c "import sys; print(sys.stdout.isatty()); print(sys.stderr.isatty())" 2>&1
True
True

@fahad3344

This comment has been minimized.

@dscho
Copy link
Member

dscho commented Dec 1, 2020

I'm actually using the python.org installers

@asottile and those installers would be where? I hoped that my comment would inspire you a bit more to imitate the verbosity and to continue the investigation.

@asottile
Copy link
Author

asottile commented Dec 1, 2020

Ah, sorry I assumed clicking downloads on python.org would be obvious, my bad!

Here's the link to 3.9.0: https://www.python.org/downloads/release/python-390/ I'm using the Windows x86-64 executable installer though all the flavors source the bits from the same binary

@dscho
Copy link
Member

dscho commented Dec 2, 2020

Okay. I still consider the remainder of the investigation in your court.

@asottile
Copy link
Author

asottile commented Dec 2, 2020

@dscho what more would you like me to show you, normal process redirection works for cmd and isatty detection in python and it does not work for git-for-windows

@dscho
Copy link
Member

dscho commented Dec 2, 2020

What more would you like to have explained? I laid out why isatty() says what it says in an earlier reply. In a lot of detail, too.

@asottile
Copy link
Author

asottile commented Dec 2, 2020

as far as I understand from your explanation git-for-windows's method of redirection is incorrect which breaks isatty -- and that's the bug that needs fixing to resolve this issue

@asottile
Copy link
Author

asottile commented Dec 3, 2020

@dscho I'm not sure how we got off on the wrong foot but I'm trying to understand your perspective -- do you not think this is a bug? what part of this makes it not a bug -- there's a very clear behaviour difference between the platforms and even between normal executables on the same platform and in every comparison git-for-windows is the odd one out -- and again in case it isn't clear, I don't particularly care about the nul oddity, I'm only concerned about stdout.

As a maintainer myself I completely understand the want to strive for simplicity and prioritize the important things, but I can't for the life of me see why you're being so dismissive for something that (to me) is very clearly a bug.

I want to understand better your attitude so I can work with you instead of against you as it seems is happening right now

I think it's fair to say that you're not being a very empathetic maintainer and at best you're being dismissive and at worst you're being unwelcoming and directly disrespectful.

We all want to make the software better for everyone and the way you're treating a newcomer to your project seems very bad.

@dscho
Copy link
Member

dscho commented Aug 24, 2022

Closing as stale.

@dscho dscho closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2022
@asottile
Copy link
Author

this isn't stale -- it's still broken -- please reopen thanks!

@dscho
Copy link
Member

dscho commented Aug 24, 2022

@asottile I see that you offered objections that my explanation somehow does not explain the discrepancy between CMD and MinTTY.

But it does. In MinTTY, there is no Win32 Console to play with. Not unless you use winpty as suggested in the Known Issues section of Git for Windows' release notes.

@asottile
Copy link
Author

no that doesn't explain why stderr behaves correctly and stdout does not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants