Re: git-http-pull broken in latest git

Petr Baudis [off-list ref] writes:

$ git-cat-file commit bf570303153902ec3d85570ed24515bcf8948848 | grep tree
tree 41f10531f1799bbb31a1e0f7652363154ce96f45
$ git-read-tree 41f10531f1799bbb31a1e0f7652363154ce96f45
fatal: failed to unpack tree object 41f10531f1799bbb31a1e0f7652363154ce96f45

Kaboom. I think the issue might be that the reference dependency tree
building is broken and it should've pulled the other pack as well.

Last time I checked, git-http-pull did not utilize the pack
dependency information, which indeed is wrong.  When it decides
to fetch a pack instead of an asked-for object, it should check
which commits the pack expects to have in your local repository
and add them to its list of things to slurp.

A good news is that "git clone" as a whole works fine.

    prompt$ cd /var/tmp/
    prompt$ rm -fr junk
    prompt$ git clone http://www.kernel.org/pub/scm/git/git.git junk
    defaulting to local storage area
    prompt$ cd junk
    prompt$ git-cat-file commit bf570303153902ec3d85570ed24515bcf8948848 |
            grep tree
    tree 41f10531f1799bbb31a1e0f7652363154ce96f45
    prompt$ git-read-tree 41f10531f1799bbb31a1e0f7652363154ce96f45
    prompt$ /bin/ls .git/objects/pack
    pack-37cba29d3df65b160afabe769470f7857f98d729.idx
    pack-37cba29d3df65b160afabe769470f7857f98d729.pack
    pack-3c5133604508466855453f3e609428f4bbba9131.idx
    pack-3c5133604508466855453f3e609428f4bbba9131.pack
    prompt$ 

On Thu, 11 Aug 2005, Junio C Hamano wrote:

Petr Baudis [off-list ref] writes:

quoted

$ git-cat-file commit bf570303153902ec3d85570ed24515bcf8948848 | grep tree
tree 41f10531f1799bbb31a1e0f7652363154ce96f45
$ git-read-tree 41f10531f1799bbb31a1e0f7652363154ce96f45
fatal: failed to unpack tree object 41f10531f1799bbb31a1e0f7652363154ce96f45

quoted

Kaboom. I think the issue might be that the reference dependency tree
building is broken and it should've pulled the other pack as well.

Last time I checked, git-http-pull did not utilize the pack
dependency information, which indeed is wrong. 

Is there documentation on the format?

When it decides to fetch a pack instead of an asked-for object, it 
should check which commits the pack expects to have in your local 
repository and add them to its list of things to slurp.

It should work anyway, except that I messed up some logic in the parallel 
pull stuff; when it finds it has something already, it ignores it 
entirely, rather than processing it. The following patch fixes this.
---
[PATCH] Fix parallel pull dependancy tracking.

It didn't refetch an object it already had (good), but didn't process
it, either (bad). Synchronously process anything you already have.

Signed-off-by: Daniel Barkalow <redacted>
---

 pull.c |   57 ++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

9b6b4b259c6b00d5b2502c158bc800d7623352bc

diff --git a/pull.c b/pull.c
--- a/pull.c
+++ b/pull.c

@@ -98,12 +98,38 @@ static int process_tag(struct tag *tag)
 static struct object_list *process_queue = NULL;
 static struct object_list **process_queue_end = &process_queue;
 
-static int process(unsigned char *sha1, const char *type)
+static int process_object(struct object *obj)
 {
-	struct object *obj;
-	if (has_sha1_file(sha1))
+	if (obj->type == commit_type) {
+		if (process_commit((struct commit *)obj))
+			return -1;
+		return 0;
+	}
+	if (obj->type == tree_type) {
+		if (process_tree((struct tree *)obj))
+			return -1;
 		return 0;
-	obj = lookup_object_type(sha1, type);
+	}
+	if (obj->type == blob_type) {
+		return 0;
+	}
+	if (obj->type == tag_type) {
+		if (process_tag((struct tag *)obj))
+			return -1;
+		return 0;
+	}
+	return error("Unable to determine requirements "
+		     "of type %s for %s",
+		     obj->type, sha1_to_hex(obj->sha1));
+}
+
+static int process(unsigned char *sha1, const char *type)
+{
+	struct object *obj = lookup_object_type(sha1, type);
+	if (has_sha1_file(sha1)) {
+		/* We already have it, so we should scan it now. */
+		return process_object(obj);
+	}
 	if (object_list_contains(process_queue, obj))
 		return 0;
 	object_list_insert(obj, process_queue_end);

@@ -134,27 +160,8 @@ static int loop(void)
 			return -1;
 		if (!obj->type)
 			parse_object(obj->sha1);
-		if (obj->type == commit_type) {
-			if (process_commit((struct commit *)obj))
-				return -1;
-			continue;
-		}
-		if (obj->type == tree_type) {
-			if (process_tree((struct tree *)obj))
-				return -1;
-			continue;
-		}
-		if (obj->type == blob_type) {
-			continue;
-		}
-		if (obj->type == tag_type) {
-			if (process_tag((struct tag *)obj))
-				return -1;
-			continue;
-		}
-		return error("Unable to determine requirements "
-			     "of type %s for %s",
-			     obj->type, sha1_to_hex(obj->sha1));
+		if (process_object(obj))
+			return -1;
 	}
 	return 0;
 }

