Hi,
On Mon, Sep 7, 2009 at 3:10 PM, Junio C Hamano<gitster@xxxxxxxxx> wrote:
> We couldn't perform the check, and then what happens? We continue as if
> everything is peachy, assuming that the *.idx file we thought we were
> going to get describe what objects are in the corresponding pack, and barf
> when we try to read the *.idx file that we failed to download even though
> the server said we should be able to get it?
We didn't check for the *.idx, only the corresponding *.pack file.
About barfing on a failed *.idx file transfer: in 48188c2 and later, we
still do have ample checks here and there in the HTTP api. http_get_file()
returns a HTTP_ERROR if we fail to get a file (a *.idx file here).
The issue is that the check on the file (a HEAD request) fails, but not
its actual transfer (a GET request). There's no compromise on error
handling; we can live with a bad response to HEAD requests, but we will
still fail if we can't GET the file.
> Ahh, v1.6.3 ships with fetch_index() that checks CURLE_OK and returns an
> error(), but that is about .idx file, and it did not have the "do they
> have the corresponding .pack file?" check introduced by 48188c2
> (http-walker: verify remote packs, 2009-06-06), which is what makes the
> server give us 500 error.
Just to clarify: the "check" of CURLE_OK in http-walker.c::fetch_index()
in v1.6.3 is fundamentally different from the check we have in 48188c2
(http-walker: verify remote packs, 2009-06-06).
The first "check" is a full-blooded GET request, and we do get back
actual data (in this case, the pack index file). The second check isn't
a GET request, just an inconsequential HEAD request; we don't get back
any real data.
> Before that check, we ignored a request to
> fetch_index() if we already had one.
48188c2 didn't really remove the check (for the presence of the *.idx
locally), it just pushed it further down.
But being placed so late makes it rather ineffective and useless.
> Why do we even call fetch_index() when we have one? After all, we are
> talking about "git clone" here, so it is not about "we failed once and the
> previous attempt left .idx files we fetched". Why
>
> should we even have .idx file to begin with, that would have protected
> v1.6.3 clients from getting this error?
>
> Unless we are calling fetch_index() on the same .idx file twice from our
> own stupidity, that is.
>
> The same logic now is in fetch_pack_index(), which is called from
> fetch_and_setup_pack_index(). I do not still see how we end up calling
> the function for the same .idx file twice, though.
We should bump up this check before the verify-remote-pack one, I think
there's no mysterious bug here to find.
> In the meantime, I do not think it is a good idea to loosen the error
> checking on our side to accomodate a server in such a (hopefully
> temporary) broken state, however popular it is.
Agreed. I didn't intend my patch to loosen up error checking, merely to
be clearer about what we're looking for. If read in another context
(separate from fixing cloning over github.com), my patch can be seen as
one that clarifies the verify-remote-pack check:
Case 1: A 404 is received. The pack file is not available, so we stop.
Case 2: Our check failed, due to some reason (request failed,
unauthorized, etc). Nothing conclusive about availabilty of
file. Continue anyway.
PS. I wrote 48188c2 with the aim of having http-push.c and http-walker.c
fetch behaviour match each other as closely as possible, and later move
these out into http. At that time, I toyed with the idea of removing
the check. I wondered if the overhead incurred by the HEAD request was
useful, since if there's going to be an error, we would be hit anyway
with a GET. We already do this for almost every other http request
(info/refs, objects/info/packs, etc). But the patch series was meant to
be a refactor job, not a feature/behaviour change, so there you have it.
--
Cheers,
Ray Chuan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html