Stefan Naewe [off-list ref] 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 <redacted>
---
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()?
I tried to rewrite the proposed commit log message to describe the real
issue, and here is what I came up with:
Author: Stefan Naewe [off-list ref]
Date: Fri Nov 4 08:03:08 2011 +0100
http: don't always prompt for password
When a username is already specified at the beginning of any HTTP
transaction (e.g. "git push https://user@hosting.example.com/project.git"
or "git ls-remote https://user@hosting.example.com/project.git"), the code
interactively asks for a password before calling into the libcurl library.
It is very likely that the reason why user included the username in the
URL is because the user knows that it would require authentication to
access the resource. Asking for the password upfront would save one
roundtrip to get a 401 response, getting the password and then retrying
the request. This is a reasonable optimization.
HOWEVER.
This is done even when $HOME/.netrc might have a corresponding entry to
access the site, or the site does not require authentication to access the
resource after all. But neither condition can be determined until we call
into libcurl library (we do not read and parse $HOME/.netrc ourselves). In
these cases, the user is forced to respond to the password prompt, only to
give a password that is not used in the HTTP transaction. If the password
is in $HOME/.netrc, an empty input would later let the libcurl layer to
pick up the password from there, and if the resource does not require
authentication, any input would be taken and then discarded without
getting used. It is wasteful to ask this unused information to the end
user.
Reduce the confusion by not trying to optimize for this case and always
incur roundtrip penalty. An alternative might be to document this and keep
this round-trip optimization as-is.
Signed-off-by: Stefan Naewe [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
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).
On Fri, Nov 04, 2011 at 09:48:17AM -0700, Junio C Hamano wrote:
Stefan Naewe [off-list ref] writes:
quoted
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 <redacted>
---
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.
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
On Fri, Nov 4, 2011 at 5:48 PM, Junio C Hamano [off-list ref] wrote:
Stefan Naewe [off-list ref] writes:
quoted
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 <redacted>
---
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()?
Dunno...
I think what Peff says makes sense.
I tried to rewrite the proposed commit log message to describe the real
issue, and here is what I came up with:
[...]
Looks good to me.
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).
Don't get me wrong, but I really don't care.
I just wanted to have that issue fixed, to get my scripted 'multi
repository pull' working
again.
Regards,
Stefan
--
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"