Dear diary, on Fri, Aug 12, 2005 at 01:21:46AM CEST, I got a letter
where Junio C Hamano [off-list ref] told me that...

Petr Baudis [off-list ref] writes:

quoted

$ git-cat-file commit bf570303153902ec3d85570ed24515bcf8948848 | grep tree
tree 41f10531f1799bbb31a1e0f7652363154ce96f45
$ git-read-tree 41f10531f1799bbb31a1e0f7652363154ce96f45
fatal: failed to unpack tree object 41f10531f1799bbb31a1e0f7652363154ce96f45

quoted

Kaboom. I think the issue might be that the reference dependency tree
building is broken and it should've pulled the other pack as well.

Last time I checked, git-http-pull did not utilize the pack
dependency information, which indeed is wrong.  When it decides
to fetch a pack instead of an asked-for object, it should check
which commits the pack expects to have in your local repository
and add them to its list of things to slurp.

A good news is that "git clone" as a whole works fine.

Yes, but cg-clone doesn't - it naively depended on the core git tools
actually, er.. working. ;-)

This became a nightmare to me by now - on two machines I tried to pull
to over HTTP, that failed miserably, and I got stuck until I applied
Daniel's patch there (and cleaned up after previous git-http-pulls).

So I have this packless git-pb repository and suspecting no evil, I pull
from you (thankfully I have .git/objects/pack there from some historical
pulls). I do a merge commit:

	packed
	 ... J
	packed \
		 > M
	       /
	 ... P

Now I want to pull on another machine. That pulls M and then fails since
I have no .git/objects/pack there, bummer. So I mkdir it, but get no
further w/o Daniel's patch - for git-*-pull, J is missing and that's it.
So I apply the patch, and get friendly

	error: Unable to determine requirements of type (null) for M

and only after I delete M from the database, I finally succeed with
git-http-pull. (That was with --repair.) That's not good since this
might occur even naturally when the pull is interrupted.

With git-ssh-pull, the situation is even more vexing - it refuses to
fetch the packs for some reason yet unknown to me (I will debug it
tomorrow).

The git-*-pull tools appear yet rather fragile. :/

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
If you want the holes in your knowledge showing up try teaching
someone.  -- Alan Cox

On Fri, 12 Aug 2005, Petr Baudis wrote:

Dear diary, on Fri, Aug 12, 2005 at 01:21:46AM CEST, I got a letter
where Junio C Hamano [off-list ref] told me that...

quoted

Petr Baudis [off-list ref] writes:

quoted

$ git-cat-file commit bf570303153902ec3d85570ed24515bcf8948848 | grep tree
tree 41f10531f1799bbb31a1e0f7652363154ce96f45
$ git-read-tree 41f10531f1799bbb31a1e0f7652363154ce96f45
fatal: failed to unpack tree object 41f10531f1799bbb31a1e0f7652363154ce96f45

quoted

Kaboom. I think the issue might be that the reference dependency tree
building is broken and it should've pulled the other pack as well.

Last time I checked, git-http-pull did not utilize the pack
dependency information, which indeed is wrong.  When it decides
to fetch a pack instead of an asked-for object, it should check
which commits the pack expects to have in your local repository
and add them to its list of things to slurp.

A good news is that "git clone" as a whole works fine.

Yes, but cg-clone doesn't - it naively depended on the core git tools
actually, er.. working. ;-)

This became a nightmare to me by now - on two machines I tried to pull
to over HTTP, that failed miserably, and I got stuck until I applied
Daniel's patch there (and cleaned up after previous git-http-pulls).

So I have this packless git-pb repository and suspecting no evil, I pull
from you (thankfully I have .git/objects/pack there from some historical
pulls). I do a merge commit:

	packed
	 ... J
	packed \
		 > M
	       /
	 ... P

Now I want to pull on another machine. That pulls M and then fails since
I have no .git/objects/pack there, bummer. So I mkdir it, but get no
further w/o Daniel's patch - for git-*-pull, J is missing and that's it.
So I apply the patch, and get friendly

	error: Unable to determine requirements of type (null) for M

and only after I delete M from the database, I finally succeed with
git-http-pull. (That was with --repair.) That's not good since this
might occur even naturally when the pull is interrupted.

Insufficient testing on my part; patch at the end.

With git-ssh-pull, the situation is even more vexing - it refuses to
fetch the packs for some reason yet unknown to me (I will debug it
tomorrow).

git-ssh-pull doesn't deal in packs; it gets individual objects out of 
packs, which git-ssh-push (on the remote side) should be extracting. 
Perhaps you have a git-ssh-push on the remote side that's before I make 
packs work (it used to need to have the files for objects it was sending). 

