Re: [PATCH 6/6] fetch-pack: fix deepen shallow over smart http with no-done cap
Duy Nguyen 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 "have"s 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
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 "have"s 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 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 "have"s 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 "have"s 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 && +
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 wrote: > Nguyễn Thái Ngọc Duy 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
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 "have"s 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 Thu, Feb 6, 2014 at 10:10 AM, Nguyễn Thái Ngọc Duy 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 "have"s 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
Nguyễn Thái Ngọc Duy 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
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
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 "have"s 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
