Re: [PATCH] http(s): automatically try NTLM authentication first

On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:

David Turner [off-list ref] writes:

quoted

Always, no.  For failed authentication (or authorization), apparently, yes.  
I tested this by  setting the variable to false and then true, and trying to 
Push to a github repository which I didn't have write access to, with 
both an empty username (https://@:github.com/...) and no username 
(http://github.com/...).   I ran this under GIT_CURL_VERBOSE=1 and
I saw two 401 responses in the "http.emptyauth=true" case and one
in the false case.  I also tried with a repo that I did have access to (first
configuring the necessary tokens for HTTPS push access), and saw two
401 responses in *both* cases.  

Thanks; that matches my observation.  I do not think we care about
an extra roundtrip for the failure case, but as long as we do not
increase the number of roundtrip in the normal case, we can declare
that this is an improvement.  I am not quite sure where that extra
401 comes from in the normal case, and that might be an indication
that we already are doing something wrong, though.

This patch drops the useless probe request:

diff --git a/http.c b/http.c
index 943e630ea..7b4c2db86 100644
--- a/http.c
+++ b/http.c

@@ -1663,6 +1663,9 @@ static int http_request(const char *url,
 		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
 				options->effective_url);
 
+	if (results.auth_avail == CURLAUTH_BASIC)
+		http_auth_methods = CURLAUTH_BASIC;
+
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 

but setting http.emptyauth adds back in the useless request. I think
that could be fixed by skipping the empty-auth thing when
http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps
other methods need it to, so maybe skip it if _just_ BASIC is set).

I suspect the patch above could probably be generalized as:

  /* cut out methods we know the server doesn't support */
  http_auth_methods &= results.auth_avail;

and let curl figure it out from there.

-Peff