threads / discuss / 29107

[PATCHv2 0/7] getpass refactoring

Subject: [PATCHv2 0/7] getpass refactoring

## tl;dr

11 messages between Dec 8, 2011 and Dec 9, 2011.

replies: 10people: 3as markdown or json

Jeff King· Dec 8, 2011, 08:21 UTC · lore

This is a re-roll of the earlier "echo usernames as they are typed" series here:

  http://article.gmane.org/gmane.comp.version-control.git/185970

It turns out that getpass() isn't even really good at getting passwords. See patch 5/7 for details. This series replaces it entirely if you set HAVE_DEV_TTY (otherwise, we continue to fallback to getpass()).

  [1/7]: imap-send: avoid buffer overflow
  [2/7]: imap-send: don't check return value of git_getpass
  [3/7]: move git_getpass to its own source file
  [4/7]: refactor git_getpass into generic prompt function
  [5/7]: add generic terminal prompt function
  [6/7]: prompt: use git_terminal_prompt
  [7/7]: credential: use git_prompt instead of git_getpass
It should be applied on top of the jk/credentials topic.
-Peff
Jeff King· Dec 8, 2011, 08:23 UTC · re: Jeff King · lore

[PATCH 1/7] imap-send: avoid buffer overflow

We format the password prompt in an 80-character static buffer. It contains the remote host and username, so it's unlikely to overflow (or be exploitable by a remote attacker), but there's no reason not to be careful and use a strbuf.

