threads / patch / 29153

patch, 2 partst5540: test DAV push with authentication

Subject: [PATCH 1/2] t5540: test DAV push with authentication

## tl;dr

14 messages between Dec 13, 2011 and Dec 14, 2011. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Jeff King· Dec 13, 2011, 20:17 UTC · lore

We don't currently test this case at all, and instead just test the DAV mechanism over an unauthenticated push. That isn't very realistic, as most people will want to authenticate pushes.

Two of the tests expect_failure as they reveal bugs:
  1. Pushing without a username in the URL fails to ask for
     credentials when we get an HTTP 401. This has always
     been the case, but it would be nice if it worked like
     smart-http.
  2. Pushing with a username fails to ask for the password
     since 986bbc0 (http: don't always prompt for password,
     2011-11-04). This is a severe regression in v1.7.8, as
     authenticated push-over-DAV is now totally unusable
     unless you have credentials in your .netrc.
Signed-off-by: Jeff King <peff@peff.net>
---
Nobody has mentioned the regression on git@vger yet, but there are two
threads already on msysgit:
  http://thread.gmane.org/gmane.comp.version-control.msysgit/14138
  http://thread.gmane.org/gmane.comp.version-control.msysgit/14161
 t/lib-httpd/apache.conf |    3 +++
 t/t5540-http-push.sh    |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)
Show changes to 2 files +37 −0

t/lib-httpd/apache.conf, t/t5540-http-push.sh

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 0a4cdfa..3c12b05 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -92,6 +92,9 @@ SSLEngine On
 	<Location /dumb/>
 		Dav on
 	</Location>
+	<Location /auth/dumb>
+		Dav on
+	</Location>
 </IfDefine>
 
 <IfDefine SVN>
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 64767d8..3300227 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -40,6 +40,22 @@ test_expect_success 'setup remote repository' '
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
 '
 
+test_expect_success 'create password-protected repository' '
+	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb" &&
+	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+	       "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git"
+'
+
+test_expect_success 'setup askpass helper' '
+	cat >askpass <<-\EOF &&
+	#!/bin/sh
+	echo user@host
+	EOF
+	chmod +x askpass &&
+	GIT_ASKPASS="$PWD/askpass" &&
+	export GIT_ASKPASS
+'
+
 test_expect_success 'clone remote repository' '
 	cd "$ROOT_PATH" &&
 	git clone $HTTPD_URL/dumb/test_repo.git test_repo_clone
@@ -144,6 +160,24 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 	"$ROOT_PATH"/test_repo_clone master
 
+test_expect_failure 'push to password-protected repository (user in URL)' '
+	test_commit pw-user &&
+	git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
+	git rev-parse --verify HEAD >expect &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+		rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'push to password-protected repository (no user in URL)' '
+	test_commit pw-nouser &&
+	git push "$HTTPD_URL/auth/dumb/test_repo.git" HEAD &&
+	git rev-parse --verify HEAD >expect &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git" \
+		rev-parse --verify HEAD >actual &&
+	test_cmp expect actual
+'
+
 stop_httpd
 
 test_done

-- 
1.7.8.17.gfd3524
Jeff King· Dec 13, 2011, 20:25 UTC · re: Jeff King · lore

[PATCH] Revert "http: don't always prompt for password"

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(-)
Show changes to http.c +4 −3
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
Junio C Hamano· Dec 13, 2011, 21:09 UTC · re: Jeff King · lore

Re: [PATCH] Revert "http: don't always prompt for password"

Jeff King <peff@peff.net> writes:
> 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.
Or just let the "dumb HTTP" die.
I thought push over DAV has long been dead; is anybody using it for real?
Eric Advincula· Dec 13, 2011, 21:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "http: don't always prompt for password"

Is there an alternative to using git on windows? I used windows, apache, dav for git. If there is a better solution please let me know.

Thanks Eric

On Tue, Dec 13, 2011 at 2:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > 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.
>
> Or just let the "dumb HTTP" die.
>
> I thought push over DAV has long been dead; is anybody using it for real?
>
Jeff King· Dec 13, 2011, 23:18 UTC · re: Eric Advincula · lore

Re: [PATCH] Revert "http: don't always prompt for password"

On Tue, Dec 13, 2011 at 02:22:12PM -0700, Eric Advincula wrote:
> Is there an alternative to using git on windows?  I used windows, apache,
> dav for git.
> If there is a better solution please let me know.

