list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: "Jarosław Honkis via GitGitGadget" <>
Cc: "Johannes Schindelin" <>,, "Junio C Hamano" <>,
	"Jarosław Honkis" <>
Subject: Re: [PATCH 1/1] credential: do not mask the username
Date: Mon, 29 Apr 2019 19:40:28 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Apr 29, 2019 at 03:06:11PM -0700, Jarosław Honkis via GitGitGadget wrote:

> From: =?UTF-8?q?Jaros=C5=82aw=20Honkis?= <>
> When a user is asked for credentials there is no need to mask the
> username, so the PROMPT_ASKPASS flag on calling credential_ask_one for
> login is unnecessary.
> The `credential_ask_one()` function internally uses `git_prompt()` which
> in case it is given the flag PROMPT_ASKPASS uses masked input method
> instead of git_terminal_prompt, which does not mask user input.

This description (and the patch) doesn't make sense to me. The
PROMPT_ASKPASS flag is just about whether we would trigger the askpass
tool (e.g., if the user does not have a terminal).

The PROMPT_ECHO flag is what you want to tell the underlying code that
the value can be shown to the user. And that's already set.

Now there is a slight issue, which is that the askpass tool has no way
for us to tell it to show the contents to the user.  There's no way
around that without disabling askpass entirely, which I guess is the
strategy this patch is trying to do. But in doing so it's going to break
anybody who _doesn't_ have a terminal, because now we have no way to
prompt there for their username!

So I think our options are:

  1. Leave it. If people don't want askpass to prompt them, they should
     not set up askpass.

  2. Use another tool besides askpass. I don't know of any askpass
     implementations that take something like our ECHO flag, but there
     are lots of other tools. I doubt there's any easy portable
     solution, though. And anyway, credential helpers are a much more
     advanced version of this anyway, so I'd probably steer people in
     that direction.

  3. If we really want to try to try to avoid using askpass for
     usernames we can, but I don't think the logic in this patch is

     If we want to avoid regressing existing cases, then we'd have to
     first check if there's a usable terminal. And only if _that_ fails,
     try askpass. And then give up if neither work. I.e., invert the
     order in git_prompt() when both ASKPASS and ECHO are set.

     I think I'd still favor option 1 over this, though. Configuring
     askpass has always overridden the terminal for usernames, and this
     would change that. I come back to: if you don't want to use
     askpass, then why are you configuring it?

>  		c->username = credential_ask_one("Username", c,
> +						 (getenv("GIT_ASKPASS") ?
> +						  PROMPT_ASKPASS : 0) |
> +						 PROMPT_ECHO);

This logic is weird. It still uses askpass if GIT_ASKPASS isn't set. But
_doesn't_ if it's set elsewhere, e.g. in core.askPass. Which makes
little sense to me.

Maybe the intent was that the original user has SSH_ASKPASS set, and
that is kicking in (which would also explain why "if you don't like it,
don't configure it" didn't work). You can get around that by setting
GIT_ASKPASS (or core.askPass) to the empty string (I don't think that's
documented anywhere, and I don't recall it being intentional, but it
does look like that's how the code works).


  parent reply	other threads:[~2019-04-29 23:40 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-29 22:06 [PATCH 0/1] " Johannes Schindelin via GitGitGadget
2019-04-29 22:06 ` [PATCH 1/1] " Jarosław Honkis via GitGitGadget
2019-04-29 22:12   ` Eric Sunshine
2019-04-29 23:06     ` Johannes Schindelin
2019-04-29 23:40   ` Jeff King [this message]
2019-04-30 22:07     ` Johannes Schindelin
2019-04-30 22:21       ` Jeff King
2019-05-03  9:09         ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).