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

use /dev/tty instead of /dev/stdout when prompting for information #1225

Closed
nhooyr opened this issue Aug 11, 2016 · 9 comments
Closed

use /dev/tty instead of /dev/stdout when prompting for information #1225

nhooyr opened this issue Aug 11, 2016 · 9 comments

Comments

@nhooyr
Copy link

nhooyr commented Aug 11, 2016

See neovim/neovim#5211

@mislav
Copy link
Owner

mislav commented Aug 11, 2016

Thanks, that is good advice! I'll have to look into how to implement this change in Go. To clear things up, which prompts from hub are you talking about?

@nhooyr
Copy link
Author

nhooyr commented Aug 11, 2016

At the very start, when it asks for the username/password for the token. https://github.com/github/hub/blob/af72087bc7c4f94cf5876b22820fa9e4cf772700/github/config.go#L137-L138

@nhooyr
Copy link
Author

nhooyr commented Aug 11, 2016

It's a very simple change. Open /dev/tty when prompting, but if you cannot, exit with an error.

@nhooyr
Copy link
Author

nhooyr commented Aug 11, 2016

I'll just go ahead and create the PR.

@mislav
Copy link
Owner

mislav commented Aug 11, 2016

Here's where we use stdin in this context. Go doesn't expose anything like os.Tty, but I guess we could just manually open /dev/tty. On Windows, I presume this won't be available, so we will continue using /dev/stdin there.

There is also interesting mentions of "tty" here https://golang.org/pkg/syscall/#SysProcAttr

Should we also write out the prompt to /dev/tty as well, or just read from it?

@nhooyr
Copy link
Author

nhooyr commented Aug 11, 2016

Yes, we have to write it out to /dev/tty as well.

@nhooyr
Copy link
Author

nhooyr commented Aug 11, 2016

Sorry never mind, I've got to do something else. Will not be able to work on the PR.

@mislav
Copy link
Owner

mislav commented Aug 11, 2016

No problem. Thanks for suggesting this in the first place.

@nhooyr
Copy link
Author

nhooyr commented Nov 21, 2019

Going to close as it has been a long time and I'm not sure if this is relevant anymore.

@nhooyr nhooyr closed this as completed Nov 21, 2019
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 a pull request may close this issue.

2 participants