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

Move prompts and progress bars to stderr rather than stdout #2004

Merged
merged 1 commit into from Oct 8, 2021

Conversation

duxovni
Copy link
Contributor

@duxovni duxovni commented Oct 4, 2021

Having password prompts/etc on stdout is inconvenient in any context where the output is being piped to something other than a terminal. This change makes gopass more scripting-friendly, especially when using the age backend. It also removes a redundant password prompt message when the age backend asks for a password, since pinentry will display its own password prompt immediately afterward.

RELEASE_NOTES=[ENHANCEMENT] Move password prompts to stderr

Signed-off-by: Faye Duxovni duxovni@duxovni.org

Having password prompts/etc on stdout is inconvenient in any context where the output is being piped to something other than a terminal.  This change makes gopass more scripting-friendly, especially when using the age backend.  It also removes a redundant password prompt message when the age backend asks for a password, since pinentry will display its own password prompt immediately afterward.

RELEASE_NOTES=[ENHANCEMENT] Move password prompts to stderr

Signed-off-by: Faye Duxovni <duxovni@duxovni.org>
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like to have second pair of eyes on this.

@AnomalRoil
Copy link
Member

I'd love to have a bit more context to understand when the stdout output is bugging you @duxovni.
What are the typical commands you are using in your scripts and how are you using them for this to be an issue for you?

I'm using gopass through scripts only and I thought we had all our basis covered on that front. (Although I'm using the GPG backend, so maybe it's not the case with the age backend, granted.)

That being said, it seems that POSIX mandates anyway to print interactive stuff on stderr anyway see PS1 here:

PS1
Each time an interactive shell is ready to read a command, the value of this variable shall be subjected to parameter expansion and written to standard error.

So doing the same seems like a sensible thing to do.

But I'll have to revisit a bit the codebase to check if you've changed really every instances of such stdout prompts or not, so I'll take a couple days to review, I'm afraid!

@duxovni
Copy link
Contributor Author

duxovni commented Oct 6, 2021

I can believe that everything works fine for you with the GPG backend; the age backend is way worse in this regard. Even just gopass show gives me password prompts in the output. There's the redundant prompt I removed, and then all the prompts generated by the CLI pinentry fallback; in order to fix those, I also needed to fix all of the other termio prompts, which then required fixing the progress bars too. There may well be a lot of other prompts that are still on stdout, but this covers all the prompts that might come up during gopass show with the age backend, and then all the prompts that use the same codepaths as those prompts.

@dominikschulz
Copy link
Member

Sorry for the sub-par experience with the age backend. It was really only a proof of concept for me (mostly to verify that Filos initial API was suitable for us). Thanks for your help in improving that!

@duxovni
Copy link
Contributor Author

duxovni commented Oct 6, 2021

Oh, yeah, sorry that came off more critical than I intended! I really appreciate the work you've put in, it's great to have a usable pass-style password manager that doesn't need me to fiddle with GPG keys, and I'm looking forward to seeing this backend grow and develop further ❤️

@dominikschulz
Copy link
Member

Oh, no worries. That didn't come across too critical or anything. I just would love to spend more time on the age backend but couldn't get myself to do that so far.

I gave the changes a short try and I think we can merge this as is and then fix any other occurences as we go.

@dominikschulz
Copy link
Member

@AnomalRoil I don't want obsolete your plans but I think if you find other instances of us using Stdout incorrectly we could just have a follow up PR, ok?

@dominikschulz dominikschulz merged commit efa78b2 into gopasspw:master Oct 8, 2021
kpitt pushed a commit to kpitt/gopass that referenced this pull request Jul 21, 2022
…#2004)

Having password prompts/etc on stdout is inconvenient in any context where the output is being piped to something other than a terminal.  This change makes gopass more scripting-friendly, especially when using the age backend.  It also removes a redundant password prompt message when the age backend asks for a password, since pinentry will display its own password prompt immediately afterward.

RELEASE_NOTES=[ENHANCEMENT] Move password prompts to stderr

Signed-off-by: Faye Duxovni <duxovni@duxovni.org>
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.

None yet

3 participants