Re: [PATCH 2/2] http: do not set up curl auth after a 401

[Brad Hein <linuxbrad@gmail.com>]

Nice work sorting this out. I don't see the commit on github
(https://github.com/git/git/commits/master) yet but once the code is
available I'll be happy to re-test if needed.


On Fri, Oct 12, 2012 at 3:35 AM, Jeff King <peff@peff.net> wrote:
> When we get an http 401, we prompt for credentials and put
> them in our global credential struct. We also feed them to
> the curl handle that produced the 401, with the intent that
> they will be used on a retry.
>
> When the code was originally introduced in commit 42653c0,
> this was a necessary step. However, since dfa1725, we always
> feed our global credential into every curl handle when we
> initialize the slot with get_active_slot. So every further
> request already feeds the credential to curl.
>
> Moreover, accessing the slot here is somewhat dubious. After
> the slot has produced a response, we don't actually control
> it any more.  If we are using curl_multi, it may even have
> been re-initialized to handle a different request.
>
> It just so happens that we will reuse the curl handle within
> the slot in such a case, and that because we only keep one
> global credential, it will be the one we want.  So the
> current code is not buggy, but it is misleading.
>
> By cleaning it up, we can remove the slot argument entirely
> from handle_curl_result, making it much more obvious that
> slots should not be accessed after they are marked as
> finished.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http.c        | 6 ++----
>  http.h        | 3 +--
>  remote-curl.c | 2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/http.c b/http.c
> index 9334386..0a74345 100644
> --- a/http.c
> +++ b/http.c
> @@ -744,8 +744,7 @@ char *get_remote_object_url(const char *url, const char *hex,
>         return strbuf_detach(&buf, NULL);
>  }
>
> -int handle_curl_result(struct active_request_slot *slot,
> -                      struct slot_results *results)
> +int handle_curl_result(struct slot_results *results)
>  {
>         if (results->curl_result == CURLE_OK) {
>                 credential_approve(&http_auth);
> @@ -758,7 +757,6 @@ int handle_curl_result(struct active_request_slot *slot,
>                         return HTTP_NOAUTH;
>                 } else {
>                         credential_fill(&http_auth);
> -                       init_curl_http_auth(slot->curl);
>                         return HTTP_REAUTH;
>                 }
>         } else {
> @@ -817,7 +815,7 @@ static int http_request(const char *url, void *result, int target, int options)
>
>         if (start_active_slot(slot)) {
>                 run_active_slot(slot);
> -               ret = handle_curl_result(slot, &results);
> +               ret = handle_curl_result(&results);
>         } else {
>                 error("Unable to start HTTP request for %s", url);
>                 ret = HTTP_START_FAILED;
> diff --git a/http.h b/http.h
> index 0bd1e84..0a80d30 100644
> --- a/http.h
> +++ b/http.h
> @@ -78,8 +78,7 @@ extern void finish_all_active_slots(void);
>  extern void run_active_slot(struct active_request_slot *slot);
>  extern void finish_active_slot(struct active_request_slot *slot);
>  extern void finish_all_active_slots(void);
> -extern int handle_curl_result(struct active_request_slot *slot,
> -                             struct slot_results *results);
> +extern int handle_curl_result(struct slot_results *results);
>
>  #ifdef USE_CURL_MULTI
>  extern void fill_active_slots(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 4281262..aefafd3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
>         slot->curl_result = curl_easy_perform(slot->curl);
>         finish_active_slot(slot);
>
> -       err = handle_curl_result(slot, &results);
> +       err = handle_curl_result(&results);
>         if (err != HTTP_OK && err != HTTP_REAUTH) {
>                 error("RPC failed; result=%d, HTTP code = %ld",
>                       results.curl_result, results.http_code);
> --
> 1.8.0.rc2.3.g303f317