Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
Duy Nguyen pclo...@gmail.com writes: On Fri, Feb 07, 2014 at 10:01:08AM -0800, Junio C Hamano wrote: Here is the difference between the posted series and what I queued after applying the changes suggested during the review. Thanks. I was going to send a reroll after the received comments. Could you put this on top of 6/6, just to make sure future changes in t5537 (maybe more or less commits created..) does not change the test behavior? It fixes the test name too. I originally thought, ok let's create commits in one test and do fetch in another. But it ended up in the same test and I forgot to update test name. Surely, and thanks for being careful. Will squash it in. -- 8 -- diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 1413caf..b300383 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -203,7 +203,7 @@ EOF # This test is tricky. We need large enough haves that fetch-pack # will put pkt-flush in between. Then we need a have the server # does not have, it'll send ACK %s ready -test_expect_success 'add more commits' ' +test_expect_success 'no shallow lines after receiving ACK ready' ' ( cd shallow for i in $(test_seq 10) @@ -224,7 +224,9 @@ test_expect_success 'add more commits' ' cd clone git checkout --orphan newnew test_commit new-too - git fetch --depth=2 + GIT_TRACE_PACKET=$TRASH_DIRECTORY/trace git fetch --depth=2 + grep fetch-pack ACK .* ready ../trace + ! grep fetch-pack done ../trace ) ' -- 8 -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index b0fa738..fb11073 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -200,5 +200,29 @@ EOF ) ' +# This test is tricky. We need large enough haves that fetch-pack +# will put pkt-flush in between. Then we need a have the the server +# does not have, it'll send ACK %s ready +test_expect_success 'add more commits' ' + ( + cd shallow + for i in $(seq 10); do + git checkout --orphan unrelated$i + test_commit unrelated$i /dev/null + git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git refs/heads/unrelated$i:refs/heads/unrelated$i + git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i In addition to two problems Eric and Peff noticed, chain is broken between these two pushes. I initially didn't notice it but it became obvious after reformatting to fix the indentation. Here is the difference between the posted series and what I queued after applying the changes suggested during the review. Thanks. Documentation/technical/pack-protocol.txt | 4 +-- Documentation/technical/protocol-capabilities.txt | 10 +++ t/t5537-fetch-shallow.sh | 34 +-- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index eb0b8a1..39c6410 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -338,8 +338,8 @@ during a prior round. This helps to ensure that at least one common ancestor is found before we give up entirely. Once the 'done' line is read from the client, the server will either -send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the -last SHA-1 determined to be common. The server only sends +send a final 'ACK obj-id' or it will send a 'NAK'. 'obj-id' is the object +name of the last commit determined to be common. The server only sends ACK after 'done' if there is at least one common base and multi_ack or multi_ack_detailed is enabled. The server always sends NAK after 'done' if there is no common base found. diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt index cb2f5eb..e174343 100644 --- a/Documentation/technical/protocol-capabilities.txt +++ b/Documentation/technical/protocol-capabilities.txt @@ -77,15 +77,15 @@ section Packfile Negotiation for more information. no-done --- -This capability should be only used with smart HTTP protocol. If +This capability should only be used with the smart HTTP protocol. If multi_ack_detailed and no-done are both present, then the sender is free to immediately send a pack following its first ACK obj-id ready message. -Without no-done in smart HTTP protocol, the server session would end -and the client has to make another trip to send done and the server -can send the pack. no-done removes the last round and thus slightly -reduces latency. +Without no-done in the smart HTTP protocol, the server session would +end and the client has to make another trip to send done before +the server can send the pack. no-done removes the last round and +thus slightly reduces latency. thin-pack - diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index fb11073..1413caf 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -201,26 +201,30 @@ EOF ' # This test is tricky. We need large enough haves that fetch-pack -# will put pkt-flush in between. Then we need a have the the server +# will put pkt-flush in between. Then we need a have the server # does not have, it'll send ACK %s ready test_expect_success 'add more commits' ' ( - cd shallow - for i in $(seq 10); do - git checkout --orphan unrelated$i - test_commit unrelated$i /dev/null - git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git refs/heads/unrelated$i:refs/heads/unrelated$i - git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i - done - git checkout master - test_commit new - git push $HTTPD_DOCUMENT_ROOT_PATH/repo.git master + cd shallow + for i in $(test_seq 10) + do + git checkout --orphan unrelated$i + test_commit unrelated$i + git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git \ + refs/heads/unrelated$i:refs/heads/unrelated$i + git push -q ../clone/.git \ + refs/heads/unrelated$i:refs/heads/unrelated$i || + exit 1 + done + git checkout master + test_commit new + git push $HTTPD_DOCUMENT_ROOT_PATH/repo.git master )
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
On Fri, Feb 07, 2014 at 10:01:08AM -0800, Junio C Hamano wrote: Here is the difference between the posted series and what I queued after applying the changes suggested during the review. Thanks. I was going to send a reroll after the received comments. Could you put this on top of 6/6, just to make sure future changes in t5537 (maybe more or less commits created..) does not change the test behavior? It fixes the test name too. I originally thought, ok let's create commits in one test and do fetch in another. But it ended up in the same test and I forgot to update test name. -- 8 -- diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 1413caf..b300383 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -203,7 +203,7 @@ EOF # This test is tricky. We need large enough haves that fetch-pack # will put pkt-flush in between. Then we need a have the server # does not have, it'll send ACK %s ready -test_expect_success 'add more commits' ' +test_expect_success 'no shallow lines after receiving ACK ready' ' ( cd shallow for i in $(test_seq 10) @@ -224,7 +224,9 @@ test_expect_success 'add more commits' ' cd clone git checkout --orphan newnew test_commit new-too - git fetch --depth=2 + GIT_TRACE_PACKET=$TRASH_DIRECTORY/trace git fetch --depth=2 + grep fetch-pack ACK .* ready ../trace + ! grep fetch-pack done ../trace ) ' -- 8 -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
In smart http, upload-pack adds new shallow lines at the beginning of
each rpc response. Only shallow lines from the first rpc call are
useful. After that they are thrown away. It's designed this way
because upload-pack is stateless and has no idea when its shallow
lines are helpful or not.
So after refs are negotiated with multi_ack_detailed and both sides
happy. The server sends ACK obj-id ready, terminates the rpc call
Is the above ... and both sides are happy, the server sends ...?
Lack of a verb makes it hard to guess.
and waits for the final rpc round. The client sends done. The server
sends another response, which also has shallow lines at the beginning,
and the last ACK obj-id line.
When no-done is active, the last round is cut out, the server sends
ACK obj-id ready and ACK obj-id in the same rpc
response. fetch-pack is updated to recognize this and not send
done. However it still tries to consume shallow lines, which are
never sent.
Update the code, make sure to skip consuming shallow lines when
no-done is enabled.
Reported-by: Jeff King p...@peff.net
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Thanks.
Do I understand the situation correctly if I said The call to
consume-shallow-list has been here from the very beginning, but it
should have been adjusted like this patch when no-done was
introduced.?
fetch-pack.c | 3 ++-
t/t5537-fetch-shallow.sh | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..f061f1f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -439,7 +439,8 @@ done:
}
strbuf_release(req_buf);
- consume_shallow_list(args, fd[0]);
+ if (!got_ready || !no_done)
+ consume_shallow_list(args, fd[0]);
while (flushes || multi_ack) {
int ack = get_ack(fd[0], result_sha1);
if (ack) {
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..fb11073 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -200,5 +200,29 @@ EOF
)
'
+# This test is tricky. We need large enough haves that fetch-pack
+# will put pkt-flush in between. Then we need a have the the server
+# does not have, it'll send ACK %s ready
+test_expect_success 'add more commits' '
+ (
+ cd shallow
+ for i in $(seq 10); do
+ git checkout --orphan unrelated$i
+ test_commit unrelated$i /dev/null
+ git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git
refs/heads/unrelated$i:refs/heads/unrelated$i
+ git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i
+ done
+ git checkout master
+ test_commit new
+ git push $HTTPD_DOCUMENT_ROOT_PATH/repo.git master
+ )
+ (
+ cd clone
+ git checkout --orphan newnew
+ test_commit new-too
+ git fetch --depth=2
+ )
+'
+
stop_httpd
test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
On Thu, Feb 6, 2014 at 10:10 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index b0fa738..fb11073 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -200,5 +200,29 @@ EOF ) ' +# This test is tricky. We need large enough haves that fetch-pack +# will put pkt-flush in between. Then we need a have the the server s/the the/that the/ +# does not have, it'll send ACK %s ready +test_expect_success 'add more commits' ' + ( + cd shallow + for i in $(seq 10); do Do you want to use test_seq here? + git checkout --orphan unrelated$i + test_commit unrelated$i /dev/null + git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git refs/heads/unrelated$i:refs/heads/unrelated$i + git push -q ../clone/.git refs/heads/unrelated$i:refs/heads/unrelated$i + done + git checkout master + test_commit new + git push $HTTPD_DOCUMENT_ROOT_PATH/repo.git master + ) + ( + cd clone + git checkout --orphan newnew + test_commit new-too + git fetch --depth=2 + ) +' + stop_httpd test_done -- 1.8.5.2.240.g8478abd -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
On Thu, Feb 06, 2014 at 10:10:39PM +0700, Nguyễn Thái Ngọc Duy wrote: In smart http, upload-pack adds new shallow lines at the beginning of each rpc response. Only shallow lines from the first rpc call are useful. After that they are thrown away. It's designed this way because upload-pack is stateless and has no idea when its shallow lines are helpful or not. So after refs are negotiated with multi_ack_detailed and both sides happy. The server sends ACK obj-id ready, terminates the rpc call and waits for the final rpc round. The client sends done. The server sends another response, which also has shallow lines at the beginning, and the last ACK obj-id line. When no-done is active, the last round is cut out, the server sends ACK obj-id ready and ACK obj-id in the same rpc response. fetch-pack is updated to recognize this and not send done. However it still tries to consume shallow lines, which are never sent. Update the code, make sure to skip consuming shallow lines when no-done is enabled. Thanks for a nice explanation. +# This test is tricky. We need large enough haves that fetch-pack +# will put pkt-flush in between. Then we need a have the the server s/the the/the/ +# does not have, it'll send ACK %s ready +test_expect_success 'add more commits' ' + ( + cd shallow + for i in $(seq 10); do This probably needs to be test_seq for portability. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
On Fri, Feb 7, 2014 at 2:16 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: In smart http, upload-pack adds new shallow lines at the beginning of each rpc response. Only shallow lines from the first rpc call are useful. After that they are thrown away. It's designed this way because upload-pack is stateless and has no idea when its shallow lines are helpful or not. So after refs are negotiated with multi_ack_detailed and both sides happy. The server sends ACK obj-id ready, terminates the rpc call Is the above ... and both sides are happy, the server sends ...? Yes. Although think again, both sides is incorrect. If the server side is happy, then it'll send ACK.. ready to stop the client. The client can hardly protest. Do I understand the situation correctly if I said The call to consume-shallow-list has been here from the very beginning, but it should have been adjusted like this patch when no-done was introduced.? It's been there since the introduction of smart http (there are so many beginnings in git). The rest is right. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
