Re: Strangely broken git repo

"Martin Langhoff (CatalystIT)" [off-list ref] writes:

$ cg-clone http://locke.catalyst.net.nz/git/moodle.git#moodle--topnz
...
Getting index for pack 3e3492f365bb0d4a1ae11dfa7cee9ebbf345e647
Getting pack 3113eb34ef85482c87c3575721ce978c7232071f
which contains 007a0323cf0941476dc262f6e3aff6bb9600dcd8
error: The requested file was not found
... (many more like these)

So it failed to grab 007a0323cf0941476dc262f6e3aff6bb9600dcd8,
but that is OK.  That object is in a pack:
pack-3113eb34ef85482c87c3575721ce978c7232071f has it.

FINISHED --15:56:38--
Downloaded: 4,723 bytes in 1 files
New branch: 6759e2800c0cef00017c63b7dbbed80e481dbe2c
Cloned to moodle.git#moodle--topnz/ (origin
http://locke.catalyst.net.nz/git/moodle.git#moodle--topnz available as
branch "origin")
aporo:~/tmp/moodle.git#moodle--topnz martin$ git-fsck-objects
bad sha1 file: .git/objects/00/7a0323cf0941476dc262f6e3aff6bb9600dcd8.temp
... (many more like these)

This *.temp file is what git-http-fetch creates while retrieving
the object.  After successfully downloading it, it is renamed to
its final name (i.e. sans .temp), but that rename did not
happen.  Well, this object is in a pack, so after failing to
retrieve that, we should have got the list of packs, retrieved
pack index files, and grabbed the pack that contained that
object.  We *should* clean up the *.temp file for this object
whe we do this, but I do not think we do.  But I do not think
this is the _cause_ of your problem.  It is a symptom that
something is going wrong -- namely, the question is why it was
not found in pack-3113eb34ef85482c87c3575721ce978c7232071f.  Did
the client download that pack?

I just tried to clone your repository with 'git clone', and saw
something failing, but I think the object trasfer went fine all
the way down to the initial commit.  I can see this object in my
cloned repository:

        commit 34cc394a7427adf730cfd3fb482dc6d6d5b58775
        Author: Martin Langhoff [off-list ref]
        Date:   Tue Aug 3 02:50:51 2004 +0000

            initial import

There are funky tag names in your refs/tags directory.  The
really core tools should be able to grok them just fine, but
git-http-fetch failed with this from curl library:

The requested URL returned error: 404
error: Couldn't get http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED **INVALID** for tags/MOODLE_15_MERGED **INVALID**

I do not speak curl, but I wonder if we should be quoting
these funky characters like SP and asterisk in the URL when we
make that request, or it is what the library does for us.

Hmph.  Interesting.  I just tried.

$ curl 'http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED **INVALID**'

gives an error page "404 Not Found", while

$ wget -O - -o /dev/null 'http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED **INVALID**'

works fine and gives 2ddfec0dfd0cffd4892af9aaf48ee29c40c7ada3
back.  So we do need to fix things up somewhat in our scripts as
well.

Anyway, I think I know the problems 'git-clone' would have had
if you tried to clone it with it (not cg-clone which I do not
know much about), and luckily it is only towards the end (after
fetching most of the heads, but hitting the first funky tag).
We should be able to fix this relatively easily.

Oh, Daniel and Nick, while I was reading the http-fetch.c code,
I noticed another thing.

The fetch_alternates() function was supposed to be call-once and
we have a static variable got_alternates that becomes 1 when it
runs for the first time.  However, there are other 'return 0's
introduced that does not set got_alternates to 1.

I am wondering if the semantics has changed that now we chain
the alternates?  Initially, if we are cloning/fetching from
repository A, which borrows from repository B (i.e. alternates
we retrieve from A would name B), and if B in turn borrows from
C, then we assumed that A's alternates would also name C, and
that was the reason why fetch_alternates() was call-once
function.  I do not mind if we change it to chain the alternates
file, but if that is the case we should move the got_alternates
variable into "struct alt_base", and pass the struct, not just
alt->base, to fetch_alternates(), like this (untested, of
course):

diff --git a/http-fetch.c b/http-fetch.c
index 5821c9e..2301f88 100644
--- a/http-fetch.c
+++ b/http-fetch.c

@@ -38,6 +38,7 @@ struct alt_base
 {
 	char *base;
 	int got_indices;
+	int got_alternates;
 	struct packed_git *packs;
 	struct alt_base *next;
 };

@@ -529,8 +530,6 @@ void prefetch(unsigned char *sha1)
 #endif
 }
 
