Re: [PATCH] http-push: don't always prompt for password

From: Jeff King <peff@peff.net>
Date: Nov 4, 2011, 17:43 UTC
Message-ID: <20111104174303.GA22568@sigill.intra.peff.net>
In-Reply-To: <7vlirvdeb2.fsf@alter.siamese.dyndns.org>

On Fri, Nov 04, 2011 at 09:48:17AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Stefan Naewe <stefan.naewe@gmail.com> writes:
> 
> > http-push prompts for a password when the URL is set as
> > 'https://user@host/repo' even though there is one set
> > in ~/.netrc. Pressing ENTER at the password prompt succeeds
> > then, but is a annoying and makes it almost useless
> > in a shell script, e.g.
> >
> > Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
> > ---
> 
> Thanks.
> 
> With this the only callsite of init_curl_http_auth() becomes the one after
> we get the 401 response, and this caller makes sure that user_name is not
> NULL.
> 
> Do we still want "if (user_name)" inside init_curl_http_auth()?

Since we now only call init_curl_http_auth when we know we need auth, I think it would make more sense to just move the user_name asking there, too, like:

  static void init_curl_http_auth(CURL *result)
  {
          struct strbuf up = STRBUF_INIT;

          if (!user_name)
                  user_name = xstrdup(git_getpass_with_description("Username", description);
          if (!user_pass)
                  user_pass = xstrdup(git_getpass_with_description("Password", description);

          strbuf_addf(&up, "%s:%s", user_name, user_pass);
          curl_easy_setopt(result, CURLOPT_USERPWD, strbuf_detach(&up, NULL));
  }

And then it's easy to swap out the asking for credential_fill() when it becomes available. But I admit I don't care that much now, as I'll just end up doing that refactoring later with my credential patches anyway.

> I tried to rewrite the proposed commit log message to describe the real
> issue, and here is what I came up with:

Your description looks accurate to me.

Show 6 quoted lines

> What is somewhat troubling is that after analyzing the root cause of the
> issue, I am wondering if a more correct fix is to remove the user@ part
> from the URL (in other words, document that a URL with an embedded
> username will ask for password upfront, and tell the users that if they
> have netrc entries or if they are accessing a resource that does not
> require authentication, they should omit the username from the URL).

It's tempting, because the non-netrc case is the common one, and we are dropping the round-trip avoidance for those people. I'm just not sure that it's going to be obvious to users that they need to drop the user@ portion from their URL when using netrc. That seems like a bizarre requirement from the user's POV, even if we do document it.

-Peff