Re: [RFC/PATCH] http-push: don't always prompt for password
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 2, 2011, 17:13 UTC
- Message-ID
- <7vk47ijvlv.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4EB104EA.2040001@atlas-elektronik.com>
Stefan Näwe <stefan.naewe@atlas-elektronik.com> writes:
Show 51 quoted lines
> Am 01.11.2011 19:12, schrieb Junio C Hamano:
>>
>> There are only handful of commits that even remotely touch http related
>> codepath between v1.7.7 and v1.7.8-rc0:
>>
>> * deba493 http_init: accept separate URL parameter
>>
>> This could change the URL string given to http_auth_init().
>> ...
>> Could you try reverting deba493 and retest, and then if the behaviour is
>> the same "need ENTER", further revert 070b4dd and retest?
>
> I did a little more testing.
> This WIP makes it work for me (i.e. "need ENTER" is gone, works with
> and without .netrc, with 'https://host/repo.git' and
> 'https://user@host...' URL). Needs testing, of course.
>
> ---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---
> diff --git a/http.c b/http.c
> index a4bc770..008ad72 100644
> --- a/http.c
> +++ b/http.c
> @@ -279,8 +279,6 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
> #endif
>
> - init_curl_http_auth(result);
> -
> if (ssl_cert != NULL)
> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
> if (has_cert_password())
> @@ -846,7 +844,7 @@ static int http_request(const char *url, void *result, int target, int options)
> else if (missing_target(&results))
> ret = HTTP_MISSING_TARGET;
> else if (results.http_code == 401) {
> - if (user_name) {
> + if (user_name && user_pass) {
> ret = HTTP_NOAUTH;
> } else {
> /*
> @@ -855,7 +853,8 @@ static int http_request(const char *url, void *result, int target, int options)
> * but that is non-portable. Using git_getpass() can at least be stubbed
> * on other platforms with a different implementation if/when necessary.
> */
> - user_name = xstrdup(git_getpass_with_description("Username", description));
> + if (!user_name)
> + user_name = xstrdup(git_getpass_with_description("Username", description));
> init_curl_http_auth(slot->curl);
> ret = HTTP_REAUTH;
> }
> ---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---8<---This defers the calls to git_getpass* until we get 401 from the server side.
I am guessing the reason why in the current code get_curl_handle() has a call to init_curl_http_auth() very early, but only when user_name is set, is because it is likely for a site to require authentication when the user already has "username@" in its URL, and doing it this way will avoid the extra round-trip because by the time we make an HTTP request, we have both name and pass. If we apply this patch, the check in init_curl_http_auth() that asks for the password only when user_name is set becomes unnecessary.
I think the second hunk at l.846 sort of makes sense, but not quite.
"We got 401 even though we know we have supplied name and pass" is a valid criterion to decide that the name/pass is an invalid combination. But it makes me wonder if this code in its early days guaranteed whenever we have user_name we always have made sure we have user_pass (otherwise by asking for it with git_getpass) and that is the reason why it had to check only for user_name, and if that is the case perhaps the real breakage is we are not keeping that guarantee in the current code?
IOW, when in the current code do we make a new HTTP request while user_name is set but no user_pass? Perhaps if we fix that codepath we do not have to have this extra 401 roundtrip this patch is introducing when the username (but not password) is given?
Peff, what do you think?