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

[Jeff King <peff@peff.net>]

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

> > 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:

> 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