-static int got_alternates = 0;
-
 static int fetch_index(struct alt_base *repo, unsigned char *sha1)
 {
 	char *hex = sha1_to_hex(sha1);

@@ -611,8 +610,9 @@ static int setup_index(struct alt_base *
 	return 0;
 }
 
-static int fetch_alternates(char *base)
+static int fetch_alternates(struct alt_base *this_alt)
 {
+	char *base = this_alt->base;
 	int ret = 0;
 	struct buffer buffer;
 	char *url;

@@ -622,15 +622,17 @@ static int fetch_alternates(char *base)
 	struct alt_base *tail = alt;
 
 	struct active_request_slot *slot;
-	if (got_alternates)
+	if (this_alt->got_alternates)
 		return 0;
+	this_alt->got_alternates = 1;
 	data = xmalloc(4096);
 	buffer.size = 4095;
 	buffer.posn = 0;
 	buffer.buffer = data;
 
 	if (get_verbosely)
-		fprintf(stderr, "Getting alternates list\n");
+		fprintf(stderr, "Getting alternates list for %s\n",
+			base);
 	
 	url = xmalloc(strlen(base) + 31);
 	sprintf(url, "%s/objects/info/http-alternates", base);

@@ -721,7 +723,6 @@ static int fetch_alternates(char *base)
 		}
 		i = posn + 1;
 	}
-	got_alternates = 1;
 	
 	return ret;
 }

@@ -1092,9 +1093,10 @@ int main(int argc, char **argv)
 	alt = xmalloc(sizeof(*alt));
 	alt->base = url;
 	alt->got_indices = 0;
+	alt->got_alternates = 0;
 	alt->packs = NULL;
 	alt->next = NULL;
-	fetch_alternates(alt->base);
+	fetch_alternates(alt);
 
 	if (pull(commit_id))
 		return 1;

On Mon, 10 Oct 2005, Junio C Hamano wrote:

Hmph.  Interesting.  I just tried.

$ curl 'http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED **INVALID**'

gives an error page "404 Not Found", while

$ wget -O - -o /dev/null 'http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED **INVALID**'

works fine and gives 2ddfec0dfd0cffd4892af9aaf48ee29c40c7ada3
back.  So we do need to fix things up somewhat in our scripts as
well.

It seems to be the space. Doing

  $ curl 'http://locke.catalyst.net.nz/git/moodle.git/refs/tags/MOODLE_15_MERGED%20**INVALID**'

works ok (ie %20 instead of ' ').

As far as I can tell, we should probably _also_ quote any curl-specific 
stuff. As far as I can tell from the manual, if the tag were to have 
special characters like '[' and '{', curl might confuse them with being 
range specifiers.

		Linus

On Mon, 10 Oct 2005, Linus Torvalds wrote:

It seems to be the space.

Doing an strace on curl vs wget shows that curl seems to do no quoting at 
all. I'd personally argue that that is a serious bug in curl: it sure as 
hell knows that it's a http transport, and it seems to be just doing

	GET %s HTTP/1.0\r\nUser-agent:...

without any sanity checking at all.

		Linus

Spaces in URLs are off-spec.  (And common, go figure.)

M.

On Mon, 10 Oct 2005, Morten Welinder wrote:

Spaces in URLs are off-spec.  (And common, go figure.)

They may be off-spec in url's, but that doesn't mean that "curl" should 
just ignore them. It should either escape them, or refuse them. Using 
user-supplied data without any checks is usually a really bad idea.

			Linus

Hi,

On Mon, 10 Oct 2005, Morten Welinder wrote:

Spaces in URLs are off-spec.  (And common, go figure.)

%20

Hth,
Dscho

On Mon, Oct 10, 2005 at 02:00:06AM -0700, Junio C Hamano wrote:

The fetch_alternates() function was supposed to be call-once and
we have a static variable got_alternates that becomes 1 when it
runs for the first time.  However, there are other 'return 0's
introduced that does not set got_alternates to 1.

Oops, thanks for catching that - I'll fix it.

I am wondering if the semantics has changed that now we chain
the alternates?  Initially, if we are cloning/fetching from
repository A, which borrows from repository B (i.e. alternates
we retrieve from A would name B), and if B in turn borrows from
C, then we assumed that A's alternates would also name C, and
that was the reason why fetch_alternates() was call-once
function.  I do not mind if we change it to chain the alternates
file, but if that is the case we should move the got_alternates
variable into "struct alt_base", and pass the struct, not just
alt->base, to fetch_alternates(), like this (untested, of
course):

I'd thought about chaining the alternates, but wasn't sure whether it's
safe to consider all alternates in the chain equivalent.  I would want to
implement fetching the chain such that it could happen parallel to other
request and not block the pull.  I think it would also be useful to track
the network performance of each alternate and prefer faster sources as
new requests are started.

-- 
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.

Nick Hengeveld [off-list ref] writes:

Oops, thanks for catching that - I'll fix it.

That's OK.  It turns out that fetch_alternates() function itself
is now called from the main just once; it used to be called
lazily and needed the call-once guard.

I'd thought about chaining the alternates, but wasn't sure whether it's
safe to consider all alternates in the chain equivalent.

Well, I decided we shouldn't, because the original repository
that has objects/info/alternates surely doesn't.