{"thread":{"id":"48766","subject":"^D to credentials prompt results in \"fatal: ... Success\"","startedAt":"2018-06-23T02:42:54Z","lastAt":"2018-06-24T09:41:07Z","messageCount":2,"participants":["Anthony Sottile","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"350775","messageId":"CA+dzEB=RNqpduF+hx-y6HMeQYpSKJrBqx_pSu_MS3yDa7JHsww@mail.gmail.com","threadId":"48766","inReplyTo":null,"subject":"^D to credentials prompt results in \"fatal: ... Success\"","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2018-06-23T02:42:38Z","receivedAt":"2018-06-23T02:42:54Z","isPatch":false,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"A bit of an amusing edge case.\n\nI'm not exactly sure the correct approach to fix this but here's my\nreproduction, triage, and a few potential options I see.\n\nNote that after the username prompt, I pressed ^D\n\n$./prefix/bin/git --version\ngit version 2.18.0\n$ PATH=$PWD/prefix/bin:$PATH git clone\nhttps://github.com/asottile/this-does-not-exist-i-promise\nCloning into 'this-does-not-exist-i-promise'...\nUsername for 'https://github.com': fatal: could not read Username for\n'https://github.com': Success\n\nLooking at `prompt.c`, it's hitting this bit of code:\n\n        if (git_env_bool(\"GIT_TERMINAL_PROMPT\", 1)) {\n            r = git_terminal_prompt(prompt, flags & PROMPT_ECHO);\n            err = strerror(errno);\n        } else {\n            err = \"terminal prompts disabled\";\n\n        }\n        if (!r) {\n            /* prompts already contain \": \" at the end */\n            die(\"could not read %s%s\", prompt, err);\n        }\n\nFrom `git_terminal_prompt` (compat/terminal.c):\n\n    r = strbuf_getline_lf(&buf, input_fh);\n    if (!echo) {\n        putc('\\n', output_fh);\n        fflush(output_fh);\n    }\n\n    restore_term();\n    fclose(input_fh);\n    fclose(output_fh);\n\n    if (r == EOF)\n        return NULL;\n    return buf.buf;\n\n\nin the `EOF` case, this is returning `NULL`, but `errno = 0` at this\npoint causing the error string to be \"Success\"\n\nI see a couple of options here:\n\n1. special case EOF in `git_terminal_prompt` / `git_prompt` and\nproduce an error message such as:\n\nfatal: could not read Username for 'https://github.com': EOF\n\n(I tried returing `EOF` directly from `git_terminal_prompt` and was\nable to get this messaging to work, however `r == EOF` is a\npointer-int comparison so this approach didn't really seem like a good\nidea -- changing the signature of `git_terminal_prompt` to set a\nspecial flag for EOF is another option, but seems a lot of work for a\ncase that probably doesn't happen all too often)\n\nI also couldn't find an appropriate errno to set in the EOF case either\n\n2. treat EOF less specially\n\nThe function this is replacing, `getpass` simply returns an empty\nstring on `EOF`.  This patch would implement that:\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex fa13ee672..8bd08108e 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n        fputs(prompt, output_fh);\n        fflush(output_fh);\n\n-       r = strbuf_getline_lf(&buf, input_fh);\n+       strbuf_getline_lf(&buf, input_fh);\n        if (!echo) {\n                putc('\\n', output_fh);\n                fflush(output_fh);\n@@ -132,8 +132,6 @@ char *git_terminal_prompt(const char *prompt, int echo)\n        fclose(input_fh);\n        fclose(output_fh);\n\n-       if (r == EOF)\n-               return NULL;\n        return buf.buf;\n }\n\n\nhowever then the output is a bit strange for ^D (note I pressed ^D twice):\n\n$ PATH=$PWD/prefix/bin:$PATH git clone\nhttps://github.com/asottile/this-does-not-exist-i-promise\nCloning into 'this-does-not-exist-i-promise'...\nUsername for 'https://github.com': Password for 'https://github.com':\nremote: Repository not found.\nfatal: Authentication failed for\n'https://github.com/asottile/this-does-not-exist-i-promise/'\n\nAnthony\n"},{"id":"350793","messageId":"20180624094102.GA32079@sigill.intra.peff.net","threadId":"48766","inReplyTo":"CA+dzEB=RNqpduF+hx-y6HMeQYpSKJrBqx_pSu_MS3yDa7JHsww@mail.gmail.com","subject":"Re: ^D to credentials prompt results in \"fatal: ... Success\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-24T09:41:02Z","receivedAt":"2018-06-24T09:41:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 22, 2018 at 07:42:38PM -0700, Anthony Sottile wrote:\n\n> A bit of an amusing edge case.\n> \n> I'm not exactly sure the correct approach to fix this but here's my\n> reproduction, triage, and a few potential options I see.\n> \n> Note that after the username prompt, I pressed ^D\n> \n> $./prefix/bin/git --version\n> git version 2.18.0\n> $ PATH=$PWD/prefix/bin:$PATH git clone\n> https://github.com/asottile/this-does-not-exist-i-promise\n> Cloning into 'this-does-not-exist-i-promise'...\n> Username for 'https://github.com': fatal: could not read Username for\n> 'https://github.com': Success\n\nYeah, agreed that is not ideal.\n\n> I see a couple of options here:\n> \n> 1. special case EOF in `git_terminal_prompt` / `git_prompt` and\n> produce an error message such as:\n> \n> fatal: could not read Username for 'https://github.com': EOF\n> [...]\n>\n> 2. treat EOF less specially\n> \n> The function this is replacing, `getpass` simply returns an empty\n> string on `EOF`.  This patch would implement that:\n\nEither of those would be fine with me. We do not have to adhere to\ngetpass() behavior exactly (the whole point of having our custom wrapper\nis that getpass() behaves badly in a few situations). But I doubt\nanybody really cares that much either way, so we might as well have\nconsistent behavior.\n\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index fa13ee672..8bd08108e 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)\n>         fputs(prompt, output_fh);\n>         fflush(output_fh);\n> \n> -       r = strbuf_getline_lf(&buf, input_fh);\n> +       strbuf_getline_lf(&buf, input_fh);\n>         if (!echo) {\n>                 putc('\\n', output_fh);\n>                 fflush(output_fh);\n> @@ -132,8 +132,6 @@ char *git_terminal_prompt(const char *prompt, int echo)\n>         fclose(input_fh);\n>         fclose(output_fh);\n> \n> -       if (r == EOF)\n> -               return NULL;\n>         return buf.buf;\n>  }\n\nI think this goes too far, though. We get EOF from strbuf_getline on\nactual EOF _or_ on a real error. And we'd want to keep reporting a real\nerror, since it may tell the user something useful.\n\nI suspect you probably need to check ferror(input_fh) before closing,\nand rewrite \"r\" to 0 in that case.\n\n-Peff\n"}]}