Re: [RFC/PATCH] http-push: don't always prompt for password

From: Jeff King <peff@peff.net>
Date: Nov 2, 2011, 17:23 UTC
Message-ID: <20111102172310.GA28525@sigill.intra.peff.net>
In-Reply-To: <7vk47ijvlv.fsf@alter.siamese.dyndns.org>

On Wed, Nov 02, 2011 at 10:13:32AM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> This defers the calls to git_getpass* until we get 401 from the server
> side.
> 
> I am guessing the reason why in the current code get_curl_handle() has a
> call to init_curl_http_auth() very early, but only when user_name is set,
> is because it is likely for a site to require authentication when the user
> already has "username@" in its URL, and doing it this way will avoid the
> extra round-trip because by the time we make an HTTP request, we have both
> name and pass. If we apply this patch, the check in init_curl_http_auth()
> that asks for the password only when user_name is set becomes unnecessary.

Yeah, that was my reading, as well. I was tempted to do away with it, as it makes the code much simpler, at the cost of doing that extra round-trip. However, most browsers do that round-trip, and I don't think it's that big a deal.

In the end I decided not to switch it just because I wanted to be as minimally invasive as possible, just in case somebody did care about the round trip.

Show 9 quoted lines

> I think the second hunk at l.846 sort of makes sense, but not quite.
> 
> "We got 401 even though we know we have supplied name and pass" is a valid
> criterion to decide that the name/pass is an invalid combination. But it
> makes me wonder if this code in its early days guaranteed whenever we have
> user_name we always have made sure we have user_pass (otherwise by asking
> for it with git_getpass) and that is the reason why it had to check only
> for user_name, and if that is the case perhaps the real breakage is we are
> not keeping that guarantee in the current code?

All of my patches attempted to keep that condition, as well (because credential_fill tries to do so). But I didn't think about it during the re-roll of the patches that are in master now, so maybe that wasn't kept.

It seems to me that Stefan's patch actually causes that (because he removes the early "set password if we have a username" logic). But I'll take another look at what's in master.

-Peff