softprops / atty

are you or are you not a tty?
MIT License
274 stars 51 forks source link

fix pty detection #25

Closed BurntSushi closed 6 years ago

BurntSushi commented 6 years ago

This commit fixes a recent regression in pty detection for the cygwin terminal. In particular, in 0.2.6, things worked:

$ git checkout 0.2.6
$ cargo run --example atty
stdout? true
stderr? true
stdin? true

But in 0.2.7, things stopped working:

$ git checkout 0.2.7
$ cargo run --example atty
stdout? false
stderr? false
stdin? false

Namely, all three of these should have returned true. The specific reason why this stopped working was because #24 introduced a change to the detection logic where both the 'msys' and 'pty' strings needed to be in the file descriptor's name, where the previous logic merely required either of them. It turns out that both were wrong. For example, in my cygwin terminal, the name of the stdout/stderr file descriptors is (from the above example):

\\cygwin-c5e39b7a9d22bafb-pty2-to-master

and stdin is:

\\cygwin-c5e39b7a9d22bafb-pty2-from-master

This makes it clear why the regression happened, since this string only contains pty but not msys.

If I run

echo foo | cargo run --example atty

then the stdin file name is \\cygwin-c5e39b7a9d22bafb-8728-pipe-0x5. My guess is that #24 fixes the case when the stdin file name is something like \\msys-c5e39b7a9d22bafb-8728-pipe-0x5, where the previous logic would have falsely reported that as a tty on stdin.

In this commit, we change the logic to require the pty substring, and additionally require either the cygwin or msys strings. The latter check is strictly to reduce the rate of false positives (in the case that an actual file name contains the substring pty).

BurntSushi commented 6 years ago

cc @alexcrichton

softprops commented 6 years ago

awesome thanks @BurntSushi!

softprops commented 6 years ago

published a in 0.2.9. 👍