Skip to content

Commit

Permalink
fix pty detection
Browse files Browse the repository at this point in the history
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 softprops#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 softprops#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`).
  • Loading branch information
BurntSushi committed Apr 21, 2018
1 parent 4d460ba commit 55ab829
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,13 @@ unsafe fn msys_tty_on(fd: DWORD) -> bool {
.as_os_str()
.to_string_lossy()
.into_owned();
name.contains("msys-") && name.contains("-pty")
// This checks whether 'pty' exists in the file name, which indicates that
// a pseudo-terminal is attached. To mitigate against false positives
// (e.g., an actual file name that contains 'pty'), we also require that
// either the strings 'msys-' or 'cygwin-' are in the file name as well.)
let is_msys = name.contains("msys-") || name.contains("cygwin-");
let is_pty = name.contains("-pty");
is_msys && is_pty
}

/// returns true if this is a tty
Expand Down

0 comments on commit 55ab829

Please sign in to comment.