Change || to && for whether MSYS is a tty
#24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The MSYS terminal appears to create pipes with the name `msys-` but *don't* contain `-pty`, so this changes the `||` logic to `&&` to ensure that the logic here doesn't fire for MSYS terminals where piping is used to erroneously diagnose a stdio stream as a tty. Closes softprops#23
|
Has someone tested this change in an MSYS terminal on Windows? Does detection still work as expected? |
|
(I can test it later today if not.) |
|
Oh certainly! I've tested this locally in an MSYS2 shell at least: |
|
thanks folks |
|
pushed these changes up in a new release on crates.io |
BurntSushi
added a commit
to BurntSushi/atty
that referenced
this pull request
Apr 21, 2018
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`).
Merged
|
I think this PR caused a regression in cygwin terminals. I sat down and thought about it for a bit, and submitted a patch in #25 that I think should fix the regression and, I suspect, also keep the fix from this PR as well. |
BurntSushi
added a commit
to BurntSushi/atty
that referenced
this pull request
Apr 21, 2018
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`).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The MSYS terminal appears to create pipes with the name
msys-but don'tcontain
-pty, so this changes the||logic to&&to ensure that the logichere doesn't fire for MSYS terminals where piping is used to erroneously
diagnose a stdio stream as a tty.
Closes #23