At some point, I have to revisit getting git-ssh-* to generate exactly the 
required pack and transfer that, but that's an efficiency issue, not a 
correctness one, and shouldn't be relevant to the problem you're having.

---
[PATCH] Also parse objects we already have

In the case where we don't know from context what type an object is, but
we don't have to fetch it, we need to parse it to determine the type
before processing it.

Signed-off-by: Daniel Barkalow <redacted>
---

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

b8c382e76da25f45ff86176a6a6affdd9a28d60b

diff --git a/pull.c b/pull.c
--- a/pull.c
+++ b/pull.c

@@ -127,6 +127,7 @@ static int process(unsigned char *sha1, 
 {
 	struct object *obj = lookup_object_type(sha1, type);
 	if (has_sha1_file(sha1)) {
+		parse_object(sha1);
 		/* We already have it, so we should scan it now. */
 		return process_object(obj);
 	}

Daniel Barkalow [off-list ref] writes:

Petr Baudis [off-list ref] writes:

quoted

Yes, but cg-clone doesn't - it naively depended on the core git tools
actually, er.. working. ;-)

Sorry about that.  I used to have a wrapper to deal with packs
around http-pull before Daniel's pack enhancement, and yanking
it before really checking that enhanced http-pull actually
worked was my fault as well.

quoted

This became a nightmare to me by now - on two machines I tried to pull
to over HTTP, that failed miserably, and I got stuck until I applied
Daniel's patch there (and cleaned up after previous git-http-pulls).

I'll push one out with two patches from Daniel today in short
order.  Currently running the final "make test" round.

At some point, I have to revisit getting git-ssh-* to generate exactly the 
required pack and transfer that, but that's an efficiency issue, not a 
correctness one, and shouldn't be relevant to the problem you're having.

Wouldn't enhancing ssh-push to generate packs on the fly involve
reinventing send-pack and/or upload-pack?

If send-pack/receive-pack pair for the push side, and/or
fetch&clone-pack/upload-pack pair for the pull side does not
work as well as you would want, then ssh-push/pull pair may
still be a useful tool, at the same time that means send-pack
and upload-pack should be fixed to address the problem you have
with them.  But if that is not the case, then it might be better
to declare that ssh-pull/push pair has outlived its usefulness.

The same thing can be said about local-pull to a lesser degree.
Lesser because people, including Pasky who said so on the list
recently, would like its hard-linking behaviour, and its not
exploding the existing packs, which send-pack and upload-pack
would not give.  So I would rate local-pull higher than
ssh-push/pull on the priority scale if I were doing them.

On Thu, 11 Aug 2005, Junio C Hamano wrote:

Daniel Barkalow [off-list ref] writes:

quoted

Petr Baudis [off-list ref] writes:

quoted

Yes, but cg-clone doesn't - it naively depended on the core git tools
actually, er.. working. ;-)

Sorry about that.  I used to have a wrapper to deal with packs
around http-pull before Daniel's pack enhancement, and yanking
it before really checking that enhanced http-pull actually
worked was my fault as well.

It was actually the patches after the http-pull fixes (the ones for 
parallelizing pull.c) that broke things; one advantage to fixing 
local-pull would be that you can set up tests for it reasonably 
effectively, which would have caught the regression.

quoted

At some point, I have to revisit getting git-ssh-* to generate exactly the 
required pack and transfer that, but that's an efficiency issue, not a 
correctness one, and shouldn't be relevant to the problem you're having.

Wouldn't enhancing ssh-push to generate packs on the fly involve
reinventing send-pack and/or upload-pack?

The idea is that you wouldn't have to identify what situation applied 
yourself; you could just invoke git-ssh-pull/git-ssh-push, and it would 
happen faster due to the compression benefits. The point is that scripts 
can just pick which git-*-pull to use based on the format of the remote 
branch address, without variation in behavior.

The same thing can be said about local-pull to a lesser degree.
Lesser because people, including Pasky who said so on the list
recently, would like its hard-linking behaviour, and its not
exploding the existing packs, which send-pack and upload-pack
would not give.  So I would rate local-pull higher than
ssh-push/pull on the priority scale if I were doing them.

This is a higher priority, but writing more than bugfixes is unpleasent at 
the moment due to my home workstation's monitor dying, so it'll probably 
be next week that I'll get to it. The git-ssh-* stuff is longer-term, 
since it works now, and isn't even all that slow with the overlapping 
requests.

You could, actually, probably do the local-pull fix if you wanted. I seem 
to recall that being your code originally; you just need to have fetch() 
identify that an object is in a pack, copy/link/symlink the index and 
pack instead of the object file, and add the pack to the list of 
registered packs. I've mostly been failing to deal with reading an index 
file that is in some directory that hasn't been registered as somewhere to 
read from (i.e. the source repository).

	-Daniel
*This .sig left intentionally blank*