Re: [PATCH 1/2] prompt.c: split up the password and non-password handling
- From
Jeff King <peff@peff.net>
- Date
- Nov 3, 2021, 11:53 UTC
- Message-ID
- <YYJ4HYyl1tBs2cZN@coredump.intra.peff.net>
- In-Reply-To
- <patch-1.2-ce5bea43f03-20211102T155046Z-avarab@gmail.com>
On Tue, Nov 02, 2021 at 05:48:09PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> 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:
Show 6 quoted lines
> - 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:
Show 11 quoted lines
> @@ -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