I don't know the status of running the smart-http backend on Windows, but that would be a preferable solution. It's way more efficient, and the client half of the code is better maintained.

See "git help http-backend" for documentation.
-Peff
Jeff King· Dec 13, 2011, 23:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "http: don't always prompt for password"

On Tue, Dec 13, 2011 at 01:09:42PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Or just let the "dumb HTTP" die.
> 
> I thought push over DAV has long been dead; is anybody using it for real?

For the record, I have no problem whatsoever with letting it die. I just think we probably shouldn't do it accidentally during a release. ;)

-Peff
Jeff King· Dec 13, 2011, 23:20 UTC · re: Jeff King · lore

Re: [PATCH] Revert "http: don't always prompt for password"

On Tue, Dec 13, 2011 at 06:19:09PM -0500, Jeff King wrote:
Show 6 quoted lines
> > Or just let the "dumb HTTP" die.
> > 
> > I thought push over DAV has long been dead; is anybody using it for real?
> 
> For the record, I have no problem whatsoever with letting it die. I just
> think we probably shouldn't do it accidentally during a release. ;)

BTW, one other solution, rather than reverting Stefan's patch, is to just re-add the "unconditionally ask for a password" behavior back to git-http-push, but not to the fetching side. Especially if we hope to kill off git-http-push soon (after a deprecation period presumably), then that lets it work in the meantime without hurting the other http code. And it's really easy to do.

-Peff
Jeff King· Dec 14, 2011, 00:11 UTC · re: Jeff King · lore

Re: [PATCH] Revert "http: don't always prompt for password"

On Tue, Dec 13, 2011 at 06:20:53PM -0500, Jeff King wrote:
Show 6 quoted lines
> BTW, one other solution, rather than reverting Stefan's patch, is to
> just re-add the "unconditionally ask for a password" behavior back to
> git-http-push, but not to the fetching side. Especially if we hope to
> kill off git-http-push soon (after a deprecation period presumably),
> then that lets it work in the meantime without hurting the other http
> code. And it's really easy to do.
And here's what that patch looks like.

Thinking on it more, this is probably better for a maint release than reverting Stefan's patch. It un-breaks http-push, and they are no differently off than they are with the revert. But it leaves the enhancement in place for the smart-http code.

-- >8 --
Subject: [PATCH] http-push: enable "proactive auth"

Before commit 986bbc08, git was proactive about asking for http passwords. It assumed that if you had a username in your URL, you would also want a password, and asked for it before making any http requests.

However, this could interfere with the use of .netrc (see 986bbc08 for details). And it was also unnecessary, since the http fetching code had learned to recognize an HTTP 401 and prompt the user then. Furthermore, the proactive prompt could interfere with the usage of .netrc (see 986bbc08 for details).

Unfortunately, the http push-over-DAV code never learned to recognize HTTP 401, and so was broken by this change. This patch does a quick fix of re-enabling the "proactive auth" strategy only for http-push, leaving the dumb http fetch and smart-http as-is.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-fetch.c         |    2 +-
 http-push.c          |    2 +-
 http.c               |    8 +++++++-
 http.h               |    3 ++-
 remote-curl.c        |    2 +-
 t/t5540-http-push.sh |    2 +-
 6 files changed, 13 insertions(+), 6 deletions(-)
Show changes to 6 files +13 −6

http-fetch.c, http-push.c, http.c, http.h, remote-curl.c, t/t5540-http-push.sh

diff --git a/http-fetch.c b/http-fetch.c
index 69299b7..94d47cb 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -67,7 +67,7 @@ int main(int argc, const char **argv)
 
 	git_config(git_default_config, NULL);
 
-	http_init(NULL, url);
+	http_init(NULL, url, 0);
 	walker = get_http_walker(url);
 	walker->get_tree = get_tree;
 	walker->get_history = get_history;
diff --git a/http-push.c b/http-push.c
index 5d01be9..70283e6 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1820,7 +1820,7 @@ int main(int argc, char **argv)
 
 	memset(remote_dir_exists, -1, 256);
 
-	http_init(NULL, repo->url);
+	http_init(NULL, repo->url, 1);
 
 #ifdef USE_CURL_MULTI
 	is_running_queue = 0;
