Re: [PATCH 0/6] echo usernames as they are typed
From: Jeff King <hidden>
Date: 2016-06-15 22:52:32
On Sun, Nov 27, 2011 at 10:17:26AM +0100, Erik Faye-Lund wrote:
quoted
And here's something I've been meaning to do on top: actually echo characters at the username prompt. We can't do this portably, but we can at least stub out a compatibility layer and let each system do something sensible.Interesting, I've been working on something pretty similar: getting rid of getpass usage all together: https://github.com/kusma/git/tree/work/askpass My reason to write a getpass replacement was to avoid capping input to PASS_MAX, which can be as low as 8 characters (and AFAIK is just that on Solaris)...
Yeah, if there are really bad getpass implementations, we would want to work around them. If we are going to do so, it might make sense to combine the effort with my getpass_echo wrapper, as they are really the same function, modulo tweaking the echo settings. It would also be nice to make getpass a little more predictable. If /dev/tty can't be opened, glibc's getpass will fall back to writing the prompt to stderr and reading the password from stdin. But we definitely don't want to do that in git-remote-curl, where stdin is already talking a special protocol with the parent fetch process. So I think it might be best to just write our own getpass. However, your implementation looks wrong to me:
quoted hunk ↗ jump to hunk
diff --git a/password.c b/password.c new file mode 100644 index 0000000..727ed84 --- /dev/null +++ b/password.c@@ -0,0 +1,94 @@ +#include "cache.h" +#include "git-compat-util.h" +#include "strbuf.h" +#include "run-command.h" + +#ifndef WIN32 + +static void ask_password(struct strbuf *sb, const char *prompt) +{ + struct termios attr; + tcflag_t c_lflag; + + if (tcgetattr(1, &attr) < 0) + die("tcgetattr failed!"); + c_lflag = attr.c_lflag;
This is getting the terminal attributes for stdout. But in many cases, stdout will not be connected to the terminal (in particular, remote-curl, as I mentioned above, will have its stdio connected to the parent fetch process). Stderr is a better guess, as you do here:
+ fputs(prompt, stderr);
but even that is not foolproof. With getpass(), this should work: git clone ... 2>errors with the prompt going to the terminal. But it doesn't with your patch. You really want to open "/dev/tty" on most Unix systems (which is what getpass() does). I have no idea what would be appropriate on Windows.
+ for (;;) {
+ int c = getchar();
+ if (c == EOF || c == '\n')
+ break;
+ strbuf_addch(sb, c);
+ }And this is even worse. You're reading from stdin, which will get whatever cruft is in the pipe coming from the parent process (or may even cause a hang, as the parent is probably blocking waiting to read from the child helper). -Peff