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

From: Tay Ray Chuan <rctay89@gmail.com>
Date: Sep 9, 2009, 12:33 UTC
Message-ID: <20090909203350.e3d7e5dc.rctay89@gmail.com>
In-Reply-To: <be6fef0d0909080610j89c0a4bkf1cb0119b9b11db@mail.gmail.com>

Hi,

On Tue, Sep 8, 2009 at 9:10 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:

Show 15 quoted lines

> On Tue, Sep 8, 2009 at 3:06 AM, Junio C Hamano<gitster@pobox.com> wrote:
>> I am torn about this.
>>
>> On one hand, if we are going to treat such a half failure as nothing
>> conclusive, I do not see a point to keep that check to begin with.
>>
>> On the other hand, if a HEAD request to a URL results in an unauthorized,
>> what plausible excuse the server operator could give for allowing a GET
>> request to the same URL?  If we are going to keep the check if *.pack that
>> corresponds to the *.idx will be available, shouldn't we trust whatever
>> check we perform?
>
> I am in favour or removing this check, not just due to its
> unreliability, but for the sake of consistency (we very rarely send a
> HEAD request to poll data before doing a GET).

my patch below is in response to earlier deliberation.

I still think github.com should look into this issue (of differing responses for HEAD and GET requests).

 -- >8 --

Subject: [PATCH] http.c: remove verification of remote packs

Make http.c::fetch_pack_index() no longer check for the remote pack with a HEAD request before fetching the corresponding pack index file.

Not only does sending a HEAD request before we do a GET incur a performance penalty, it does not offer any significant error- prevention advantages (pack fetching in the *_http_pack_request() methods is capable of handling any errors on its own).

This addresses an issue raised elsewhere:

  http://code.google.com/p/msysgit/issues/detail?id=323
  http://support.github.com/discussions/repos/957-cant-clone-over-http-or-git

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Junio, I'm not sure if the credits and references ("This addresses...") should be included, since the patch doesn't look like it's fixing any thing, even though it is a response to an acknowledged issue.

Please remove those lines if you so wish.

 http.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/http.c b/http.c
index 5926c5b..84def9f 100644
--- a/http.c
+++ b/http.c
@@ -869,17 +869,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	char *url;
 	struct strbuf buf = STRBUF_INIT;

-	/* Don't use the index if the pack isn't there */
-	end_url_with_slash(&buf, 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",
-			    hex);
-		goto cleanup;
-	}
-
 	if (has_pack_index(sha1)) {
 		ret = 0;
 		goto cleanup;
--
1.6.4.2