Re: Issue 323 in msysgit: Can't clone over http

[Junio C Hamano <gitster@pobox.com>]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Subject: [PATCH] http.c: clarify missing-pack-check
>
> Abort the pack transfer only if the pack is not available in the HTTP-
> served repository; otherwise, allow the transfer to continue, even if
> the check failed.
>
> This addresses an issue raised by bjelli:
>
>   http://code.google.com/p/msysgit/issues/detail?id=323
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  http.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/http.c b/http.c
> index 5926c5b..cba7e9a 100644
> --- a/http.c
> +++ b/http.c
> @@ -864,6 +864,7 @@ int http_fetch_ref(const char *base, struct ref *ref)
>  static int fetch_pack_index(unsigned char *sha1, const char *base_url)
>  {
>  	int ret = 0;
> +	int result;
>  	char *hex = xstrdup(sha1_to_hex(sha1));
>  	char *filename;
>  	char *url;
> @@ -874,11 +875,14 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
>  	strbuf_addf(&buf, "objects/pack/pack-%s.pack", hex);
>  	url = strbuf_detach(&buf, 0);
>
> -	if (http_get_strbuf(url, NULL, 0)) {
> -		ret = error("Unable to verify pack %s is available",
> +	result = http_get_strbuf(url, NULL, 0);
> +	if (result == HTTP_MISSING_TARGET) {
> +		ret = error("Unable to find pack %s",
>  			    hex);
>  		goto cleanup;
> -	}
> +	} else if (result && http_is_verbose)
> +		fprintf(stderr, "Unable to verify pack %s is available\n",
> +			hex);
>
>  	if (has_pack_index(sha1)) {
>  		ret = 0;

You said:

> Releases including and after v1.6.4 will have this issue:
>
>>> error: Unable to verify pack 382c25c935b744e909c749532578112d72a4aff9 is
>>> available
>>> error: Unable to find 0a41ac04d56ccc96491989dc71d9875cd804fc6b under
>>> http://github.com/tekkub/addontemplate.git
>
> The issue at hand is due to git checking the http repository for the
> pack file before commencing the transfer; failing which, the transfer
> aborts.
>
> Right now, git chokes on the 500 error that github.com gives it, which
> shouldn't be the case, even though that's a weird response.

I am assuming that you meant 39dc52c was the culprit by "including and
after v1.6.4", but it is not quite clear how this patch helps if that is
the case.

Before 39dc52c (http: use new http API in fetch_index(), 2009-06-06), we
used to run the slot by hand and checked results.curl_request against
CURLE_OK.  Everything else was an error.

With the updated code, that all went to http_get_strbuf() which in turn
calls http_request() that does the same thing, and the function returns
HTTP_OK only when it gets CURLE_OK, but now it says MISSING_TARGET when we
ask for an idx file we think exists in the repository but the server says
it doesn't, and all other errors will be ignored with this patch.

If this codepath is what was broken by github returning 500 [*1*], the
client before 39dc52c would have failed the same way.  I do not think
loosening error checking like this is a real solution, but I may be
reading the patch incorrectly.

Do people on the github side see something strange in the log?  Perhaps we
think we are making a request to objects/pack/ of the repository but by
mistake the request is going to somewhere completely off (but then we
would get 401 not 500).


[Footnote]

*1* Which I do agree is somewhat strange thing to say for a request to a
file in the objects/pack directory in a public repository---I would
understand it if it were 404, but then it means the repository is
inconsistent, i.e. has a stale objects/info/packs relative to its set of
packs it has.