Re: [PATCH 0/6] echo usernames as they are typed

On Mon, Nov 28, 2011 at 4:53 AM, Jeff King [off-list ref] wrote:

On Sun, Nov 27, 2011 at 10:17:26AM +0100, Erik Faye-Lund wrote:

quoted

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.

My thinking exactly ;)

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:

It probably is, yes. It was a very naive attempt ;)

quoted

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:

quoted

+     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).

Yes, you're right. Opening "/dev/tty" is much better. But what happens
for processes started by GUI applications (with no easily observable
tty, if any)? Does open simply fail? If so, is it desirable for us to
fail in that case?

I have no idea what would be appropriate on Windows.

It's pretty similar, but not exactly: CreateFile("CONIN$", ...) or
CreateFile("CONOUT$", ...), depending on if you want the read-handle
or the write-handle... I can probably cook up something a bit more
concrete, though.

But _getch() that we already use always reads from the console
(according to MSDN, I haven't actually tested this myself:
http://msdn.microsoft.com/en-us/library/078sfkak%28v=VS.80%29.aspx).
But I don't think this allows us to fail when no console is attached.
Question is, should we fail in such cases? Windows does have an API to
prompt for passwords in a GUI window... Perhaps fallbacking to those
are the way to go? Something like:

if (GetConsoleWindow()) {
	/*  normal console-stuff */
} else {
	/* call CredUIPromptForWindowsCredentials(...) instead */
}

This might be nicer towards GUI tools, but it requires us to
explicitly ask for a password (and possibly a username at the same
time)...

quoted

+     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).

Yeah, thanks for pointing these glitches out ;)