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

aliases.sh: allow pipe to work #414

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

temsa
Copy link

@temsa temsa commented May 2, 2022

After investigating for a while why this issue, by which I have been affected recently, happened, I found out that we could avoid this pipe problem.

this is how typically it materializes :

$ node -e 'console.log(666)'
666

$ node -e 'console.log(666)' | pino-pretty
stdout is not a tty

$ node.exe -e 'console.log(666)' | pino-pretty
666

We just need to use the undocumented switch from winpty to allow for the pipe to another command to actually work without getting into stdout is not a tty. It also still provides the value of winpty, e.g. for running an interactive shell

This PR just adds the switch, which should allow both of those commands to work in the intended manner:

$ node.exe -e 'console.log(666)' | pino-pretty
666

$ node -e 'console.log(666)' | pino-pretty
666

@temsa temsa force-pushed the better-aliases.sh branch from 4a06848 to 92c3dec Compare May 2, 2022 14:44
@temsa temsa force-pushed the better-aliases.sh branch from 92c3dec to 2d8eaff Compare May 2, 2022 14:54
@dscho
Copy link
Member

dscho commented May 2, 2022

Interesting! This switch was introduced in rprichard/winpty@222ecb9. The commit message suggests that those options might be pulled at any point, but then, they have been in winpty since 2016...

@dscho
Copy link
Member

dscho commented May 2, 2022

Seems that the -X options are still under consideration: rprichard/winpty#103. Might be worth reviving that discussion, as well as rprichard/winpty#101.

@temsa
Copy link
Author

temsa commented May 3, 2022

Moving instead to a wrapper script running winpty only when relevant could be an option too, like rprichard/winpty#101 (comment), but I've also asked if this option is here to stay

@rimrul
Copy link
Member

rimrul commented May 3, 2022

I don't think you'll get a response, active winpty development has seemingly stopped a few years ago.
A wrapper script is probably still the better solution, since it prevents running into the issue that is outlined in rprichard/winpty#103 (comment), i.e. programs that don't produce coloured output when piped to a file producing colour sequences when piped to a file in Git Bash.

@dscho
Copy link
Member

dscho commented May 3, 2022

A wrapper script is probably still the better solution

Since we're talking about a Bash alias, we could do the same in the alias itself, too, right?

@dscho
Copy link
Member

dscho commented May 6, 2022

Since we're talking about a Bash alias, we could do the same in the alias itself, too, right?

Seems that I was wrong about aliases: they cannot contain if-then-else blocks. But shell functions can:

$ unalias node

$ function node () { if test -t 0 -a -t 1; then winpty.exe node.exe "$@"; else node.exe "$@"; fi; }

$ node
Welcome to Node.js v16.15.0.
Type ".help" for more information.
> console.log("123")
123
undefined
> process.exit(0)

$ node -e 'console.log("123")' | cat
123

@dscho
Copy link
Member

dscho commented May 10, 2022

@temsa how about trying your hand at patching aliases.sh accordingly?

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

Successfully merging this pull request may close these issues.

3 participants