git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Stefan Naewe <stefan.naewe@gmail.com>,
	Sebastian Schuberth <sschuberth@gmail.com>,
	Eric <eric.advincula@gmail.com>,
	git@vger.kernel.org, msysgit@googlegroups.com
Subject: [PATCH] Revert "http: don't always prompt for password"
Date: Tue, 13 Dec 2011 15:25:08 -0500	[thread overview]
Message-ID: <20111213202508.GA12187@sigill.intra.peff.net> (raw)
In-Reply-To: <20111213201704.GA12072@sigill.intra.peff.net>

This reverts commit 986bbc0842334f0e07731fa37f2a55d2930a5b8c.

The rationale for that commit relied on the fact that asking
for the password up-front was merely an optimization,
because git will notice an HTTP 401 and prompt for the
password. However, that is only true for smart-http, and for
dumb fetching. Dumb push over DAV does not have this
feature; as a result, authenticated push-over-DAV does not
work at all, as it never prompts the user for a password.

Signed-off-by: Jeff King <peff@peff.net>
---
We need to deal with this regression for v1.7.8.1, I think.

There are basically three options for fixing it:

  1. Teach http-push the same retry-after-401 trick that the rest of the
     http code knows.

  2. Refactor the retry-after-401 logic from http.c into a common
     function that http-push can build on top of.

  3. Revert 986bbc08 and leave it alone; it only hurts .netrc users,
     there's a reasonable workaround (don't put the user in the URL) and
     hopefully those people will convert to using better storage via
     credential helper once it is available.

I looked at doing (1), but my first attempt[1] didn't quite work. So
it's not a huge amount of code, but it's annoyingly non-trivial. And as
a long-term solution, it's just making hack-y code hackier.

Doing (2) would be the best solution, but it's going to require some
pretty major surgery to http.c and http-push.c. I'll take a look, but if
it gets too complex, it may simply not be worth it (now that smart-http
is available, I would hope that push-over-DAV is slowly going away).

Doing (3) is obviously the easiest thing. And given the complexity of
the other two solutions, I think it makes sense to revert 986bbc08
(i.e., apply this patch), ship a working v1.7.8.1, and then look at
doing one of the other two solutions for v1.7.9.

[1] http://article.gmane.org/gmane.comp.version-control.msysgit/14153

 http.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 008ad72..a4bc770 100644
--- a/http.c
+++ b/http.c
@@ -279,6 +279,8 @@ 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())
@@ -844,7 +846,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 && user_pass) {
+			if (user_name) {
 				ret = HTTP_NOAUTH;
 			} else {
 				/*
@@ -853,8 +855,7 @@ 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.
 				 */
-				if (!user_name)
-					user_name = xstrdup(git_getpass_with_description("Username", description));
+				user_name = xstrdup(git_getpass_with_description("Username", description));
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}
-- 
1.7.8.17.gfd3524

  reply	other threads:[~2011-12-13 20:25 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-13 20:17 [PATCH 1/2] t5540: test DAV push with authentication Jeff King
2011-12-13 20:25 ` Jeff King [this message]
2011-12-13 21:09   ` [PATCH] Revert "http: don't always prompt for password" Junio C Hamano
2011-12-13 21:22     ` Eric Advincula
2011-12-13 23:18       ` Jeff King
2011-12-13 23:19     ` Jeff King
2011-12-13 23:20       ` Jeff King
2011-12-14  0:11         ` Jeff King
2011-12-14  0:33           ` Junio C Hamano
2011-12-14  8:20     ` Matthieu Moy
2011-12-13 21:25   ` Junio C Hamano
2011-12-13 23:10     ` Jeff King
2011-12-13 21:28 ` [PATCH 1/2] t5540: test DAV push with authentication Sebastian Schuberth
2011-12-13 23:16   ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20111213202508.GA12187@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=eric.advincula@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=msysgit@googlegroups.com \
    --cc=sschuberth@gmail.com \
    --cc=stefan.naewe@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).