Re: ^D to credentials prompt results in "fatal: ... Success"

From: Jeff King <peff@peff.net>
Date: Jun 24, 2018, 09:41 UTC
Message-ID: <20180624094102.GA32079@sigill.intra.peff.net>
In-Reply-To: <CA+dzEB=RNqpduF+hx-y6HMeQYpSKJrBqx_pSu_MS3yDa7JHsww@mail.gmail.com>

On Fri, Jun 22, 2018 at 07:42:38PM -0700, Anthony Sottile wrote:

Show 14 quoted lines

> A bit of an amusing edge case.
> 
> I'm not exactly sure the correct approach to fix this but here's my
> reproduction, triage, and a few potential options I see.
> 
> Note that after the username prompt, I pressed ^D
> 
> $./prefix/bin/git --version
> git version 2.18.0
> $ PATH=$PWD/prefix/bin:$PATH git clone
> https://github.com/asottile/this-does-not-exist-i-promise
> Cloning into 'this-does-not-exist-i-promise'...
> Username for 'https://github.com': fatal: could not read Username for
> 'https://github.com': Success

Yeah, agreed that is not ideal.

Show 12 quoted lines

> I see a couple of options here:
> 
> 1. special case EOF in `git_terminal_prompt` / `git_prompt` and
> produce an error message such as:
> 
> fatal: could not read Username for 'https://github.com': EOF
> [...]
>
> 2. treat EOF less specially
> 
> The function this is replacing, `getpass` simply returns an empty
> string on `EOF`.  This patch would implement that:

Either of those would be fine with me. We do not have to adhere to getpass() behavior exactly (the whole point of having our custom wrapper is that getpass() behaves badly in a few situations). But I doubt anybody really cares that much either way, so we might as well have consistent behavior.

Show 21 quoted lines

> diff --git a/compat/terminal.c b/compat/terminal.c
> index fa13ee672..8bd08108e 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)
>         fputs(prompt, output_fh);
>         fflush(output_fh);
> 
> -       r = strbuf_getline_lf(&buf, input_fh);
> +       strbuf_getline_lf(&buf, input_fh);
>         if (!echo) {
>                 putc('\n', output_fh);
>                 fflush(output_fh);
> @@ -132,8 +132,6 @@ char *git_terminal_prompt(const char *prompt, int echo)
>         fclose(input_fh);
>         fclose(output_fh);
> 
> -       if (r == EOF)
> -               return NULL;
>         return buf.buf;
>  }

I think this goes too far, though. We get EOF from strbuf_getline on actual EOF _or_ on a real error. And we'd want to keep reporting a real error, since it may tell the user something useful.

I suspect you probably need to check ferror(input_fh) before closing, and rewrite "r" to 0 in that case.

-Peff