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

From: Jeff King <peff@peff.net>
Date: Nov 28, 2011, 03:53 UTC
Message-ID: <20111128035321.GA15640@sigill.intra.peff.net>
In-Reply-To: <CABPQNSb09kxjvdvz2P-WqU2VtMspaaA8ujTNLJ4+MuCrV=2zsw@mail.gmail.com>

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

Show 13 quoted lines

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

Show 21 quoted lines

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

Show 6 quoted lines

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