{"thread":{"id":"29153","subject":"[PATCH 1/2] t5540: test DAV push with authentication","startedAt":"2011-12-13T20:17:04Z","lastAt":"2011-12-14T08:20:45Z","messageCount":14,"participants":["Jeff King","Junio C Hamano","Sebastian Schuberth","Eric Advincula","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"181076","messageId":"20111213201704.GA12072@sigill.intra.peff.net","threadId":"29153","inReplyTo":null,"subject":"[PATCH 1/2] t5540: test DAV push with authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T20:17:04Z","receivedAt":"2011-12-13T20:17:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't currently test this case at all, and instead just\ntest the DAV mechanism over an unauthenticated push. That\nisn't very realistic, as most people will want to\nauthenticate pushes.\n\nTwo of the tests expect_failure as they reveal bugs:\n\n 1. Pushing without a username in the URL fails to ask for\n credentials when we get an HTTP 401. This has always\n been the case, but it would be nice if it worked like\n smart-http.\n\n 2. Pushing with a username fails to ask for the password\n since 986bbc0 (http: don't always prompt for password,\n 2011-11-04). This is a severe regression in v1.7.8, as\n authenticated push-over-DAV is now totally unusable\n unless you have credentials in your .netrc.\n\nSigned-off-by: Jeff King \n---\nNobody has mentioned the regression on git@vger yet, but there are two\nthreads already on msysgit:\n\n http://thread.gmane.org/gmane.comp.version-control.msysgit/14138\n\n http://thread.gmane.org/gmane.comp.version-control.msysgit/14161\n\n t/lib-httpd/apache.conf | 3 +++\n t/t5540-http-push.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 0 deletions(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 0a4cdfa..3c12b05 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -92,6 +92,9 @@ SSLEngine On\n \t\n \t\tDav on\n \t\n+\t\n+\t\tDav on\n+\t\n \n \n \ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 64767d8..3300227 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -40,6 +40,22 @@ test_expect_success 'setup remote repository' '\n \tmv test_repo.git \"$HTTPD_DOCUMENT_ROOT_PATH\"\n '\n \n+test_expect_success 'create password-protected repository' '\n+\tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb\" &&\n+\tcp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git\" \\\n+\t \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\"\n+'\n+\n+test_expect_success 'setup askpass helper' '\n+\tcat >askpass <<-\\EOF &&\n+\t#!/bin/sh\n+\techo user@host\n+\tEOF\n+\tchmod +x askpass &&\n+\tGIT_ASKPASS=\"$PWD/askpass\" &&\n+\texport GIT_ASKPASS\n+'\n+\n test_expect_success 'clone remote repository' '\n \tcd \"$ROOT_PATH\" &&\n \tgit clone $HTTPD_URL/dumb/test_repo.git test_repo_clone\n@@ -144,6 +160,24 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '\n test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n \t\"$ROOT_PATH\"/test_repo_clone master\n \n+test_expect_failure 'push to password-protected repository (user in URL)' '\n+\ttest_commit pw-user &&\n+\tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n+\tgit rev-parse --verify HEAD >expect &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n+\t\trev-parse --verify HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'push to password-protected repository (no user in URL)' '\n+\ttest_commit pw-nouser &&\n+\tgit push \"$HTTPD_URL/auth/dumb/test_repo.git\" HEAD &&\n+\tgit rev-parse --verify HEAD >expect &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git\" \\\n+\t\trev-parse --verify HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n stop_httpd\n \n test_done\n-- \n1.7.8.17.gfd3524\n"},{"id":"181077","messageId":"20111213202508.GA12187@sigill.intra.peff.net","threadId":"29153","inReplyTo":"20111213201704.GA12072@sigill.intra.peff.net","subject":"[PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T20:25:08Z","receivedAt":"2011-12-13T20:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 986bbc0842334f0e07731fa37f2a55d2930a5b8c.\n\nThe rationale for that commit relied on the fact that asking\nfor the password up-front was merely an optimization,\nbecause git will notice an HTTP 401 and prompt for the\npassword. However, that is only true for smart-http, and for\ndumb fetching. Dumb push over DAV does not have this\nfeature; as a result, authenticated push-over-DAV does not\nwork at all, as it never prompts the user for a password.\n\nSigned-off-by: Jeff King \n---\nWe need to deal with this regression for v1.7.8.1, I think.\n\nThere are basically three options for fixing it:\n\n 1. Teach http-push the same retry-after-401 trick that the rest of the\n http code knows.\n\n 2. Refactor the retry-after-401 logic from http.c into a common\n function that http-push can build on top of.\n\n 3. Revert 986bbc08 and leave it alone; it only hurts .netrc users,\n there's a reasonable workaround (don't put the user in the URL) and\n hopefully those people will convert to using better storage via\n credential helper once it is available.\n\nI looked at doing (1), but my first attempt[1] didn't quite work. So\nit's not a huge amount of code, but it's annoyingly non-trivial. And as\na long-term solution, it's just making hack-y code hackier.\n\nDoing (2) would be the best solution, but it's going to require some\npretty major surgery to http.c and http-push.c. I'll take a look, but if\nit gets too complex, it may simply not be worth it (now that smart-http\nis available, I would hope that push-over-DAV is slowly going away).\n\nDoing (3) is obviously the easiest thing. And given the complexity of\nthe other two solutions, I think it makes sense to revert 986bbc08\n(i.e., apply this patch), ship a working v1.7.8.1, and then look at\ndoing one of the other two solutions for v1.7.9.\n\n[1] http://article.gmane.org/gmane.comp.version-control.msysgit/14153\n\n http.c | 7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 008ad72..a4bc770 100644\n--- a/http.c\n+++ b/http.c\n@@ -279,6 +279,8 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n #endif\n \n+\tinit_curl_http_auth(result);\n+\n \tif (ssl_cert != NULL)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n \tif (has_cert_password())\n@@ -844,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\telse if (missing_target(&results))\n \t\t\tret = HTTP_MISSING_TARGET;\n \t\telse if (results.http_code == 401) {\n-\t\t\tif (user_name && user_pass) {\n+\t\t\tif (user_name) {\n \t\t\t\tret = HTTP_NOAUTH;\n \t\t\t} else {\n \t\t\t\t/*\n@@ -853,8 +855,7 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\t\t\t * but that is non-portable. Using git_getpass() can at least be stubbed\n \t\t\t\t * on other platforms with a different implementation if/when necessary.\n \t\t\t\t */\n-\t\t\t\tif (!user_name)\n-\t\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n+\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n \t\t\t\tinit_curl_http_auth(slot->curl);\n \t\t\t\tret = HTTP_REAUTH;\n \t\t\t}\n-- \n1.7.8.17.gfd3524\n"},{"id":"181078","messageId":"7vaa6wuqjt.fsf@alter.siamese.dyndns.org","threadId":"29153","inReplyTo":"20111213202508.GA12187@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T21:09:42Z","receivedAt":"2011-12-13T21:09:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Doing (3) is obviously the easiest thing. And given the complexity of\n> the other two solutions, I think it makes sense to revert 986bbc08\n> (i.e., apply this patch), ship a working v1.7.8.1, and then look at\n> doing one of the other two solutions for v1.7.9.\n\nOr just let the \"dumb HTTP\" die.\n\nI thought push over DAV has long been dead; is anybody using it for real?\n"},{"id":"181084","messageId":"CA+e6NDTQNEJB6Y17ow+yuLp2Bc799wsyQdnS8immMDYdCEyCGA@mail.gmail.com","threadId":"29153","inReplyTo":"7vaa6wuqjt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Eric Advincula","fromEmail":"eric.advincula@gmail.com","sentAt":"2011-12-13T21:22:12Z","receivedAt":"2011-12-13T21:22:12Z","isPatch":true,"sender":{"key":"eric.advincula@gmail.com","avatar":null},"body":"Is there an alternative to using git on windows? I used windows, apache,\ndav for git.\nIf there is a better solution please let me know.\n\nThanks\nEric\n\nOn Tue, Dec 13, 2011 at 2:09 PM, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n> > Doing (3) is obviously the easiest thing. And given the complexity of\n> > the other two solutions, I think it makes sense to revert 986bbc08\n> > (i.e., apply this patch), ship a working v1.7.8.1, and then look at\n> > doing one of the other two solutions for v1.7.9.\n>\n> Or just let the \"dumb HTTP\" die.\n>\n> I thought push over DAV has long been dead; is anybody using it for real?\n>\n"},{"id":"181081","messageId":"7v62hkuptt.fsf@alter.siamese.dyndns.org","threadId":"29153","inReplyTo":"20111213202508.GA12187@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T21:25:18Z","receivedAt":"2011-12-13T21:25:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -844,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)\n> \t\telse if (missing_target(&results))\n> \t\t\tret = HTTP_MISSING_TARGET;\n> \t\telse if (results.http_code == 401) {\n> -\t\t\tif (user_name && user_pass) {\n> +\t\t\tif (user_name) {\n> \t\t\t\tret = HTTP_NOAUTH;\n> \t\t\t} else {\n> \t\t\t\t/*\n\nIn the credential series, this is where we declare the given credential is\nto be rejected (if we have both username and password), or ask them to be\nfilled by calling credential_fill(), so I think the code in the credential\nseries does not need this revert. Right?\n\n> @@ -853,8 +855,7 @@ static int http_request(const char *url, void *result, int target, int options)\n> \t\t\t\t * but that is non-portable. Using git_getpass() can at least be stubbed\n> \t\t\t\t * on other platforms with a different implementation if/when necessary.\n> \t\t\t\t */\n> -\t\t\t\tif (!user_name)\n> -\t\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n> +\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n> \t\t\t\tinit_curl_http_auth(slot->curl);\n> \t\t\t\tret = HTTP_REAUTH;\n> \t\t\t}\n\nSo is this one.\n"},{"id":"181082","messageId":"CAHGBnuO1CFGacMQb+Md_uGsLu=V9bGDpcFLd9Lb4C5jSN+uUiw@mail.gmail.com","threadId":"29153","inReplyTo":"20111213201704.GA12072@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t5540: test DAV push with authentication","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-12-13T21:28:07Z","receivedAt":"2011-12-13T21:28:07Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Dec 13, 2011 at 21:17, Jeff King wrote:\n\n> We don't currently test this case at all, and instead just\n> test the DAV mechanism over an unauthenticated push. That\n> isn't very realistic, as most people will want to\n> authenticate pushes.\n\nThanks for adding this, Peff!\n\n-- \nSebastian Schuberth\n"},{"id":"181090","messageId":"20111213231040.GA12432@sigill.intra.peff.net","threadId":"29153","inReplyTo":"7v62hkuptt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T23:10:41Z","receivedAt":"2011-12-13T23:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 01:25:18PM -0800, Junio C Hamano wrote:\n\n> > @@ -844,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)\n> > \t\telse if (missing_target(&results))\n> > \t\t\tret = HTTP_MISSING_TARGET;\n> > \t\telse if (results.http_code == 401) {\n> > -\t\t\tif (user_name && user_pass) {\n> > +\t\t\tif (user_name) {\n> > \t\t\t\tret = HTTP_NOAUTH;\n> > \t\t\t} else {\n> > \t\t\t\t/*\n> \n> In the credential series, this is where we declare the given credential is\n> to be rejected (if we have both username and password), or ask them to be\n> filled by calling credential_fill(), so I think the code in the credential\n> series does not need this revert. Right?\n\nSort of. The point of this conditional is \"did we actually send a\ncredential? If yes, then return HTTP_NOAUTH. Otherwise, ask for the\ncredential and return HTTP_REAUTH\"[1].\n\nPrior to Stefan's patch, if user_name was set, then we sent a credential\n(because we always filled in the password if user_name was set). After,\nwe had to check both (actually, I think checking user_pass would have\nbeen sufficient)).\n\nThe situation is the same with credentials, but the variable name is\ndifferent. Even though credential_fill will fill in both parts, we may\nhave set http_auth.username from the URL, but not actually called\ncredential_fill (and therefore not sent any credentials).\n\nSo logically, the revert in the credential series would be:\n\n - if (http_auth.username && http_auth.password)\n + if (http_auth.username)\n\nexcept that I believe the former is a superset of the latter in both\ncases (with or without the credential topic). So leaving it as-is would\nbe OK. In fact, when reverting Stefan's patch, you could just drop this\nhunk entirely, which might be worth it to avoid conflicts with in-flight\ntopics.\n\n[1] A much saner approach would be to always return HTTP_NOAUTH, and\n then let the caller decide to re-ask for credentials and re-try.\n But we need the magic curl slot object, which the caller doesn't\n have. So doing it that way would have taken some refactoring.\n\n> > @@ -853,8 +855,7 @@ static int http_request(const char *url, void *result, int target, int options)\n> > \t\t\t\t * but that is non-portable. Using git_getpass() can at least be stubbed\n> > \t\t\t\t * on other platforms with a different implementation if/when necessary.\n> > \t\t\t\t */\n> > -\t\t\t\tif (!user_name)\n> > -\t\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n> > +\t\t\t\tuser_name = xstrdup(git_getpass_with_description(\"Username\", description));\n> > \t\t\t\tinit_curl_http_auth(slot->curl);\n> > \t\t\t\tret = HTTP_REAUTH;\n> > \t\t\t}\n> \n> So is this one.\n\nYeah, this code just goes away, as credential_fill() does the right\nthing.\n\nBut again, the \"if (!user_name)\" version post-986bbc08 handles both the\npre-986bbc08 condition (because it will always be NULL then) and the\npost-986bbc08 (because we do need to check then). So I believe you could\ndrop the hunk entirely.\n\nHere's a re-roll of the revert that touches as little as possible. I\nbelieve it's correct from my analysis above, and it does pass the tests.\nI also included the flipping of the \"expect_failure\" test, which I\nforgot to put in my original patch.\n\n-- >8 --\nSubject: [PATCH] Revert \"http: don't always prompt for password\"\n\nThis is a partial revert of commit 986bbc0842334f0e07731fa37f2a55d2930a5b8c.\n\nThe rationale for that commit relied on the fact that asking\nfor the password up-front was merely an optimization,\nbecause git will notice an HTTP 401 and prompt for the\npassword. However, that is only true for smart-http, and for\ndumb fetching. Dumb push over DAV does not have this\nfeature; as a result, authenticated push-over-DAV does not\nwork at all, as it never prompts the user for a password.\n\nThis is a partial revert that just restores the \"don't ask\nfor password bits\". There were some other related\nadjustments in 986bbc08 to handle the fact that the\nuser_name field might be set even if we didn't send a\ncredential on the first try. The new logic introduced by\n986bbc08 handles both the old case and the new case, so we\ncan leave them intact. That will prevent unnecessary\nconflicts with other in-flight topics that touch this code.\n\nSigned-off-by: Jeff King \n---\n http.c | 2 ++\n t/t5540-http-push.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 008ad72..33c63b5 100644\n--- a/http.c\n+++ b/http.c\n@@ -279,6 +279,8 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n #endif\n \n+\tinit_curl_http_auth(result);\n+\n \tif (ssl_cert != NULL)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n \tif (has_cert_password())\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 3300227..1eea647 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -160,7 +160,7 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '\n test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n \t\"$ROOT_PATH\"/test_repo_clone master\n \n-test_expect_failure 'push to password-protected repository (user in URL)' '\n+test_expect_success 'push to password-protected repository (user in URL)' '\n \ttest_commit pw-user &&\n \tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n \tgit rev-parse --verify HEAD >expect &&\n-- \n1.7.8.17.gfd3524\n"},{"id":"181091","messageId":"20111213231630.GB12432@sigill.intra.peff.net","threadId":"29153","inReplyTo":"CAHGBnuO1CFGacMQb+Md_uGsLu=V9bGDpcFLd9Lb4C5jSN+uUiw@mail.gmail.com","subject":"Re: [PATCH 1/2] t5540: test DAV push with authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T23:16:30Z","receivedAt":"2011-12-13T23:16:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 10:28:07PM +0100, Sebastian Schuberth wrote:\n\n> On Tue, Dec 13, 2011 at 21:17, Jeff King wrote:\n> \n> > We don't currently test this case at all, and instead just\n> > test the DAV mechanism over an unauthenticated push. That\n> > isn't very realistic, as most people will want to\n> > authenticate pushes.\n> \n> Thanks for adding this, Peff!\n\nYou're welcome. Thank you for forwarding the bug report. I would never\nhave seen it on the msysgit list, and for some reason it seems that\nmsysgit people are more likely to use DAV.\n\nHaving looked a lot at the http code the past month or two, I knew it\nwas pretty flaky and I was nervous when we added Stefan's patch (and no,\nI don't blame Stefan; his patch was completely reasonable, but just\nhappened to trigger a problem in a seldom-looked-at corner of the code).\n\nBut I hadn't looked at http-push at all until yesterday, and it didn't\neven occur to me that there was another whole area of code relying in a\nvery obscure way on the http.c auth code. I'll take a look at some of\nthe refactoring I've done in http.c (both for the credentials topic as\nwell as the bundle topic) and see if we can't integrate http-push.c a\nlittle more smoothly.\n\n-Peff\n"},{"id":"181092","messageId":"20111213231800.GC12432@sigill.intra.peff.net","threadId":"29153","inReplyTo":"CA+e6NDTQNEJB6Y17ow+yuLp2Bc799wsyQdnS8immMDYdCEyCGA@mail.gmail.com","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T23:18:00Z","receivedAt":"2011-12-13T23:18:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 02:22:12PM -0700, Eric Advincula wrote:\n\n> Is there an alternative to using git on windows? I used windows, apache,\n> dav for git.\n> If there is a better solution please let me know.\n\nI don't know the status of running the smart-http backend on Windows,\nbut that would be a preferable solution. It's way more efficient, and\nthe client half of the code is better maintained.\n\nSee \"git help http-backend\" for documentation.\n\n-Peff\n"},{"id":"181093","messageId":"20111213231909.GD12432@sigill.intra.peff.net","threadId":"29153","inReplyTo":"7vaa6wuqjt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T23:19:09Z","receivedAt":"2011-12-13T23:19:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 01:09:42PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Doing (3) is obviously the easiest thing. And given the complexity of\n> > the other two solutions, I think it makes sense to revert 986bbc08\n> > (i.e., apply this patch), ship a working v1.7.8.1, and then look at\n> > doing one of the other two solutions for v1.7.9.\n> \n> Or just let the \"dumb HTTP\" die.\n> \n> I thought push over DAV has long been dead; is anybody using it for real?\n\nFor the record, I have no problem whatsoever with letting it die. I just\nthink we probably shouldn't do it accidentally during a release. ;)\n\n-Peff\n"},{"id":"181094","messageId":"20111213232053.GE12432@sigill.intra.peff.net","threadId":"29153","inReplyTo":"20111213231909.GD12432@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-13T23:20:53Z","receivedAt":"2011-12-13T23:20:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 06:19:09PM -0500, Jeff King wrote:\n\n> > Or just let the \"dumb HTTP\" die.\n> > \n> > I thought push over DAV has long been dead; is anybody using it for real?\n> \n> For the record, I have no problem whatsoever with letting it die. I just\n> think we probably shouldn't do it accidentally during a release. ;)\n\nBTW, one other solution, rather than reverting Stefan's patch, is to\njust re-add the \"unconditionally ask for a password\" behavior back to\ngit-http-push, but not to the fetching side. Especially if we hope to\nkill off git-http-push soon (after a deprecation period presumably),\nthen that lets it work in the meantime without hurting the other http\ncode. And it's really easy to do.\n\n-Peff\n"},{"id":"181099","messageId":"20111214001156.GA21940@sigill.intra.peff.net","threadId":"29153","inReplyTo":"20111213232053.GE12432@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T00:11:56Z","receivedAt":"2011-12-14T00:11:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 06:20:53PM -0500, Jeff King wrote:\n\n> BTW, one other solution, rather than reverting Stefan's patch, is to\n> just re-add the \"unconditionally ask for a password\" behavior back to\n> git-http-push, but not to the fetching side. Especially if we hope to\n> kill off git-http-push soon (after a deprecation period presumably),\n> then that lets it work in the meantime without hurting the other http\n> code. And it's really easy to do.\n\nAnd here's what that patch looks like.\n\nThinking on it more, this is probably better for a maint release than\nreverting Stefan's patch. It un-breaks http-push, and they are no\ndifferently off than they are with the revert. But it leaves the\nenhancement in place for the smart-http code.\n\n-- >8 --\nSubject: [PATCH] http-push: enable \"proactive auth\"\n\nBefore commit 986bbc08, git was proactive about asking for\nhttp passwords. It assumed that if you had a username in\nyour URL, you would also want a password, and asked for it\nbefore making any http requests.\n\nHowever, this could interfere with the use of .netrc (see\n986bbc08 for details). And it was also unnecessary, since\nthe http fetching code had learned to recognize an HTTP 401\nand prompt the user then. Furthermore, the proactive prompt\ncould interfere with the usage of .netrc (see 986bbc08 for\ndetails).\n\nUnfortunately, the http push-over-DAV code never learned to\nrecognize HTTP 401, and so was broken by this change. This\npatch does a quick fix of re-enabling the \"proactive auth\"\nstrategy only for http-push, leaving the dumb http fetch and\nsmart-http as-is.\n\nSigned-off-by: Jeff King \n---\n http-fetch.c | 2 +-\n http-push.c | 2 +-\n http.c | 8 +++++++-\n http.h | 3 ++-\n remote-curl.c | 2 +-\n t/t5540-http-push.sh | 2 +-\n 6 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 69299b7..94d47cb 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -67,7 +67,7 @@ int main(int argc, const char **argv)\n \n \tgit_config(git_default_config, NULL);\n \n-\thttp_init(NULL, url);\n+\thttp_init(NULL, url, 0);\n \twalker = get_http_walker(url);\n \twalker->get_tree = get_tree;\n \twalker->get_history = get_history;\ndiff --git a/http-push.c b/http-push.c\nindex 5d01be9..70283e6 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1820,7 +1820,7 @@ int main(int argc, char **argv)\n \n \tmemset(remote_dir_exists, -1, 256);\n \n-\thttp_init(NULL, repo->url);\n+\thttp_init(NULL, repo->url, 1);\n \n #ifdef USE_CURL_MULTI\n \tis_running_queue = 0;\ndiff --git a/http.c b/http.c\nindex 008ad72..6c90092 100644\n--- a/http.c\n+++ b/http.c\n@@ -43,6 +43,7 @@\n static const char *curl_http_proxy;\n static const char *curl_cookie_file;\n static char *user_name, *user_pass, *description;\n+static int http_proactive_auth;\n static const char *user_agent;\n \n #if LIBCURL_VERSION_NUM >= 0x071700\n@@ -279,6 +280,9 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n #endif\n \n+\tif (http_proactive_auth)\n+\t\tinit_curl_http_auth(result);\n+\n \tif (ssl_cert != NULL)\n \t\tcurl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);\n \tif (has_cert_password())\n@@ -367,7 +371,7 @@ static void set_from_env(const char **var, const char *envname)\n \t\t*var = val;\n }\n \n-void http_init(struct remote *remote, const char *url)\n+void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n \tchar *low_speed_limit;\n \tchar *low_speed_time;\n@@ -378,6 +382,8 @@ void http_init(struct remote *remote, const char *url)\n \n \tcurl_global_init(CURL_GLOBAL_ALL);\n \n+\thttp_proactive_auth = proactive_auth;\n+\n \tif (remote && remote->http_proxy)\n \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n \ndiff --git a/http.h b/http.h\nindex 3c332a9..51f6ba7 100644\n--- a/http.h\n+++ b/http.h\n@@ -86,7 +86,8 @@ struct buffer {\n extern void step_active_slots(void);\n #endif\n \n-extern void http_init(struct remote *remote, const char *url);\n+extern void http_init(struct remote *remote, const char *url,\n+\t\t int proactive_auth);\n extern void http_cleanup(void);\n \n extern int data_received;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0e720ee..0757b19 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -859,7 +859,7 @@ int main(int argc, const char **argv)\n \n \turl = strbuf_detach(&buf, NULL);\n \n-\thttp_init(remote, url);\n+\thttp_init(remote, url, 0);\n \n \tdo {\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\ndiff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh\nindex 3300227..1eea647 100755\n--- a/t/t5540-http-push.sh\n+++ b/t/t5540-http-push.sh\n@@ -160,7 +160,7 @@ test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '\n test_http_push_nonff \"$HTTPD_DOCUMENT_ROOT_PATH\"/test_repo.git \\\n \t\"$ROOT_PATH\"/test_repo_clone master\n \n-test_expect_failure 'push to password-protected repository (user in URL)' '\n+test_expect_success 'push to password-protected repository (user in URL)' '\n \ttest_commit pw-user &&\n \tgit push \"$HTTPD_URL_USER/auth/dumb/test_repo.git\" HEAD &&\n \tgit rev-parse --verify HEAD >expect &&\n-- \n1.7.8.17.gfd3524\n"},{"id":"181104","messageId":"7v7h20t2iw.fsf@alter.siamese.dyndns.org","threadId":"29153","inReplyTo":"20111214001156.GA21940@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T00:33:59Z","receivedAt":"2011-12-14T00:33:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... This\n> patch does a quick fix of re-enabling the \"proactive auth\"\n> strategy only for http-push, leaving the dumb http fetch and\n> smart-http as-is.\n\nYeah, I somehow like this better. Thanks.\n"},{"id":"181117","messageId":"vpqr507d0o2.fsf@bauges.imag.fr","threadId":"29153","inReplyTo":"7vaa6wuqjt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"http: don't always prompt for password\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-12-14T08:20:45Z","receivedAt":"2011-12-14T08:20:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Doing (3) is obviously the easiest thing. And given the complexity of\n>> the other two solutions, I think it makes sense to revert 986bbc08\n>> (i.e., apply this patch), ship a working v1.7.8.1, and then look at\n>> doing one of the other two solutions for v1.7.9.\n>\n> Or just let the \"dumb HTTP\" die.\n>\n> I thought push over DAV has long been dead; is anybody using it for real?\n\nI am.\n\nI'm working with people behind a firewall, hence HTTP is mandantory. My\nlab has a webdav server, without Git installed on it. Being able to work\nwith this setup was one of the key feature of Git when I adopted it\n(after a few years using GNU Arch the same way).\n\nI could probably manage to convince my sysadmin to install Git on our\nwebserver, but I'd prefer if Git continues supporting my current setup.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}