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

On Tue, Dec 13, 2011 at 01:25:18PM -0800, Junio C Hamano wrote:

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.

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(-)