diff --git a/http.c b/http.c
index 008ad72..6c90092 100644
--- a/http.c
+++ b/http.c
@@ -43,6 +43,7 @@
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static char *user_name, *user_pass, *description;
+static int http_proactive_auth;
 static const char *user_agent;
 
 #if LIBCURL_VERSION_NUM >= 0x071700
@@ -279,6 +280,9 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 #endif
 
+	if (http_proactive_auth)
+		init_curl_http_auth(result);
+
 	if (ssl_cert != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
@@ -367,7 +371,7 @@ static void set_from_env(const char **var, const char *envname)
 		*var = val;
 }
 
-void http_init(struct remote *remote, const char *url)
+void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
 	char *low_speed_limit;
 	char *low_speed_time;
@@ -378,6 +382,8 @@ void http_init(struct remote *remote, const char *url)
 
 	curl_global_init(CURL_GLOBAL_ALL);
 
+	http_proactive_auth = proactive_auth;
+
 	if (remote && remote->http_proxy)
 		curl_http_proxy = xstrdup(remote->http_proxy);
 
diff --git a/http.h b/http.h
index 3c332a9..51f6ba7 100644
--- a/http.h
+++ b/http.h
@@ -86,7 +86,8 @@ struct buffer {
 extern void step_active_slots(void);
 #endif
 
-extern void http_init(struct remote *remote, const char *url);
+extern void http_init(struct remote *remote, const char *url,
+		      int proactive_auth);
 extern void http_cleanup(void);
 
 extern int data_received;
diff --git a/remote-curl.c b/remote-curl.c
index 0e720ee..0757b19 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -859,7 +859,7 @@ int main(int argc, const char **argv)
 
 	url = strbuf_detach(&buf, NULL);
 
-	http_init(remote, url);
+	http_init(remote, url, 0);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 3300227..1eea647 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -160,7 +160,7 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 	"$ROOT_PATH"/test_repo_clone master
 
-test_expect_failure 'push to password-protected repository (user in URL)' '
+test_expect_success 'push to password-protected repository (user in URL)' '
 	test_commit pw-user &&
 	git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
 	git rev-parse --verify HEAD >expect &&

-- 
1.7.8.17.gfd3524
Junio C Hamano· Dec 14, 2011, 00:33 UTC · re: Jeff King · lore

Re: [PATCH] Revert "http: don't always prompt for password"

Jeff King <peff@peff.net> writes:
> ... This
> patch does a quick fix of re-enabling the "proactive auth"
> strategy only for http-push, leaving the dumb http fetch and
> smart-http as-is.
Yeah, I somehow like this better. Thanks.
Matthieu Moy· Dec 14, 2011, 08:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "http: don't always prompt for password"

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> 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.
>
> Or just let the "dumb HTTP" die.
>
> I thought push over DAV has long been dead; is anybody using it for real?
I am.

I'm working with people behind a firewall, hence HTTP is mandantory. My lab has a webdav server, without Git installed on it. Being able to work with this setup was one of the key feature of Git when I adopted it (after a few years using GNU Arch the same way).

I could probably manage to convince my sysadmin to install Git on our webserver, but I'd prefer if Git continues supporting my current setup.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Dec 13, 2011, 21:25 UTC · re: Jeff King · lore

Re: [PATCH] Revert "http: don't always prompt for password"

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> @@ -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 {
>  				/*

In the credential series, this is where we declare the given credential is to be rejected (if we have both username and password), or ask them to be filled by calling credential_fill(), so I think the code in the credential series does not need this revert. Right?

Show 10 quoted lines
> @@ -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;
>  			}
So is this one.
Jeff King· Dec 13, 2011, 23:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] Revert "http: don't always prompt for password"

On Tue, Dec 13, 2011 at 01:25:18PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> > @@ -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 {
> >  				/*
> 
> In the credential series, this is where we declare the given credential is
> to be rejected (if we have both username and password), or ask them to be
> filled by calling credential_fill(), so I think the code in the credential
> series does not need this revert. Right?

Sort of. The point of this conditional is "did we actually send a credential? If yes, then return HTTP_NOAUTH. Otherwise, ask for the credential and return HTTP_REAUTH"[1].

Prior to Stefan's patch, if user_name was set, then we sent a credential (because we always filled in the password if user_name was set). After, we had to check both (actually, I think checking user_pass would have been sufficient)).

The situation is the same with credentials, but the variable name is different. Even though credential_fill will fill in both parts, we may have set http_auth.username from the URL, but not actually called credential_fill (and therefore not sent any credentials).

So logically, the revert in the credential series would be:
  -       if (http_auth.username && http_auth.password)
  +       if (http_auth.username)

except that I believe the former is a superset of the latter in both cases (with or without the credential topic). So leaving it as-is would be OK. In fact, when reverting Stefan's patch, you could just drop this hunk entirely, which might be worth it to avoid conflicts with in-flight topics.

[1] A much saner approach would be to always return HTTP_NOAUTH, and
    then let the caller decide to re-ask for credentials and re-try.
    But we need the magic curl slot object, which the caller doesn't
    have. So doing it that way would have taken some refactoring.
Show 12 quoted lines
> > @@ -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;
> >  			}
> 
> So is this one.

Yeah, this code just goes away, as credential_fill() does the right thing.

But again, the "if (!user_name)" version post-986bbc08 handles both the pre-986bbc08 condition (because it will always be NULL then) and the post-986bbc08 (because we do need to check then). So I believe you could drop the hunk entirely.

Here's a re-roll of the revert that touches as little as possible. I believe it's correct from my analysis above, and it does pass the tests. I also included the flipping of the "expect_failure" test, which I forgot to put in my original patch.

-- >8 --
Subject: [PATCH] Revert "http: don't always prompt for password"
This is a partial revert of 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.

This is a partial revert that just restores the "don't ask for password bits". There were some other related adjustments in 986bbc08 to handle the fact that the user_name field might be set even if we didn't send a credential on the first try. The new logic introduced by 986bbc08 handles both the old case and the new case, so we can leave them intact. That will prevent unnecessary conflicts with other in-flight topics that touch this code.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c               |    2 ++
 t/t5540-http-push.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)
Show changes to 2 files +3 −1

http.c, t/t5540-http-push.sh

diff --git a/http.c b/http.c
index 008ad72..33c63b5 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())
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 3300227..1eea647 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -160,7 +160,7 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 	"$ROOT_PATH"/test_repo_clone master
 
-test_expect_failure 'push to password-protected repository (user in URL)' '
+test_expect_success 'push to password-protected repository (user in URL)' '
 	test_commit pw-user &&
 	git push "$HTTPD_URL_USER/auth/dumb/test_repo.git" HEAD &&
 	git rev-parse --verify HEAD >expect &&

-- 
1.7.8.17.gfd3524
Sebastian Schuberth· Dec 13, 2011, 21:28 UTC · re: Jeff King · lore

Re: [PATCH 1/2] t5540: test DAV push with authentication

On Tue, Dec 13, 2011 at 21:17, Jeff King <peff@peff.net> wrote:
> We don't currently test this case at all, and instead just
> test the DAV mechanism over an unauthenticated push. That
> isn't very realistic, as most people will want to
> authenticate pushes.
Thanks for adding this, Peff!
-- 
Sebastian Schuberth
Jeff King· Dec 13, 2011, 23:16 UTC · re: Sebastian Schuberth · lore

Re: [PATCH 1/2] t5540: test DAV push with authentication

On Tue, Dec 13, 2011 at 10:28:07PM +0100, Sebastian Schuberth wrote:
Show 8 quoted lines
> On Tue, Dec 13, 2011 at 21:17, Jeff King <peff@peff.net> wrote:
> 
> > We don't currently test this case at all, and instead just
> > test the DAV mechanism over an unauthenticated push. That
> > isn't very realistic, as most people will want to
> > authenticate pushes.
> 
> Thanks for adding this, Peff!

You're welcome. Thank you for forwarding the bug report. I would never have seen it on the msysgit list, and for some reason it seems that msysgit people are more likely to use DAV.

Having looked a lot at the http code the past month or two, I knew it was pretty flaky and I was nervous when we added Stefan's patch (and no, I don't blame Stefan; his patch was completely reasonable, but just happened to trigger a problem in a seldom-looked-at corner of the code).

But I hadn't looked at http-push at all until yesterday, and it didn't even occur to me that there was another whole area of code relying in a very obscure way on the http.c auth code. I'll take a look at some of the refactoring I've done in http.c (both for the credentials topic as well as the bundle topic) and see if we can't integrate http-push.c a little more smoothly.

-Peff

← back to recent threads