Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

4 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:45

Dennis Kaarsemaker [off-list ref] writes:

By not clearing the request buffer in stateless-rpc mode, fetch-pack
would keep sending already known-common commits, leading to ever bigger
http requests, eventually getting too large for git-http-backend to
handle properly without filling up the pipe buffer in inflate_request.
---
I'm still not quite sure whether this is the right thing to do, but make
test still passes :) The new testcase demonstrates the problem, when
running t5551 with EXPENSIVE, this test will hang without the patch to
fetch-pack.c and succeed otherwise.

IIUC, because "stateless" is just that, i.e. the server-end does not
keep track of what is already known, not telling what is known to be
common in each request would fundamentally break the protocol.  Am I
mistaken?

quoted hunk

 fetch-pack.c                |  1 -
 t/t5551-http-fetch-smart.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 655ee64..258245c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -410,7 +410,6 @@ static int find_common(struct fetch_pack_args *args,
 						 */
 						const char *hex = sha1_to_hex(result_sha1);
 						packet_buf_write(&req_buf, "have %s\n", hex);
-						state_len = req_buf.len;
 					}
 					mark_common(commit, 0, 1);
 					retval = 0;

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6cbc12d..2aac237 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh

@@ -245,5 +245,37 @@ test_expect_success EXPENSIVE 'clone the 50,000 tag repo to check OS command lin
 	)
 '
 
+test_expect_success EXPENSIVE 'create 50,000 more tags' '
+	(
+	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	for i in `test_seq 50001 100000`
+	do
+		echo "commit refs/heads/too-many-refs-again"
+		echo "mark :$i"
+		echo "committer git <git@example.com> $i +0000"
+		echo "data 0"
+		echo "M 644 inline bla.txt"
+		echo "data 4"
+		echo "bla"
+		# make every commit dangling by always
+		# rewinding the branch after each commit
+		echo "reset refs/heads/too-many-refs-again"
+		echo "from :50001"
+	done | git fast-import --export-marks=marks &&
+
+	# now assign tags to all the dangling commits we created above
+	tag=$(perl -e "print \"bla\" x 30") &&
+	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
+	)
+'
+
+test_expect_success EXPENSIVE 'fetch the new tags' '
+	(
+		cd too-many-refs &&
+		git fetch --tags &&
+		test $(git for-each-ref refs/tags | wc -l) = 100000
+	)
+'
+
 stop_httpd
 test_done

Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

From: Dennis Kaarsemaker <hidden>
Date: 2016-06-15 23:02:46

On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:

Dennis Kaarsemaker [off-list ref] writes:

quoted

By not clearing the request buffer in stateless-rpc mode, fetch-pack
would keep sending already known-common commits, leading to ever bigger
http requests, eventually getting too large for git-http-backend to
handle properly without filling up the pipe buffer in inflate_request.
---
I'm still not quite sure whether this is the right thing to do, but make
test still passes :) The new testcase demonstrates the problem, when
running t5551 with EXPENSIVE, this test will hang without the patch to
fetch-pack.c and succeed otherwise.

IIUC, because "stateless" is just that, i.e. the server-end does not
keep track of what is already known, not telling what is known to be
common in each request would fundamentally break the protocol.  Am I
mistaken?

That sounds plausible, but why then does the fetch complete with this
line removed, and why does 'make test' still pass? I tried to understand
the protocol, but the documentation has TODO's in some critical
places :)

And if that's true, it means the inflate_request / upload-pack
interaction should be fixed, so more than 64k (current linux pipe buffer
size) of uncompressed data is supported. I see two options:

* Turning that interaction into a more cooperative process, with a
  select/poll loop
* Make upload-pack buffer its entire response when run in stateless_rpc
  mode until it has consumed all of the request

The latter sounds easier to do, but not being very familiar with the
protocol, I may have missed something obvious.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

From: Duy Nguyen <hidden>
Date: 2016-06-15 23:02:46

On Wed, Oct 22, 2014 at 2:41 PM, Dennis Kaarsemaker
[off-list ref] wrote:

I see two options:

* Turning that interaction into a more cooperative process, with a
  select/poll loop
* Make upload-pack buffer its entire response when run in stateless_rpc
  mode until it has consumed all of the request

Or add a helper daemon and support stateful smart http. Or maybe
that's what you meant in the first option.
-- 
Duy

Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

From: Dennis Kaarsemaker <hidden>
Date: 2016-06-15 23:02:47

On Wed, Oct 22, 2014 at 05:07:31PM +0700, Duy Nguyen wrote:

On Wed, Oct 22, 2014 at 2:41 PM, Dennis Kaarsemaker
[off-list ref] wrote:

quoted

I see two options:

* Turning that interaction into a more cooperative process, with a
  select/poll loop
* Make upload-pack buffer its entire response when run in stateless_rpc
  mode until it has consumed all of the request

Or add a helper daemon and support stateful smart http. Or maybe
that's what you meant in the first option.

No, I meant that get-http-backend should have a select/poll loop that
can read from and write to git-upload-pack at the same time, but option
two was easier to implement :)

Stateful smart http seems to be against the design goals of smart http if I
interpret Documentation/technical/http-protocol.txt correctly though, so
that doesn't seem to be the right approach.
-- 
Dennis Kaarsemaker [off-list ref]
http://twitter.com/seveas

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help