diff mbox series

[1/2] prompt.c: split up the password and non-password handling

Message ID patch-1.2-ce5bea43f03-20211102T155046Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series prompt.c: split up git_prompt(), read from /dev/tty, not STDIN | expand

Commit Message

Ævar Arnfjörð Bjarmason Nov. 2, 2021, 4:48 p.m. UTC 
Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()
let's split it up into git_prompt_echo() and git_prompt_noecho()
functions, the latter only ever needs to be called from credential.c,
and the same goes for the previous password-only codepath in
git_prompt(), which is now exposed as a git_prompt_password().

Doing this paves the way towards addressing some of the issues brought
up in 97387c8bdd9 (am: read interactive input from stdin, 2019-05-20).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                 |  1 +
 builtin/bisect--helper.c |  5 ++-
 credential.c             | 17 ++++++----
 help.c                   |  2 +-
 prompt-password.c        | 63 ++++++++++++++++++++++++++++++++++
 prompt-password.h        |  7 ++++
 prompt.c                 | 73 ++++++----------------------------------
 prompt.h                 |  7 ++--
 8 files changed, 97 insertions(+), 78 deletions(-)
 create mode 100644 prompt-password.c
 create mode 100644 prompt-password.h

Comments

Jeff King Nov. 3, 2021, 11:53 a.m. UTC | #1 
On Tue, Nov 02, 2021 at 05:48:09PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Rather than pass PROMPT_ASKPASS and PROMPT_ECHO flags to git_prompt()
> let's split it up into git_prompt_echo() and git_prompt_noecho()
> functions, the latter only ever needs to be called from credential.c,
> and the same goes for the previous password-only codepath in
> git_prompt(), which is now exposed as a git_prompt_password().

I'm pretty "meh" on this direction. Moving from a flag field to separate
functions means a potential combinatoric explosion if new callers are
added later, or new flags are added.

And callers get more awkward, as the choices are no longer expressed
as data, so you need new conditionals like:

> -	r = git_prompt(prompt.buf, flags);
> +	/* We'll try "askpass" for both usernames and passwords */
> +	r = git_prompt_askpass(prompt.buf);
> +	if (!r)
> +		r = password ? git_prompt_noecho(prompt.buf)
> +			     : git_prompt_echo(prompt.buf);

And we lose the human-readable names for the flags in favor of ad-hoc
booleans:

