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

[Jeff King <peff@peff.net>]

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