Skip to contentSkip to conversation

HTTP questions

DISCUSSION Nick Hengeveld 4 participants 9 messages Jun 15, 2016

lore
Share & tools
lore

Nick Hengeveld

author
#0 lore raw

While testing recent http-fetch changes with a current checkout of next, I noticed a couple of things:

- "git push" seems to pass --thin by default to http-push, which subsequently barfs because that's not a valid http-push option. Should it be? Should it be silently ignored? Should git-push not default to --thin when pushing with HTTP transport?

- when I clone, http-fetch outputs a whole bunch of "error: Could not read ..." messages - is that expected?

Signature · 2 lines
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.
#1 lore raw
Quoted 4 lines · Nick Hengeveld

- "git push" seems to pass --thin by default to http-push, which subsequently barfs because that's not a valid http-push option. Should it be? Should it be silently ignored? Should git-push not default to --thin when pushing with HTTP transport?

The way I understand http-push works is that it does not use packed transfer, so I presume even if we give --thin as a hint it cannot even take advantage of it. Probably we should stop passing --thin to http transport (git native one uses it as a cue that it can generate baseless deltas in the resulting pack).

Quoted 2 lines

- when I clone, http-fetch outputs a whole bunch of "error: Could not read ..." messages - is that expected?

The clone over http seems to be severely broken in "next" right now. The one in "master" seems to be OK. bisecting suggests the breakage is coming from the tree parser rewrite.

bisect points at 136f2e548a34f1f504b0f062f87ddf33e8d6e83b.

#2 lore raw

http-fetch troubles

Quoted 13 lines · Junio C Hamano
Quoted 4 lines · Nick Hengeveld

- "git push" seems to pass --thin by default to http-push, which subsequently barfs because that's not a valid http-push option. Should it be? Should it be silently ignored? Should git-push not default to --thin when pushing with HTTP transport?

The way I understand http-push works is that it does not use packed transfer, so I presume even if we give --thin as a hint it cannot even take advantage of it. Probably we should stop passing --thin to http transport (git native one uses it as a cue that it can generate baseless deltas in the resulting pack).

Quoted 8 lines
Quoted 2 lines

- when I clone, http-fetch outputs a whole bunch of "error: Could not read ..." messages - is that expected?

The clone over http seems to be severely broken in "next" right now. The one in "master" seems to be OK. bisecting suggests the breakage is coming from the tree parser rewrite.

bisect points at 136f2e548a34f1f504b0f062f87ddf33e8d6e83b.

I've pushed out Nick's http-fetch fixes in "master" to see if it fixes problems for people. Right now the one in "next" branch seems to be having unrelated problems coming from another topic which I haven't started tracking down yet.

#3 lore raw

Re: http-fetch troubles

Woohoo! The stuff you moved to master (which is what I was building from, not "next", as Nick pointed out) has fixed the git-http-fetch segfault problem I was seeing.

Thanks!
-Becky

Quoted 30 lines · Junio C Hamano
Quoted 13 lines · Junio C Hamano
Quoted 4 lines · Nick Hengeveld

- "git push" seems to pass --thin by default to http-push, which subsequently barfs because that's not a valid http-push option. Should it be? Should it be silently ignored? Should git-push not default to --thin when pushing with HTTP transport?

The way I understand http-push works is that it does not use packed transfer, so I presume even if we give --thin as a hint it cannot even take advantage of it. Probably we should stop passing --thin to http transport (git native one uses it as a cue that it can generate baseless deltas in the resulting pack).

Quoted 8 lines
Quoted 2 lines

- when I clone, http-fetch outputs a whole bunch of "error: Could not read ..." messages - is that expected?

The clone over http seems to be severely broken in "next" right now. The one in "master" seems to be OK. bisecting suggests the breakage is coming from the tree parser rewrite.

bisect points at 136f2e548a34f1f504b0f062f87ddf33e8d6e83b.

I've pushed out Nick's http-fetch fixes in "master" to see if it fixes problems for people. Right now the one in "next" branch seems to be having unrelated problems coming from another topic which I haven't started tracking down yet.

#4 lore raw

Re: http-fetch troubles

Quoted 5 lines · Becky Bruce

Woohoo! The stuff you moved to master (which is what I was building from, not "next", as Nick pointed out) has fixed the git-http-fetch segfault problem I was seeing.

Thanks!

Thanks Nick for fixing, and Becky for confirming.

... and I take all the credit ;-).

I haven't pushed it out yet, but I believe "next" is also good to go.

#5 lore raw

Re: http-fetch troubles

Quoted 2 lines · Junio C Hamano

... and I take all the credit ;-).

I always knew you'd work out as a maintainer.

Linus

#6 lore raw

Re: http-fetch troubles

Quoted 7 lines · Linus Torvalds
Quoted 2 lines · Junio C Hamano

... and I take all the credit ;-).

I always knew you'd work out as a maintainer.

Linus

Thanks for the compliment. I realize that is your way to take the credit ;-).

#7 lore raw

Re: http-fetch troubles

In reply to Junio C Hamano

I think this fixes the http trouble with tree parser change in the "next" branch.

-- >8 --
fetch.c: do not call process_tree() from process_tree().

This function reads a freshly fetched tree object, and schedules the objects pointed by it for further fetching, so calling lookup-tree and doing process_tree() recursively from there does not make much sense. We need to use process() on it to make sure we fetch it first.

Signed-off-by Junio C Hamano <[email protected]>

---

fetch.c +10 -3
index ec2d8c3..107504b 100644
@@ -46,13 +46,20 @@ static int process_tree(struct tree *tre
 	desc.buf = tree->buffer;
 	desc.size = tree->size;
 	while (tree_entry(&desc, &entry)) {
+		struct object *obj = NULL;
+
 		if (S_ISDIR(entry.mode)) {
 			struct tree *tree = lookup_tree(entry.sha1);
-			process_tree(tree);
-		} else {
+			if (tree)
+				obj = &tree->object;
+		}
+		else {
 			struct blob *blob = lookup_blob(entry.sha1);
-			process(&blob->object);
+			if (blob)
+				obj = &blob->object;
 		}
+		if (!obj || process(obj))
+			return -1;
 	}
 	free(tree->buffer);
 	tree->buffer = NULL;
#8 lore raw

Re: http-fetch troubles

Quoted 3 lines · Junio C Hamano

I think this fixes the http trouble with tree parser change in the "next" branch.

Ahh, my bad. That happened as a bug in my original understanding of that fetch.c thing.

Your fix looks obviously fine,

Linus