Signed-off-by: Jeff King <peff@peff.net>
---
Just something I noticed while doing the cleanup in the next patch.
 imap-send.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index e1ad1a4..4c1e897 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1209,9 +1209,10 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 			goto bail;
 		}
 		if (!srvc->pass) {
-			char prompt[80];
-			sprintf(prompt, "Password (%s@%s): ", srvc->user, srvc->host);
-			arg = git_getpass(prompt);
+			struct strbuf prompt = STRBUF_INIT;
+			strbuf_addf(&prompt, "Password (%s@%s): ", srvc->user, srvc->host);
+			arg = git_getpass(prompt.buf);
+			strbuf_release(&prompt);
 			if (!arg) {
 				perror("getpass");
 				exit(1);
-- 
1.7.8.rc2.8.gf0f4f
Jeff King· Dec 8, 2011, 08:24 UTC · re: Jeff King · lore

[PATCH 2/7] imap-send: don't check return value of git_getpass

git_getpass will always die() if we weren't able to get input, so there's no point looking for NULL.

Signed-off-by: Jeff King <peff@peff.net>
---
Not a bug, but just useless code I noticed.
 imap-send.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 4c1e897..227253e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1213,10 +1213,6 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 			strbuf_addf(&prompt, "Password (%s@%s): ", srvc->user, srvc->host);
 			arg = git_getpass(prompt.buf);
 			strbuf_release(&prompt);
-			if (!arg) {
-				perror("getpass");
-				exit(1);
-			}
 			if (!*arg) {
 				fprintf(stderr, "Skipping account %s@%s, no password\n", srvc->user, srvc->host);
 				goto bail;
-- 
1.7.8.rc2.8.gf0f4f
Jeff King· Dec 8, 2011, 08:24 UTC · re: Jeff King · lore

[PATCH 3/7] move git_getpass to its own source file

This is currently in connect.c, but really has nothing to do with the git protocol itself. Let's make a new source file all about prompting the user, which will make it cleaner to refactor.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as patch 1 from the previous series.
 Makefile     |    2 ++
 cache.h      |    1 -
 connect.c    |   44 --------------------------------------------
 credential.c |    1 +
 imap-send.c  |    1 +
 prompt.c     |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 prompt.h     |    6 ++++++
 7 files changed, 58 insertions(+), 45 deletions(-)
 create mode 100644 prompt.c
 create mode 100644 prompt.h
diff --git a/Makefile b/Makefile
index a26f58a..b024e2f 100644
--- a/Makefile
+++ b/Makefile
@@ -563,6 +563,7 @@ LIB_H += parse-options.h
 LIB_H += patch-ids.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
+LIB_H += prompt.h
 LIB_H += quote.h
 LIB_H += reflog-walk.h
 LIB_H += refs.h
@@ -669,6 +670,7 @@ LIB_OBJS += pkt-line.o
 LIB_OBJS += preload-index.o
 LIB_OBJS += pretty.o
 LIB_OBJS += progress.o
+LIB_OBJS += prompt.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
diff --git a/cache.h b/cache.h
index 2e6ad36..f320c98 100644
--- a/cache.h
+++ b/cache.h
@@ -1024,7 +1024,6 @@ struct ref {
 extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
 
 #define CONNECT_VERBOSE       (1u << 0)
-extern char *git_getpass(const char *prompt);
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);
diff --git a/connect.c b/connect.c
index 51990fa..519e527 100644
--- a/connect.c
+++ b/connect.c
@@ -619,47 +619,3 @@ int finish_connect(struct child_process *conn)
 	free(conn);
 	return code;
 }
-
-char *git_getpass(const char *prompt)
-{
-	const char *askpass;
-	struct child_process pass;
-	const char *args[3];
-	static struct strbuf buffer = STRBUF_INIT;
-
-	askpass = getenv("GIT_ASKPASS");
-	if (!askpass)
-		askpass = askpass_program;
-	if (!askpass)
-		askpass = getenv("SSH_ASKPASS");
-	if (!askpass || !(*askpass)) {
-		char *result = getpass(prompt);
-		if (!result)
-			die_errno("Could not read password");
-		return result;
-	}
-
-	args[0] = askpass;
-	args[1]	= prompt;
-	args[2] = NULL;
-
-	memset(&pass, 0, sizeof(pass));
-	pass.argv = args;
-	pass.out = -1;
-
-	if (start_command(&pass))
-		exit(1);
-
-	strbuf_reset(&buffer);
-	if (strbuf_read(&buffer, pass.out, 20) < 0)
-		die("failed to read password from %s\n", askpass);
-
-	close(pass.out);
-
-	if (finish_command(&pass))
-		exit(1);
-
-	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
-
-	return buffer.buf;
-}
diff --git a/credential.c b/credential.c
index a17eafe..fbb7231 100644
--- a/credential.c
+++ b/credential.c
@@ -3,6 +3,7 @@
 #include "string-list.h"
 #include "run-command.h"
 #include "url.h"
+#include "prompt.h"
 
 void credential_init(struct credential *c)
 {
diff --git a/imap-send.c b/imap-send.c
index 227253e..43588e8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -25,6 +25,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "run-command.h"
+#include "prompt.h"
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #else
diff --git a/prompt.c b/prompt.c
new file mode 100644
index 0000000..42a1c9f
--- /dev/null
+++ b/prompt.c
@@ -0,0 +1,48 @@
+#include "cache.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "prompt.h"
+
+char *git_getpass(const char *prompt)
+{
+	const char *askpass;
+	struct child_process pass;
+	const char *args[3];
+	static struct strbuf buffer = STRBUF_INIT;
+
+	askpass = getenv("GIT_ASKPASS");
+	if (!askpass)
+		askpass = askpass_program;
+	if (!askpass)
+		askpass = getenv("SSH_ASKPASS");
+	if (!askpass || !(*askpass)) {
+		char *result = getpass(prompt);
+		if (!result)
+			die_errno("Could not read password");
+		return result;
+	}
+
+	args[0] = askpass;
+	args[1]	= prompt;
+	args[2] = NULL;
+
+	memset(&pass, 0, sizeof(pass));
+	pass.argv = args;
+	pass.out = -1;
+
+	if (start_command(&pass))
+		exit(1);
+
+	strbuf_reset(&buffer);
+	if (strbuf_read(&buffer, pass.out, 20) < 0)
+		die("failed to read password from %s\n", askpass);
+
+	close(pass.out);
+
+	if (finish_command(&pass))
+		exit(1);
+
+	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
+
+	return buffer.buf;
+}
diff --git a/prompt.h b/prompt.h
new file mode 100644
index 0000000..0fd7bd9
--- /dev/null
+++ b/prompt.h
@@ -0,0 +1,6 @@
+#ifndef PROMPT_H
+#define PROMPT_H
+
+char *git_getpass(const char *prompt);
+
+#endif /* PROMPT_H */
-- 
1.7.8.rc2.8.gf0f4f
Jeff King· Dec 8, 2011, 08:31 UTC · re: Jeff King · lore

[PATCH 4/7] refactor git_getpass into generic prompt function

This will allow callers to specify more options (e.g., leaving echo on). The original git_getpass becomes a slim wrapper around the new function.

Signed-off-by: Jeff King <peff@peff.net>
---
Similar to patch 2 from the previous series. Two big differences:
 1. The first series accidentally dropped the "die if we don't get a
    password" behavior during the refactor, but we want to keep it.
 2. The first series had a special "name" parameter just for generating
    error messages. This drops it in the name of simplicity, so error
    messages have gone from (assuming you don't have a tty):
      Could not read password: No such device or address
    to:
      Could not read 'Username for 'https://example.com': ': No such
      device or address
    which is verbose, yes, but contains a little more useful
    information. The formatting is rather unfortunate, but I don't think
    it's worth worrying too much about.
 prompt.c |   46 ++++++++++++++++++++++++++++++----------------
 prompt.h |    3 +++
 2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/prompt.c b/prompt.c
index 42a1c9f..2002644 100644
--- a/prompt.c
+++ b/prompt.c
@@ -3,26 +3,13 @@
 #include "strbuf.h"
 #include "prompt.h"
 
-char *git_getpass(const char *prompt)
+static char *do_askpass(const char *cmd, const char *prompt)
 {
-	const char *askpass;
 	struct child_process pass;
 	const char *args[3];
 	static struct strbuf buffer = STRBUF_INIT;
 
-	askpass = getenv("GIT_ASKPASS");
-	if (!askpass)
-		askpass = askpass_program;
-	if (!askpass)
-		askpass = getenv("SSH_ASKPASS");
-	if (!askpass || !(*askpass)) {
-		char *result = getpass(prompt);
-		if (!result)
-			die_errno("Could not read password");
-		return result;
-	}
-
-	args[0] = askpass;
+	args[0] = cmd;
 	args[1]	= prompt;
 	args[2] = NULL;
 
@@ -35,7 +22,7 @@
 
 	strbuf_reset(&buffer);
 	if (strbuf_read(&buffer, pass.out, 20) < 0)
-		die("failed to read password from %s\n", askpass);
+		die("failed to get '%s' from %s\n", prompt, cmd);
 
 	close(pass.out);
 
@@ -46,3 +33,30 @@
 
 	return buffer.buf;
 }
+
+char *git_prompt(const char *prompt, int flags)
+{
+	char *r;
+
+	if (flags & PROMPT_ASKPASS) {
+		const char *askpass;
+
+		askpass = getenv("GIT_ASKPASS");
+		if (!askpass)
+			askpass = askpass_program;
+		if (!askpass)
+			askpass = getenv("SSH_ASKPASS");
+		if (askpass && *askpass)
+			return do_askpass(askpass, prompt);
+	}
+
+	r = getpass(prompt);
+	if (!r)
+		die_errno("could not read '%s'", prompt);
+	return r;
+}
+
+char *git_getpass(const char *prompt)
+{
+	return git_prompt(prompt, PROMPT_ASKPASS);
+}
diff --git a/prompt.h b/prompt.h
index 0fd7bd9..9ab85a7 100644
--- a/prompt.h
+++ b/prompt.h
@@ -1,6 +1,9 @@
 #ifndef PROMPT_H
 #define PROMPT_H
 
+#define PROMPT_ASKPASS (1<<0)
+
+char *git_prompt(const char *prompt, int flags);
 char *git_getpass(const char *prompt);
 
 #endif /* PROMPT_H */
-- 
1.7.8.rc2.8.gf0f4f
Junio C Hamano· Dec 9, 2011, 23:58 UTC · re: Jeff King · lore

Re: [PATCH 4/7] refactor git_getpass into generic prompt function

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
>  2. The first series had a special "name" parameter just for generating
>     error messages. This drops it in the name of simplicity, so error
>     messages have gone from (assuming you don't have a tty):
>
>       Could not read password: No such device or address
>
>     to:
>
>       Could not read 'Username for 'https://example.com': ': No such
>       device or address
>
>     which is verbose, yes, but contains a little more useful
>     information. The formatting is rather unfortunate,...
It also would be unpleasant to i18n it, I suspect. 
> +	r = getpass(prompt);
> +	if (!r)
> +		die_errno("could not read '%s'", prompt);

Taking advantage of the "prompt-string"-ness of the message, this might be a cuter workaround:

    fatal: Password: <<could not be read>>

But I do not think it matters that much. Let's queue what you have, and work out these details in-tree.

Jeff King· Dec 8, 2011, 08:33 UTC · re: Jeff King · lore

[PATCH 5/7] add generic terminal prompt function

When we need to prompt the user for input interactively, we want to access their terminal directly. We can't rely on stdio because it may be connected to pipes or files, rather than the terminal. Instead, we use "getpass()", because it abstracts the idea of prompting and reading from the terminal. However, it has some problems:

  1. It never echoes the typed characters, which makes it OK
     for passwords but annoying for other input (like usernames).
  2. Some implementations of getpass() have an extremely
     small input buffer (e.g., Solaris 8 is reported to
     support only 8 characters).
  3. Some implementations of getpass() will fall back to
     reading from stdin (e.g., glibc). We explicitly don't
     want this, because our stdin may be connected to a pipe
     speaking a particular protocol, and reading will
     disrupt the protocol flow (e.g., the remote-curl
     helper).
  4. Some implementations of getpass() turn off signals, so
     that hitting "^C" on the terminal does not break out of
     the password prompt. This can be a mild annoyance.

Instead, let's provide an abstract "git_terminal_prompt" function that addresses these concerns. This patch includes an implementation based on /dev/tty, enabled by setting HAVE_DEV_TTY. The fallback is to use getpass() as before.

For now, only Linux enables this by default. People on other /dev/tty-enabled systems can submit patches to turn it on once they have tested it.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the interesting one. I suspect most Unixes will want to use
this, but I was conservative about enabling it. msysgit can just drop
its own function into compat/terminal.c, and the existing custom
getpass() implementation can just go away.
 Makefile          |   10 ++++++
 compat/terminal.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 compat/terminal.h |    6 ++++
 3 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 compat/terminal.c
 create mode 100644 compat/terminal.h
diff --git a/Makefile b/Makefile
index b024e2f..2a9bb3d 100644
--- a/Makefile
+++ b/Makefile
@@ -227,6 +227,9 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
+# user.
+#
 # Define GETTEXT_POISON if you are debugging the choice of strings marked
 # for translation.  In a GETTEXT_POISON build, you can turn all strings marked
 # for translation into gibberish by setting the GIT_GETTEXT_POISON variable
@@ -523,6 +526,7 @@ LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
 LIB_H += compat/mingw.h
 LIB_H += compat/obstack.h
+LIB_H += compat/terminal.h
 LIB_H += compat/win32/pthread.h
 LIB_H += compat/win32/syslog.h
 LIB_H += compat/win32/poll.h
@@ -610,6 +614,7 @@ LIB_OBJS += color.o
 LIB_OBJS += combine-diff.o
 LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
+LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
@@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
 	HAVE_PATHS_H = YesPlease
+	HAVE_DEV_TTY = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease
@@ -1639,6 +1645,10 @@ ifdef HAVE_PATHS_H
 	BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
 
+ifdef HAVE_DEV_TTY
+	BASIC_CFLAGS += -DHAVE_DEV_TTY
+endif
+
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
 	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif
diff --git a/compat/terminal.c b/compat/terminal.c
new file mode 100644
index 0000000..49f16ca
--- /dev/null
+++ b/compat/terminal.c
@@ -0,0 +1,81 @@
+#include "git-compat-util.h"
+#include "compat/terminal.h"
+#include "sigchain.h"
+#include "strbuf.h"
+
+#ifndef NO_DEV_TTY
+
+static int term_fd = -1;
+static struct termios old_term;
+
+static void restore_term(void)
+{
+	if (term_fd < 0)
+		return;
+
+	tcsetattr(term_fd, TCSAFLUSH, &old_term);
+	term_fd = -1;
+}
+
+static void restore_term_on_signal(int sig)
+{
+	restore_term();
+	sigchain_pop(sig);
+	raise(sig);
+}
+
+char *git_terminal_prompt(const char *prompt, int echo)
+{
+	static struct strbuf buf = STRBUF_INIT;
+	int r;
+	FILE *fh;
+
+	fh = fopen("/dev/tty", "w+");
+	if (!fh)
+		return NULL;
+
+	if (!echo) {
+		struct termios t;
+
+		if (tcgetattr(fileno(fh), &t) < 0) {
+			fclose(fh);
+			return NULL;
+		}
+
+		old_term = t;
+		term_fd = fileno(fh);
+		sigchain_push_common(restore_term_on_signal);
+
+		t.c_lflag &= ~ECHO;
+		if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
+			term_fd = -1;
+			fclose(fh);
+			return NULL;
+		}
+	}
+
+	fputs(prompt, fh);
+	fflush(fh);
+
+	r = strbuf_getline(&buf, fh, '\n');
+	if (!echo) {
+		putc('\n', fh);
+		fflush(fh);
+	}
+
+	restore_term();
+	fclose(fh);
+
+	if (r == EOF)
+		return NULL;
+	return buf.buf;
+}
+
+#else
+
+char *git_terminal_prompt(const char *prompt, int echo)
+{
+	return getpass(prompt);
+}
+
+#endif
diff --git a/compat/terminal.h b/compat/terminal.h
new file mode 100644
index 0000000..97db7cd
--- /dev/null
+++ b/compat/terminal.h
@@ -0,0 +1,6 @@
+#ifndef COMPAT_TERMINAL_H
+#define COMPAT_TERMINAL_H
+
+char *git_terminal_prompt(const char *prompt, int echo);
+
+#endif /* COMPAT_TERMINAL_H */
-- 
1.7.8.rc2.8.gf0f4f
Jakub Narebski· Dec 8, 2011, 21:48 UTC · re: Jeff King · lore

Re: [PATCH 5/7] add generic terminal prompt function

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> diff --git a/Makefile b/Makefile
> index b024e2f..2a9bb3d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -227,6 +227,9 @@ all::
> +# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
> +# user.
Show 6 quoted lines
> @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
>  	NO_STRLCPY = YesPlease
>  	NO_MKSTEMPS = YesPlease
>  	HAVE_PATHS_H = YesPlease
> +	HAVE_DEV_TTY = YesPlease
>  endif

Here you use HAVE_DEV_TTY (by the way, I wonder if it could be automatically detected by ./configure script)...

Show 12 quoted lines
> diff --git a/compat/terminal.c b/compat/terminal.c
> new file mode 100644
> index 0000000..49f16ca
> --- /dev/null
> +++ b/compat/terminal.c
> @@ -0,0 +1,81 @@
> +#include "git-compat-util.h"
> +#include "compat/terminal.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +
> +#ifndef NO_DEV_TTY
> +#endif
  +#endif /* NO_DEV_TTY */
...and here you have NO_DEV_TTY
-- 
Jakub Narębski
Jeff King· Dec 8, 2011, 21:52 UTC · re: Jakub Narebski · lore

Re: [PATCH 5/7] add generic terminal prompt function

On Thu, Dec 08, 2011 at 01:48:33PM -0800, Jakub Narebski wrote:
Show 11 quoted lines
> > @@ -833,6 +838,7 @@ ifeq ($(uname_S),Linux)
> >  	NO_STRLCPY = YesPlease
> >  	NO_MKSTEMPS = YesPlease
> >  	HAVE_PATHS_H = YesPlease
> > +	HAVE_DEV_TTY = YesPlease
> >  endif
> 
> Here you use HAVE_DEV_TTY (by the way, I wonder if it could be
> automatically detected by ./configure script)...
> [...]
> ...and here you have NO_DEV_TTY

Whoops. Thanks for catching. I converted it to NO_DEV_TTY, which would turn this code on by default (because I think _most_ platforms we use are going to want this), but then I decided to go the conservative route and let platforms opt into it.

And of course since my platform is the one that enables it, I didn't notice during my testing.

I'll fix it for the next re-roll.
-Peff
Jeff King· Dec 8, 2011, 08:33 UTC · re: Jeff King · lore

[PATCH 6/7] prompt: use git_terminal_prompt

Our custom implementation of git_terminal_prompt has many advantages over regular getpass(), as described in the prior commit.

This also lets us implement a PROMPT_ECHO flag for callers who want it.

Signed-off-by: Jeff King <peff@peff.net>
---
 prompt.c |    3 ++-
 prompt.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/prompt.c b/prompt.c
index 2002644..72ab9de 100644
--- a/prompt.c
+++ b/prompt.c
@@ -2,6 +2,7 @@
 #include "run-command.h"
 #include "strbuf.h"
 #include "prompt.h"
+#include "compat/terminal.h"
 
 static char *do_askpass(const char *cmd, const char *prompt)
 {
@@ -50,7 +51,7 @@
 			return do_askpass(askpass, prompt);
 	}
 
-	r = getpass(prompt);
+	r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
 	if (!r)
 		die_errno("could not read '%s'", prompt);
 	return r;
diff --git a/prompt.h b/prompt.h
index 9ab85a7..04f321a 100644
--- a/prompt.h
+++ b/prompt.h
@@ -2,6 +2,7 @@
 #define PROMPT_H
 
 #define PROMPT_ASKPASS (1<<0)
+#define PROMPT_ECHO    (1<<1)
 
 char *git_prompt(const char *prompt, int flags);
 char *git_getpass(const char *prompt);
-- 
1.7.8.rc2.8.gf0f4f
Jeff King· Dec 8, 2011, 08:33 UTC · re: Jeff King · lore

[PATCH 7/7] credential: use git_prompt instead of git_getpass

We use git_getpass to retrieve the username and password from the terminal. However, git_getpass will not echo the username as the user types. We can fix this by using the more generic git_prompt, which underlies git_getpass but lets us specify an "echo" option.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/credential.c b/credential.c
index fbb7231..62d1c56 100644
--- a/credential.c
+++ b/credential.c
@@ -109,7 +109,8 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 		strbuf_addf(out, "/%s", c->path);
 }
 
-static char *credential_ask_one(const char *what, struct credential *c)
+static char *credential_ask_one(const char *what, struct credential *c,
+				int flags)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct strbuf prompt = STRBUF_INIT;
@@ -121,11 +122,7 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 	else
 		strbuf_addf(&prompt, "%s: ", what);
 
-	/* FIXME: for usernames, we should do something less magical that
-	 * actually echoes the characters. However, we need to read from
-	 * /dev/tty and not stdio, which is not portable (but getpass will do
-	 * it for us). http.c uses the same workaround. */
-	r = git_getpass(prompt.buf);
+	r = git_prompt(prompt.buf, flags);
 
 	strbuf_release(&desc);
 	strbuf_release(&prompt);
@@ -135,9 +132,11 @@ static void credential_describe(struct credential *c, struct strbuf *out)
 static void credential_getpass(struct credential *c)
 {
 	if (!c->username)
-		c->username = credential_ask_one("Username", c);
+		c->username = credential_ask_one("Username", c,
+						 PROMPT_ASKPASS|PROMPT_ECHO);
 	if (!c->password)
-		c->password = credential_ask_one("Password", c);
+		c->password = credential_ask_one("Password", c,
+						 PROMPT_ASKPASS);
 }
 
 int credential_read(struct credential *c, FILE *fp)
-- 
1.7.8.rc2.8.gf0f4f

← back to recent threads