> @@ -192,11 +197,9 @@ static char *credential_ask_one(const char *what, struct credential *c,
>  static void credential_getpass(struct credential *c)
>  {
>  	if (!c->username)
> -		c->username = credential_ask_one("Username", c,
> -						 PROMPT_ASKPASS|PROMPT_ECHO);
> +		c->username = credential_ask_one(c, 0);
>  	if (!c->password)
> -		c->password = credential_ask_one("Password", c,
> -						 PROMPT_ASKPASS);
> +		c->password = credential_ask_one(c, 1);

The only thing I do like here is that askpass can be split into its own
function (as in the first hunk above), and callers can simply decide
whether they need to go on to prompt at the terminal. But even there it
feels like closing a potential door around PROMPT_ECHO. The standard
askpass interface doesn't have a way for us to ask for it to show the
contents to the user (which is useful for the username prompt). But it
would be a reasonable extension to add one. Say, a GIT_ASKPASS_ECHO
that, if set, is preferred for the username prompt.

So I dunno. This seems somewhere between churn and making things worse,
and I don't see what it's buying us at all.

-Peff
Junio C Hamano Nov. 3, 2021, 5:28 p.m. UTC | #2 
Jeff King <peff@peff.net> writes:

> So I dunno. This seems somewhere between churn and making things worse,
> and I don't see what it's buying us at all.

Unfortunately I have to agree with the summary.  Not a material I
want to spend time on this time in the cycle anyway X-<.

Thanks for reading.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 12be39ac497..e8815860e22 100644
--- a/Makefile
+++ b/Makefile
@@ -967,6 +967,7 @@  LIB_OBJS += pretty.o
 LIB_OBJS += prio-queue.o
 LIB_OBJS += progress.o
 LIB_OBJS += promisor-remote.o
+LIB_OBJS += prompt-password.o
 LIB_OBJS += prompt.o
 LIB_OBJS += protocol.o
 LIB_OBJS += protocol-caps.o
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a5750..30533a70b53 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -370,7 +370,7 @@  static int decide_next(const struct bisect_terms *terms,
 		 * translation. The program will only accept English input
 		 * at this point.
 		 */
-		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
+		yesno = git_prompt_echo(_("Are you sure [Y/n]? "));
 		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
 			return -1;
 		return 0;
@@ -838,8 +838,7 @@  static int bisect_autostart(struct bisect_terms *terms)
 	 * translation. The program will only accept English input
 	 * at this point.
 	 */
-	yesno = git_prompt(_("Do you want me to do it for you "
-			     "[Y/n]? "), PROMPT_ECHO);
+	yesno = git_prompt_echo(_("Do you want me to do it for you [Y/n]? "));
 	res = tolower(*yesno) == 'n' ?
 		-1 : bisect_start(terms, empty_strvec, 0);
 
diff --git a/credential.c b/credential.c
index e7240f3f636..5509de382a5 100644
--- a/credential.c
+++ b/credential.c
@@ -5,6 +5,7 @@ 
 #include "run-command.h"
 #include "url.h"
 #include "prompt.h"
+#include "prompt-password.h"
 #include "sigchain.h"
 #include "urlmatch.h"
 
@@ -169,11 +170,11 @@  static void credential_format(struct credential *c, struct strbuf *out)
 	}
 }
 
-static char *credential_ask_one(const char *what, struct credential *c,
-				int flags)
+static char *credential_ask_one(struct credential *c, unsigned int password)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct strbuf prompt = STRBUF_INIT;
+	const char *what = password ? "Password" : "Username";
 	char *r;
 
 	credential_describe(c, &desc);
@@ -182,7 +183,11 @@  static char *credential_ask_one(const char *what, struct credential *c,
 	else
 		strbuf_addf(&prompt, "%s: ", what);
 
-	r = git_prompt(prompt.buf, flags);
+	/* We'll try "askpass" for both usernames and passwords */
+	r = git_prompt_askpass(prompt.buf);
+	if (!r)
+		r = password ? git_prompt_noecho(prompt.buf)
+			     : git_prompt_echo(prompt.buf);
 
 	strbuf_release(&desc);
 	strbuf_release(&prompt);
@@ -192,11 +197,9 @@  static char *credential_ask_one(const char *what, struct credential *c,
 static void credential_getpass(struct credential *c)
 {
 	if (!c->username)
-		c->username = credential_ask_one("Username", c,
-						 PROMPT_ASKPASS|PROMPT_ECHO);
+		c->username = credential_ask_one(c, 0);
 	if (!c->password)
-		c->password = credential_ask_one("Password", c,
-						 PROMPT_ASKPASS);
+		c->password = credential_ask_one(c, 1);
 }
 
 int credential_read(struct credential *c, FILE *fp)
diff --git a/help.c b/help.c
index 973e47cdc30..6eebc26fbeb 100644
--- a/help.c
+++ b/help.c
@@ -644,7 +644,7 @@  const char *help_unknown_cmd(const char *cmd)
 			char *answer;
 			struct strbuf msg = STRBUF_INIT;
 			strbuf_addf(&msg, _("Run '%s' instead? (y/N)"), assumed);
-			answer = git_prompt(msg.buf, PROMPT_ECHO);
+			answer = git_prompt_echo(msg.buf);
 			strbuf_release(&msg);
 			if (!(starts_with(answer, "y") ||
 			      starts_with(answer, "Y")))
diff --git a/prompt-password.c b/prompt-password.c
new file mode 100644
index 00000000000..5fa00363d6c
--- /dev/null
+++ b/prompt-password.c
@@ -0,0 +1,63 @@ 
+#include "cache.h"
+#include "prompt-password.h"
+#include "strbuf.h"
+#include "run-command.h"
+#include "prompt.h"
+
+static char *do_askpass(const char *cmd, const char *prompt)
+{
+	struct child_process pass = CHILD_PROCESS_INIT;
+	const char *args[3];
+	static struct strbuf buffer = STRBUF_INIT;
+	int err = 0;
+
+	args[0] = cmd;
+	args[1]	= prompt;
+	args[2] = NULL;
+
+	pass.argv = args;
+	pass.out = -1;
+
+	if (start_command(&pass))
+		return NULL;
+
+	strbuf_reset(&buffer);
+	if (strbuf_read(&buffer, pass.out, 20) < 0)
+		err = 1;
+
+	close(pass.out);
+
+	if (finish_command(&pass))
+		err = 1;
+
+	if (err) {
+		error("unable to read askpass response from '%s'", cmd);
+		strbuf_release(&buffer);
+		return NULL;
+	}
+
+	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
+
+	return buffer.buf;
+}
+
+char *git_prompt_askpass(const char *prompt)
+{
+	const char *askpass;
+	char *r = NULL;
+
+	askpass = getenv("GIT_ASKPASS");
+	if (!askpass)
+		askpass = askpass_program;
+	if (!askpass)
+		askpass = getenv("SSH_ASKPASS");
+	if (askpass && *askpass)
+		r = do_askpass(askpass, prompt);
+
+	return r;
+}
+
+char *git_prompt_noecho(const char *prompt)
+{
+	return git_prompt(prompt, 0);
+}
diff --git a/prompt-password.h b/prompt-password.h
new file mode 100644
index 00000000000..84933d2b7c5
--- /dev/null
+++ b/prompt-password.h
@@ -0,0 +1,7 @@ 
+#ifndef PROMPT_PASSWORD_H
+#define PROMPT_PASSWORD_H
+
+char *git_prompt_askpass(const char *prompt);
+char *git_prompt_noecho(const char *prompt);
+
+#endif
diff --git a/prompt.c b/prompt.c
index 5ded21a017f..458d6637506 100644
--- a/prompt.c
+++ b/prompt.c
@@ -1,78 +1,27 @@ 
 #include "cache.h"
 #include "config.h"
-#include "run-command.h"
 #include "strbuf.h"
 #include "prompt.h"
 #include "compat/terminal.h"
 
-static char *do_askpass(const char *cmd, const char *prompt)
+char *git_prompt(const char *prompt, unsigned int echo)
 {
-	struct child_process pass = CHILD_PROCESS_INIT;
-	const char *args[3];
-	static struct strbuf buffer = STRBUF_INIT;
-	int err = 0;
-
-	args[0] = cmd;
-	args[1]	= prompt;
-	args[2] = NULL;
-
-	pass.argv = args;
-	pass.out = -1;
-
-	if (start_command(&pass))
-		return NULL;
-
-	strbuf_reset(&buffer);
-	if (strbuf_read(&buffer, pass.out, 20) < 0)
-		err = 1;
-
-	close(pass.out);
-
-	if (finish_command(&pass))
-		err = 1;
+	char *r = NULL;
 
-	if (err) {
-		error("unable to read askpass response from '%s'", cmd);
-		strbuf_release(&buffer);
-		return NULL;
+	if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
+		r = git_terminal_prompt(prompt, echo);
+		if (!r)
+			die_errno("could not read");
+	} else {
+		die("could not read terminal prompts disabled");
 	}
 
-	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
-
-	return buffer.buf;
+	return r;
 }
 
-char *git_prompt(const char *prompt, int flags)
+char *git_prompt_echo(const char *prompt)
 {
-	char *r = NULL;
-
-	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)
-			r = do_askpass(askpass, prompt);
-	}
-
-	if (!r) {
-		const char *err;
-
-		if (git_env_bool("GIT_TERMINAL_PROMPT", 1)) {
-			r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);
-			err = strerror(errno);
-		} else {
-			err = "terminal prompts disabled";
-		}
-		if (!r) {
-			/* prompts already contain ": " at the end */
-			die("could not read %s%s", prompt, err);
-		}
-	}
-	return r;
+	return git_prompt(prompt, 1);
 }
 
 int git_read_line_interactively(struct strbuf *line)
diff --git a/prompt.h b/prompt.h
index e294e93541c..f4d38178bc4 100644
--- a/prompt.h
+++ b/prompt.h
@@ -1,11 +1,8 @@ 
 #ifndef PROMPT_H
 #define PROMPT_H
 
-#define PROMPT_ASKPASS (1<<0)
-#define PROMPT_ECHO    (1<<1)
-
-char *git_prompt(const char *prompt, int flags);
-
+char *git_prompt(const char *prompt, unsigned int echo);
+char *git_prompt_echo(const char *prompt);
 int git_read_line_interactively(struct strbuf *line);
 
 #endif /* PROMPT_H */