Re: [PATCH] http: attempt updating base URL only if no error

On Mon, Feb 27, 2017 at 06:53:11PM -0800, Jonathan Tan wrote:

http.c supports HTTP redirects of the form

  http://foo/info/refs?service=git-upload-pack
  -> http://anything
  -> http://bar/info/refs?service=git-upload-pack

(that is to say, as long as the Git part of the path and the query
string is preserved in the final redirect destination, the intermediate
steps can have any URL). However, if one of the intermediate steps
results in an HTTP exception, a confusing "unable to update url base
from redirection" message is printed instead of a Curl error message
with the HTTP exception code.

Right, your patch makes sense. A real HTTP error should take precedence
over the url-update trickery.

Acked-by: Jeff King <redacted>

Therefore, teach http to check the HTTP status before attempting to
check if only the "base" part of the URL differed. This commit teaches
http_request_reauth to return early without updating options->base_url
upon an error; the only invoker of this function that passes a non-NULL
"options" is remote-curl.c (through "http_get_strbuf"), which only uses
options->base_url for an informational message in the situations that
this commit cares about (that is, when the return value is not HTTP_OK).

Running your included test, we get:

  fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The
  requested URL returned error: 502

but the error really happened in the intermediate step. I wonder if we
should show the effective_url in that case, as it's more likely to
pinpoint the problem. OTOH, we do not mention the intermediate redirect
at all, so they might be confused about where that URL came from. If you
really want to debug HTTP confusion, you should use GIT_TRACE_CURL.

At any rate, that's orthogonal to your patch, which is obviously the
right thing to do.

-Peff