{"thread":{"id":"53314","subject":"[PATCH 0/2] Protocol v2 in_vain fixes","startedAt":"2020-04-25T00:56:11Z","lastAt":"2020-10-13T14:45:43Z","messageCount":25,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder","Michal Suchánek","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"396208","messageId":"cover.1587775989.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":null,"subject":"[PATCH 0/2] Protocol v2 in_vain fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-25T00:56:00Z","receivedAt":"2020-04-25T00:56:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Patch 1 is a fix for what I described in [1].\n\nThe fix in patch 1 exposes a bug, which patch 2 fixes. Jonathan Nieder\nfound the bug [2], and I have written a test covering this case. I've\ncredited him as \"Helped-by\".\n\n[1] https://lore.kernel.org/git/20200423213735.242662-1-jonathantanmy@google.com/\n[2] https://lore.kernel.org/git/20200422165358.GB140314@google.com/\n\nJonathan Tan (2):\n  fetch-pack: in protocol v2, in_vain only after ACK\n  fetch-pack: in protocol v2, reset in_vain upon ACK\n\n fetch-pack.c          | 14 +++++++++----\n t/t5500-fetch-pack.sh | 49 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 4 deletions(-)\n\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396209","messageId":"51174e53527f9b29bb0a5ebb8726f07333113dfb.1587775989.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1587775989.git.jonathantanmy@google.com","subject":"[PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-25T00:56:01Z","receivedAt":"2020-04-25T00:56:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching, Git stops negotiation when it has sent at least\nMAX_IN_VAIN (which is 256) \"have\" lines without having any of them\nACK-ed. But this is supposed to trigger only after the first ACK, as\npack-protocol.txt says:\n\n  However, the 256 limit *only* turns on in the canonical client\n  implementation if we have received at least one \"ACK %s continue\"\n  during a prior round.  This helps to ensure that at least one common\n  ancestor is found before we give up entirely.\n\nThe code path for protocol v0 observes this, but not protocol v2,\nresulting in shorter negotiation rounds but significantly larger\npackfiles. Teach the code path for protocol v2 to check this criterion\nonly after at least one ACK was received.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c          | 13 +++++++++----\n t/t5500-fetch-pack.sh | 19 +++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0b07b3ee73..7d15c7af81 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n }\n \n static int add_haves(struct fetch_negotiator *negotiator,\n+\t\t     int seen_ack,\n \t\t     struct strbuf *req_buf,\n \t\t     int *haves_to_send, int *in_vain)\n {\n@@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \t}\n \n \t*in_vain += haves_added;\n-\tif (!haves_added || *in_vain >= MAX_IN_VAIN) {\n+\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n \t\t/* Send Done */\n \t\tpacket_buf_write(req_buf, \"done\\n\");\n \t\tret = 1;\n@@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t      struct fetch_pack_args *args,\n \t\t\t      const struct ref *wants, struct oidset *common,\n \t\t\t      int *haves_to_send, int *in_vain,\n-\t\t\t      int sideband_all)\n+\t\t\t      int sideband_all, int seen_ack)\n {\n \tint ret = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tadd_common(&req_buf, common);\n \n \t\t/* Add initial haves */\n-\t\tret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);\n+\t\tret = add_haves(negotiator, seen_ack, &req_buf,\n+\t\t\t\thaves_to_send, in_vain);\n \t}\n \n \t/* Send request */\n@@ -1444,6 +1446,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tint haves_to_send = INITIAL_FLUSH;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n+\tint seen_ack = 0;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1500,7 +1503,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tif (send_fetch_request(negotiator, fd[1], args, ref,\n \t\t\t\t\t       &common,\n \t\t\t\t\t       &haves_to_send, &in_vain,\n-\t\t\t\t\t       reader.use_sideband))\n+\t\t\t\t\t       reader.use_sideband,\n+\t\t\t\t\t       seen_ack))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_PROCESS_ACKS;\n@@ -1513,6 +1517,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\tbreak;\n \t\t\tcase 1:\n \t\t\t\tin_vain = 0;\n+\t\t\t\tseen_ack = 1;\n \t\t\t\t/* fallthrough */\n \t\t\tdefault:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex baa1a99f45..fcc5cc8ab0 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -385,6 +385,25 @@ test_expect_success 'clone shallow with packed refs' '\n \ttest_cmp count8.expected count8.actual\n '\n \n+test_expect_success 'in_vain not triggered before first ACK' '\n+\trm -rf myserver myclient trace &&\n+\tgit init myserver &&\n+\ttest_commit -C myserver foo &&\n+\tgit clone \"file://$(pwd)/myserver\" myclient &&\n+\n+\t# MAX_IN_VAIN is 256. Because of batching, the client will send 496\n+\t# (16+32+64+128+256) commits, not 256, before giving up. So create 496\n+\t# irrelevant commits.\n+\ttest_commit_bulk -C myclient 496 &&\n+\n+\t# The new commit that the client wants to fetch.\n+\ttest_commit -C myserver bar &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n+\tcp trace /tmp/x &&\n+\ttest_i18ngrep \"Total 3 \" trace\n+'\n+\n test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t(\n \t\tgit clone --bare --branch B --single-branch \"file://$(pwd)/.\" no-reflog &&\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396210","messageId":"8499d5268e49b6b823b0b9312d77e41e311d9a75.1587775989.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1587775989.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-25T00:56:02Z","receivedAt":"2020-04-25T00:56:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the function process_acks() in fetch-pack.c, the variable\nreceived_ack is meant to track that an ACK was received, but it was\nnever set. This results in negotiation terminating prematurely through\nthe in_vain counter, when the counter should have been reset upon every\nACK.\n\nTherefore, reset the in_vain counter upon every ACK.\n\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c          |  1 +\n t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7d15c7af81..8551c07ed9 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1287,6 +1287,7 @@ static int process_acks(struct fetch_negotiator *negotiator,\n \n \t\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n \t\t\tstruct object_id oid;\n+\t\t\treceived_ack = 1;\n \t\t\tif (!get_oid_hex(arg, &oid)) {\n \t\t\t\tstruct commit *commit;\n \t\t\t\toidset_insert(common, &oid);\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex fcc5cc8ab0..98e1442cbf 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -404,6 +404,36 @@ test_expect_success 'in_vain not triggered before first ACK' '\n \ttest_i18ngrep \"Total 3 \" trace\n '\n \n+test_expect_success 'in_vain resetted upon ACK' '\n+\trm -rf myserver myclient trace &&\n+\tgit init myserver &&\n+\n+\t# Linked list of commits on master. The first is common; the rest are\n+\t# not.\n+\ttest_commit -C myserver first_master_commit &&\n+\tgit clone \"file://$(pwd)/myserver\" myclient &&\n+\ttest_commit_bulk -C myclient 255 &&\n+\n+\t# Another linked list of commits on anotherbranch with no connection to\n+\t# master. The first is common; the rest are not.\n+\tgit -C myserver checkout --orphan anotherbranch &&\n+\ttest_commit -C myserver first_anotherbranch_commit &&\n+\tgit -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&\n+\tgit -C myclient checkout anotherbranch &&\n+\ttest_commit_bulk -C myclient 255 &&\n+\n+\t# The new commit that the client wants to fetch.\n+\tgit -C myserver checkout master &&\n+\ttest_commit -C myserver to_fetch &&\n+\n+\t# The client will send (as \"have\"s) all 256 commits in anotherbranch\n+\t# first. The 256th commit is common between the client and the server,\n+\t# and should reset in_vain. This allows negotiation to continue until\n+\t# the client reports that first_anotherbranch_commit is common.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n+\ttest_i18ngrep \"Total 3 \" trace\n+'\n+\n test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t(\n \t\tgit clone --bare --branch B --single-branch \"file://$(pwd)/.\" no-reflog &&\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396233","messageId":"xmqqlfmk9mph.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"51174e53527f9b29bb0a5ebb8726f07333113dfb.1587775989.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-25T05:08:26Z","receivedAt":"2020-04-25T05:15:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When fetching, Git stops negotiation when it has sent at least\n> MAX_IN_VAIN (which is 256) \"have\" lines without having any of them\n> ACK-ed. But this is supposed to trigger only after the first ACK, as\n> pack-protocol.txt says:\n>\n>   However, the 256 limit *only* turns on in the canonical client\n>   implementation if we have received at least one \"ACK %s continue\"\n>   during a prior round.  This helps to ensure that at least one common\n>   ancestor is found before we give up entirely.\n>\n> The code path for protocol v0 observes this, but not protocol v2,\n> resulting in shorter negotiation rounds but significantly larger\n> packfiles. Teach the code path for protocol v2 to check this criterion\n> only after at least one ACK was received.\n\nMakes sense.\n\nI think we can instead use in_vain==NULL as a signal that we haven't\nseen any ack yet and that shrinks the patch somewhat ([Patch 1/2]\nbecomes +6/-4 from +9/-4 for fetch-pack.c).  I do not know if the\nresult is easier to follow (as there is one less variable to keep in\nmind), though.  The callee is probably easier to reason about (it\nneeds to worry about the *in_vain variable only when it is given---\nyou cannot beat the simplicity of it), but the caller side may\nbecome harder to reason about, especially without any comment where\nin_vain_p becomes non-NULL.  Your version has an assignment to make\n\"seen_ack\" to true there, which makes it very clear without any\ncomment what is going on.\n\nSo I dunno.  I've queued your version and discarded the following.\n\n fetch-pack.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1734a573b0..7b837b6a76 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1156,8 +1156,9 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \t\t\tbreak;\n \t}\n \n-\t*in_vain += haves_added;\n-\tif (!haves_added || *in_vain >= MAX_IN_VAIN) {\n+\tif (in_vain)\n+\t\t*in_vain += haves_added;\n+\tif (!haves_added || (in_vain && *in_vain >= MAX_IN_VAIN)) {\n \t\t/* Send Done */\n \t\tpacket_buf_write(req_buf, \"done\\n\");\n \t\tret = 1;\n@@ -1444,6 +1445,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tint haves_to_send = INITIAL_FLUSH;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n+\tint *in_vain_p = NULL;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1499,7 +1501,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t}\n \t\t\tif (send_fetch_request(negotiator, fd[1], args, ref,\n \t\t\t\t\t       &common,\n-\t\t\t\t\t       &haves_to_send, &in_vain,\n+\t\t\t\t\t       &haves_to_send, in_vain_p,\n \t\t\t\t\t       reader.use_sideband))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n@@ -1512,7 +1514,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t\tbreak;\n \t\t\tcase 1:\n-\t\t\t\tin_vain = 0;\n+\t\t\t\tin_vain_p = &in_vain;\n \t\t\t\t/* fallthrough */\n \t\t\tdefault:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n"},{"id":"396275","messageId":"20200426002802.GD877@gmail.com","threadId":"53314","inReplyTo":"51174e53527f9b29bb0a5ebb8726f07333113dfb.1587775989.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-26T00:28:02Z","receivedAt":"2020-04-26T00:28:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> When fetching, Git stops negotiation when it has sent at least\n> MAX_IN_VAIN (which is 256) \"have\" lines without having any of them\n> ACK-ed. But this is supposed to trigger only after the first ACK, as\n> pack-protocol.txt says:\n> \n>   However, the 256 limit *only* turns on in the canonical client\n>   implementation if we have received at least one \"ACK %s continue\"\n>   during a prior round.  This helps to ensure that at least one common\n>   ancestor is found before we give up entirely.\n> \n> The code path for protocol v0 observes this, but not protocol v2,\n> resulting in shorter negotiation rounds but significantly larger\n> packfiles. Teach the code path for protocol v2 to check this criterion\n> only after at least one ACK was received.\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  fetch-pack.c          | 13 +++++++++----\n>  t/t5500-fetch-pack.sh | 19 +++++++++++++++++++\n>  2 files changed, 28 insertions(+), 4 deletions(-)\n\nMakes a lot of sense.  Good find.\n\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n[...]\n> @@ -1513,6 +1517,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>  \t\t\t\tbreak;\n>  \t\t\tcase 1:\n>  \t\t\t\tin_vain = 0;\n> +\t\t\t\tseen_ack = 1;\n\nnot about this patch: can these return values from process_acks be made\ninto an enum with named enumerators?  That would make what's happening\nin the call site more obvious.\n\n>  \t\t\t\t/* fallthrough */\n>  \t\t\tdefault:\n>  \t\t\t\tstate = FETCH_SEND_REQUEST;\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index baa1a99f45..fcc5cc8ab0 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -385,6 +385,25 @@ test_expect_success 'clone shallow with packed refs' '\n>  \ttest_cmp count8.expected count8.actual\n>  '\n>  \n> +test_expect_success 'in_vain not triggered before first ACK' '\n> +\trm -rf myserver myclient trace &&\n> +\tgit init myserver &&\n> +\ttest_commit -C myserver foo &&\n> +\tgit clone \"file://$(pwd)/myserver\" myclient &&\n> +\n> +\t# MAX_IN_VAIN is 256. Because of batching, the client will send 496\n> +\t# (16+32+64+128+256) commits, not 256, before giving up. So create 496\n> +\t# irrelevant commits.\n> +\ttest_commit_bulk -C myclient 496 &&\n> +\n> +\t# The new commit that the client wants to fetch.\n> +\ttest_commit -C myserver bar &&\n> +\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n> +\tcp trace /tmp/x &&\n\nLeftover debugging line?\n\n> +\ttest_i18ngrep \"Total 3 \" trace\n\nClever.\n\nIn some sense this is a fragile test, since the server could change\nhow it reports progress some day.  Would it make sense (perhaps as a\nfollowup patch) for this to use a trace2 log instead?  For example,\nif we turn on tracing in the server, then since 9ed8790282\n(pack-objects: write objects packed to trace2, 2019-04-11) it will\nreport how many objects were in the pack it wrote.\n\nAfter removing the \"cp trace /tmp/x\" line,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"396279","messageId":"20200426011044.GF877@gmail.com","threadId":"53314","inReplyTo":"8499d5268e49b6b823b0b9312d77e41e311d9a75.1587775989.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-26T01:10:44Z","receivedAt":"2020-04-26T01:10:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -404,6 +404,36 @@ test_expect_success 'in_vain not triggered before first ACK' '\n>  \ttest_i18ngrep \"Total 3 \" trace\n>  '\n>  \n> +test_expect_success 'in_vain resetted upon ACK' '\n> +\trm -rf myserver myclient trace &&\n> +\tgit init myserver &&\n> +\n> +\t# Linked list of commits on master. The first is common; the rest are\n> +\t# not.\n> +\ttest_commit -C myserver first_master_commit &&\n> +\tgit clone \"file://$(pwd)/myserver\" myclient &&\n> +\ttest_commit_bulk -C myclient 255 &&\n> +\n> +\t# Another linked list of commits on anotherbranch with no connection to\n> +\t# master. The first is common; the rest are not.\n> +\tgit -C myserver checkout --orphan anotherbranch &&\n> +\ttest_commit -C myserver first_anotherbranch_commit &&\n> +\tgit -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&\n> +\tgit -C myclient checkout anotherbranch &&\n> +\ttest_commit_bulk -C myclient 255 &&\n> +\n> +\t# The new commit that the client wants to fetch.\n> +\tgit -C myserver checkout master &&\n> +\ttest_commit -C myserver to_fetch &&\n> +\n> +\t# The client will send (as \"have\"s) all 256 commits in anotherbranch\n> +\t# first. The 256th commit is common between the client and the server,\n> +\t# and should reset in_vain. This allows negotiation to continue until\n> +\t# the client reports that first_anotherbranch_commit is common.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n> +\ttest_i18ngrep \"Total 3 \" trace\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\nAs with the other patch, trace2 output from the server might be more\nstable.\n\nDo these tests pass with protocol v0 as well?  (Forgive my laziness in\nnot checking yet.)\n"},{"id":"396357","messageId":"20200427172736.61159-1-jonathantanmy@google.com","threadId":"53314","inReplyTo":"20200426002802.GD877@gmail.com","subject":"Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-27T17:27:36Z","receivedAt":"2020-04-27T17:27:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> not about this patch: can these return values from process_acks be made\n> into an enum with named enumerators?  That would make what's happening\n> in the call site more obvious.\n\nThat sounds reasonable to me.\n\n> > +\tcp trace /tmp/x &&\n> \n> Leftover debugging line?\n\nAh, yes. If Junio can't or won't do it locally then I'll send out\nanother set with this changed.\n\n> > +\ttest_i18ngrep \"Total 3 \" trace\n> \n> Clever.\n> \n> In some sense this is a fragile test, since the server could change\n> how it reports progress some day.  Would it make sense (perhaps as a\n> followup patch) for this to use a trace2 log instead?  For example,\n> if we turn on tracing in the server, then since 9ed8790282\n> (pack-objects: write objects packed to trace2, 2019-04-11) it will\n> report how many objects were in the pack it wrote.\n\nProbably better to have tracing in the client and use that, but this\nrequires us to add that tracing. But in general I agree.\n\n> After removing the \"cp trace /tmp/x\" line,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Thanks.\n\nThanks for your review.\n"},{"id":"396358","messageId":"20200427172837.62068-1-jonathantanmy@google.com","threadId":"53314","inReplyTo":"20200426011044.GF877@gmail.com","subject":"Re: [PATCH 2/2] fetch-pack: in protocol v2, reset in_vain upon ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-27T17:28:37Z","receivedAt":"2020-04-27T17:28:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> Thanks.\n> \n> As with the other patch, trace2 output from the server might be more\n> stable.\n> \n> Do these tests pass with protocol v0 as well?  (Forgive my laziness in\n> not checking yet.)\n\nThanks for your review. Yes they pass with protocol v0 too (checked by\npassing \"-c protocol.version=0\" when fetching).\n"},{"id":"396400","messageId":"xmqq8sig7ewl.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"20200427172736.61159-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T22:16:42Z","receivedAt":"2020-04-27T22:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> not about this patch: can these return values from process_acks be made\n>> into an enum with named enumerators?  That would make what's happening\n>> in the call site more obvious.\n>\n> That sounds reasonable to me.\n>\n>> > +\tcp trace /tmp/x &&\n>> \n>> Leftover debugging line?\n>\n> Ah, yes. If Junio can't or won't do it locally then I'll send out\n> another set with this changed.\n\nWell, if I am expecting the \"named enumerators\" patch anyway, I'd\nwait the fix to be done at the source ;-)\n"},{"id":"396413","messageId":"f0cfbc03c27658e54a73c46570c5153961ed85b6.1588031728.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1588031728.git.jonathantanmy@google.com","subject":"[PATCH v2 1/3] fetch-pack: return enum from process_acks()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-28T00:01:08Z","receivedAt":"2020-04-28T00:01:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"process_acks() returns 0, 1, or 2, depending on whether \"ready\" was\nreceived and if not, whether at least one commit was found to be common.\nReplace these magic numbers with a documented enum.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c | 35 ++++++++++++++++++++++++++++-------\n 1 file changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0b07b3ee73..45547a621e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1268,9 +1268,29 @@ static int process_section_header(struct packet_reader *reader,\n \treturn ret;\n }\n \n-static int process_acks(struct fetch_negotiator *negotiator,\n-\t\t\tstruct packet_reader *reader,\n-\t\t\tstruct oidset *common)\n+enum common_found {\n+\t/*\n+\t * No commit was found to be possessed by both the client and the\n+\t * server, and \"ready\" was not received.\n+\t */\n+\tNO_COMMON_FOUND,\n+\n+\t/*\n+\t * At least one commit was found to be possessed by both the client and\n+\t * the server, and \"ready\" was not received.\n+\t */\n+\tCOMMON_FOUND,\n+\n+\t/*\n+\t * \"ready\" was received, indicating that the server is ready to send\n+\t * the packfile without any further negotiation.\n+\t */\n+\tREADY\n+};\n+\n+static enum common_found process_acks(struct fetch_negotiator *negotiator,\n+\t\t\t\t      struct packet_reader *reader,\n+\t\t\t\t      struct oidset *common)\n {\n \t/* received */\n \tint received_ready = 0;\n@@ -1320,7 +1340,8 @@ static int process_acks(struct fetch_negotiator *negotiator,\n \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n \n \t/* return 0 if no common, 1 if there are common, or 2 if ready */\n-\treturn received_ready ? 2 : (received_ack ? 1 : 0);\n+\treturn received_ready ? READY :\n+\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n@@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_PROCESS_ACKS:\n \t\t\t/* Process ACKs/NAKs */\n \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n-\t\t\tcase 2:\n+\t\t\tcase READY:\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t\tbreak;\n-\t\t\tcase 1:\n+\t\t\tcase COMMON_FOUND:\n \t\t\t\tin_vain = 0;\n \t\t\t\t/* fallthrough */\n-\t\t\tdefault:\n+\t\t\tcase NO_COMMON_FOUND:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n \t\t\t\tbreak;\n \t\t\t}\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396414","messageId":"eb18faea2d00d9b09d3fcc79646cc7556023c258.1588031728.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1588031728.git.jonathantanmy@google.com","subject":"[PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-28T00:01:09Z","receivedAt":"2020-04-28T00:01:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching, Git stops negotiation when it has sent at least\nMAX_IN_VAIN (which is 256) \"have\" lines without having any of them\nACK-ed. But this is supposed to trigger only after the first ACK, as\npack-protocol.txt says:\n\n  However, the 256 limit *only* turns on in the canonical client\n  implementation if we have received at least one \"ACK %s continue\"\n  during a prior round.  This helps to ensure that at least one common\n  ancestor is found before we give up entirely.\n\nThe code path for protocol v0 observes this, but not protocol v2,\nresulting in shorter negotiation rounds but significantly larger\npackfiles. Teach the code path for protocol v2 to check this criterion\nonly after at least one ACK was received.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c          | 13 +++++++++----\n t/t5500-fetch-pack.sh | 18 ++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 45547a621e..76691dc6c0 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n }\n \n static int add_haves(struct fetch_negotiator *negotiator,\n+\t\t     int seen_ack,\n \t\t     struct strbuf *req_buf,\n \t\t     int *haves_to_send, int *in_vain)\n {\n@@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,\n \t}\n \n \t*in_vain += haves_added;\n-\tif (!haves_added || *in_vain >= MAX_IN_VAIN) {\n+\tif (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {\n \t\t/* Send Done */\n \t\tpacket_buf_write(req_buf, \"done\\n\");\n \t\tret = 1;\n@@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\t\t      struct fetch_pack_args *args,\n \t\t\t      const struct ref *wants, struct oidset *common,\n \t\t\t      int *haves_to_send, int *in_vain,\n-\t\t\t      int sideband_all)\n+\t\t\t      int sideband_all, int seen_ack)\n {\n \tint ret = 0;\n \tstruct strbuf req_buf = STRBUF_INIT;\n@@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t\tadd_common(&req_buf, common);\n \n \t\t/* Add initial haves */\n-\t\tret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);\n+\t\tret = add_haves(negotiator, seen_ack, &req_buf,\n+\t\t\t\thaves_to_send, in_vain);\n \t}\n \n \t/* Send request */\n@@ -1465,6 +1467,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tint haves_to_send = INITIAL_FLUSH;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n+\tint seen_ack = 0;\n \n \tif (args->no_dependents) {\n \t\tnegotiator = NULL;\n@@ -1521,7 +1524,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tif (send_fetch_request(negotiator, fd[1], args, ref,\n \t\t\t\t\t       &common,\n \t\t\t\t\t       &haves_to_send, &in_vain,\n-\t\t\t\t\t       reader.use_sideband))\n+\t\t\t\t\t       reader.use_sideband,\n+\t\t\t\t\t       seen_ack))\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\telse\n \t\t\t\tstate = FETCH_PROCESS_ACKS;\n@@ -1534,6 +1538,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\tbreak;\n \t\t\tcase COMMON_FOUND:\n \t\t\t\tin_vain = 0;\n+\t\t\t\tseen_ack = 1;\n \t\t\t\t/* fallthrough */\n \t\t\tcase NO_COMMON_FOUND:\n \t\t\t\tstate = FETCH_SEND_REQUEST;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex baa1a99f45..961cd6beec 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -385,6 +385,24 @@ test_expect_success 'clone shallow with packed refs' '\n \ttest_cmp count8.expected count8.actual\n '\n \n+test_expect_success 'in_vain not triggered before first ACK' '\n+\trm -rf myserver myclient trace &&\n+\tgit init myserver &&\n+\ttest_commit -C myserver foo &&\n+\tgit clone \"file://$(pwd)/myserver\" myclient &&\n+\n+\t# MAX_IN_VAIN is 256. Because of batching, the client will send 496\n+\t# (16+32+64+128+256) commits, not 256, before giving up. So create 496\n+\t# irrelevant commits.\n+\ttest_commit_bulk -C myclient 496 &&\n+\n+\t# The new commit that the client wants to fetch.\n+\ttest_commit -C myserver bar &&\n+\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n+\ttest_i18ngrep \"Total 3 \" trace\n+'\n+\n test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t(\n \t\tgit clone --bare --branch B --single-branch \"file://$(pwd)/.\" no-reflog &&\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396415","messageId":"cover.1588031728.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1587775989.git.jonathantanmy@google.com","subject":"[PATCH v2 0/3] Protocol v2 in_vain fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-28T00:01:07Z","receivedAt":"2020-04-28T00:01:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I'm not sure if Jonathan Nieder meant the enum-from-process_acks()\nchange should be in this patch set, but I've included it here anyway.\nAlso I've removed an extraneous debug \"cp\" from patch 2/3 (previously\npatch 1/2).\n\nJonathan Tan (3):\n  fetch-pack: return enum from process_acks()\n  fetch-pack: in protocol v2, in_vain only after ACK\n  fetch-pack: in protocol v2, reset in_vain upon ACK\n\n fetch-pack.c          | 49 +++++++++++++++++++++++++++++++++----------\n t/t5500-fetch-pack.sh | 48 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 86 insertions(+), 11 deletions(-)\n\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396416","messageId":"e89646cb9ab1164d866ba9747575cab9199e3f62.1588031728.git.jonathantanmy@google.com","threadId":"53314","inReplyTo":"cover.1588031728.git.jonathantanmy@google.com","subject":"[PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-28T00:01:10Z","receivedAt":"2020-04-28T00:01:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the function process_acks() in fetch-pack.c, the variable\nreceived_ack is meant to track that an ACK was received, but it was\nnever set. This results in negotiation terminating prematurely through\nthe in_vain counter, when the counter should have been reset upon every\nACK.\n\nTherefore, reset the in_vain counter upon every ACK.\n\nHelped-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c          |  1 +\n t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 76691dc6c0..edbfc2af1c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1307,6 +1307,7 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \n \t\tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n \t\t\tstruct object_id oid;\n+\t\t\treceived_ack = 1;\n \t\t\tif (!get_oid_hex(arg, &oid)) {\n \t\t\t\tstruct commit *commit;\n \t\t\t\toidset_insert(common, &oid);\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 961cd6beec..52dd1a688c 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -403,6 +403,36 @@ test_expect_success 'in_vain not triggered before first ACK' '\n \ttest_i18ngrep \"Total 3 \" trace\n '\n \n+test_expect_success 'in_vain resetted upon ACK' '\n+\trm -rf myserver myclient trace &&\n+\tgit init myserver &&\n+\n+\t# Linked list of commits on master. The first is common; the rest are\n+\t# not.\n+\ttest_commit -C myserver first_master_commit &&\n+\tgit clone \"file://$(pwd)/myserver\" myclient &&\n+\ttest_commit_bulk -C myclient 255 &&\n+\n+\t# Another linked list of commits on anotherbranch with no connection to\n+\t# master. The first is common; the rest are not.\n+\tgit -C myserver checkout --orphan anotherbranch &&\n+\ttest_commit -C myserver first_anotherbranch_commit &&\n+\tgit -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&\n+\tgit -C myclient checkout anotherbranch &&\n+\ttest_commit_bulk -C myclient 255 &&\n+\n+\t# The new commit that the client wants to fetch.\n+\tgit -C myserver checkout master &&\n+\ttest_commit -C myserver to_fetch &&\n+\n+\t# The client will send (as \"have\"s) all 256 commits in anotherbranch\n+\t# first. The 256th commit is common between the client and the server,\n+\t# and should reset in_vain. This allows negotiation to continue until\n+\t# the client reports that first_anotherbranch_commit is common.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n+\ttest_i18ngrep \"Total 3 \" trace\n+'\n+\n test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n \t(\n \t\tgit clone --bare --branch B --single-branch \"file://$(pwd)/.\" no-reflog &&\n-- \n2.26.2.303.gf8c07b1a785-goog\n\n"},{"id":"396420","messageId":"20200428005333.GE36078@google.com","threadId":"53314","inReplyTo":"f0cfbc03c27658e54a73c46570c5153961ed85b6.1588031728.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-28T00:53:33Z","receivedAt":"2020-04-28T00:53:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> process_acks() returns 0, 1, or 2, depending on whether \"ready\" was\n> received and if not, whether at least one commit was found to be common.\n> Replace these magic numbers with a documented enum.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  fetch-pack.c | 35 ++++++++++++++++++++++++++++-------\n>  1 file changed, 28 insertions(+), 7 deletions(-)\n\nYay!\n\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1268,9 +1268,29 @@ static int process_section_header(struct packet_reader *reader,\n>  \treturn ret;\n>  }\n>  \n> -static int process_acks(struct fetch_negotiator *negotiator,\n> -\t\t\tstruct packet_reader *reader,\n> -\t\t\tstruct oidset *common)\n> +enum common_found {\n> +\t/*\n> +\t * No commit was found to be possessed by both the client and the\n> +\t * server, and \"ready\" was not received.\n> +\t */\n> +\tNO_COMMON_FOUND,\n> +\n> +\t/*\n> +\t * At least one commit was found to be possessed by both the client and\n> +\t * the server, and \"ready\" was not received.\n> +\t */\n> +\tCOMMON_FOUND,\n> +\n> +\t/*\n> +\t * \"ready\" was received, indicating that the server is ready to send\n> +\t * the packfile without any further negotiation.\n> +\t */\n> +\tREADY\n> +};\n> +\n> +static enum common_found process_acks(struct fetch_negotiator *negotiator,\n> +\t\t\t\t      struct packet_reader *reader,\n> +\t\t\t\t      struct oidset *common)\n\nThanks for the clear comments.\n\n>  {\n>  \t/* received */\n>  \tint received_ready = 0;\n> @@ -1320,7 +1340,8 @@ static int process_acks(struct fetch_negotiator *negotiator,\n>  \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n>  \n>  \t/* return 0 if no common, 1 if there are common, or 2 if ready */\n\nThis comment can be removed now, which is a nice bonus.\n\n> -\treturn received_ready ? 2 : (received_ack ? 1 : 0);\n> +\treturn received_ready ? READY :\n> +\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n>  }\n>  \n>  static void receive_shallow_info(struct fetch_pack_args *args,\n> @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>  \t\tcase FETCH_PROCESS_ACKS:\n>  \t\t\t/* Process ACKs/NAKs */\n>  \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n> -\t\t\tcase 2:\n> +\t\t\tcase READY:\n>  \t\t\t\tstate = FETCH_GET_PACK;\n>  \t\t\t\tbreak;\n> -\t\t\tcase 1:\n> +\t\t\tcase COMMON_FOUND:\n>  \t\t\t\tin_vain = 0;\n>  \t\t\t\t/* fallthrough */\n> -\t\t\tdefault:\n> +\t\t\tcase NO_COMMON_FOUND:\n>  \t\t\t\tstate = FETCH_SEND_REQUEST;\n>  \t\t\t\tbreak;\n>  \t\t\t}\n\nWith the extraneous comment removed,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\ndiff --git i/fetch-pack.c w/fetch-pack.c\nindex 45547a621e6..7b0eef98db5 100644\n--- i/fetch-pack.c\n+++ w/fetch-pack.c\n@@ -1339,7 +1339,6 @@ static enum common_found process_acks(struct fetch_negotiator *negotiator,\n \tif (!received_ready && reader->status != PACKET_READ_FLUSH)\n \t\tdie(_(\"expected no other sections to be sent after no 'ready'\"));\n \n-\t/* return 0 if no common, 1 if there are common, or 2 if ready */\n \treturn received_ready ? READY :\n \t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n }\n"},{"id":"396421","messageId":"20200428005431.GF36078@google.com","threadId":"53314","inReplyTo":"eb18faea2d00d9b09d3fcc79646cc7556023c258.1588031728.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-28T00:54:31Z","receivedAt":"2020-04-28T00:54:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  fetch-pack.c          | 13 +++++++++----\n>  t/t5500-fetch-pack.sh | 18 ++++++++++++++++++\n>  2 files changed, 27 insertions(+), 4 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"396422","messageId":"20200428005509.GG36078@google.com","threadId":"53314","inReplyTo":"e89646cb9ab1164d866ba9747575cab9199e3f62.1588031728.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/3] fetch-pack: in protocol v2, reset in_vain upon ACK","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-28T00:55:09Z","receivedAt":"2020-04-28T00:55:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> In the function process_acks() in fetch-pack.c, the variable\n> received_ack is meant to track that an ACK was received, but it was\n> never set. This results in negotiation terminating prematurely through\n> the in_vain counter, when the counter should have been reset upon every\n> ACK.\n>\n> Therefore, reset the in_vain counter upon every ACK.\n>\n> Helped-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  fetch-pack.c          |  1 +\n>  t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++\n>  2 files changed, 31 insertions(+)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"396475","messageId":"xmqqftcn4klg.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"20200428005333.GE36078@google.com","subject":"Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T16:54:19Z","receivedAt":"2020-04-28T16:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>  \t/* return 0 if no common, 1 if there are common, or 2 if ready */\n>\n> This comment can be removed now, which is a nice bonus.\n>\n>> -\treturn received_ready ? 2 : (received_ack ? 1 : 0);\n>> +\treturn received_ready ? READY :\n>> +\t\t(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);\n>>  }\n>>  \n>>  static void receive_shallow_info(struct fetch_pack_args *args,\n>> @@ -1508,13 +1529,13 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n>>  \t\tcase FETCH_PROCESS_ACKS:\n>>  \t\t\t/* Process ACKs/NAKs */\n>>  \t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n>> -\t\t\tcase 2:\n>> +\t\t\tcase READY:\n>>  \t\t\t\tstate = FETCH_GET_PACK;\n>>  \t\t\t\tbreak;\n>> -\t\t\tcase 1:\n>> +\t\t\tcase COMMON_FOUND:\n>>  \t\t\t\tin_vain = 0;\n>>  \t\t\t\t/* fallthrough */\n>> -\t\t\tdefault:\n>> +\t\t\tcase NO_COMMON_FOUND:\n>>  \t\t\t\tstate = FETCH_SEND_REQUEST;\n>>  \t\t\t\tbreak;\n>>  \t\t\t}\n>\n> With the extraneous comment removed,\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nYes, the result reads very clearly.\n\nLocally amended.\n\nThanks.\n"},{"id":"396482","messageId":"20200428200056.6fb5dd6b@naga.burning-in.hell","threadId":"53314","inReplyTo":"xmqqftcn4klg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2020-04-28T18:00:56Z","receivedAt":"2020-04-28T18:01:00Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Tue, 28 Apr 2020 09:54:19 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> >>  \t/* return 0 if no common, 1 if there are common, or 2 if ready */  \n> >\n> > This comment can be removed now, which is a nice bonus.\nIt is still present in the pu branch.\n\nIs there other branch where I should look?\n\nThanks\n\nMichal\n"},{"id":"396490","messageId":"xmqq4kt32zer.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"20200428200056.6fb5dd6b@naga.burning-in.hell","subject":"Re: [PATCH v2 1/3] fetch-pack: return enum from process_acks()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T19:17:16Z","receivedAt":"2020-04-28T19:17:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Suchánek <msuchanek@suse.de> writes:\n\n> On Tue, 28 Apr 2020 09:54:19 -0700\n> Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Jonathan Nieder <jrnieder@gmail.com> writes:\n>> \n>> >>  \t/* return 0 if no common, 1 if there are common, or 2 if ready */  \n>> >\n>> > This comment can be removed now, which is a nice bonus.\n> It is still present in the pu branch.\n>\n> Is there other branch where I should look?\n\nSome branch that is pushed out _after_ I push out, perhaps?\n"},{"id":"397237","messageId":"nycvar.QRO.7.76.6.2005062304410.56@tvgsbejvaqbjf.bet","threadId":"53314","inReplyTo":"eb18faea2d00d9b09d3fcc79646cc7556023c258.1588031728.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/3] fetch-pack: in protocol v2, in_vain only after ACK","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-06T21:08:43Z","receivedAt":"2020-05-06T21:08:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Mon, 27 Apr 2020, Jonathan Tan wrote:\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index baa1a99f45..961cd6beec 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -385,6 +385,24 @@ test_expect_success 'clone shallow with packed refs' '\n>  \ttest_cmp count8.expected count8.actual\n>  '\n>\n> +test_expect_success 'in_vain not triggered before first ACK' '\n> +\trm -rf myserver myclient trace &&\n> +\tgit init myserver &&\n> +\ttest_commit -C myserver foo &&\n> +\tgit clone \"file://$(pwd)/myserver\" myclient &&\n> +\n> +\t# MAX_IN_VAIN is 256. Because of batching, the client will send 496\n> +\t# (16+32+64+128+256) commits, not 256, before giving up. So create 496\n> +\t# irrelevant commits.\n> +\ttest_commit_bulk -C myclient 496 &&\n> +\n> +\t# The new commit that the client wants to fetch.\n> +\ttest_commit -C myserver bar &&\n> +\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n> +\ttest_i18ngrep \"Total 3 \" trace\n\nThis just failed in one of the Pipelines I monitor:\nhttps://github.com/git-for-windows/git-sdk-64/runs/648253955?check_suite_focus=true\n\nThe short of it is:\n\n-- snip --\n[...]\npacket:     sideband< \\2Enumerating objects: 4, done.\npacket:     sideband< \\2Counting objects: 25% (1/4)\\15Counting objects:  50% (2/4)\\15Counting objects:  75% (3/4)\\15Counting objects: 100% (4/4)\\15Counting obj\npacket:     sideband< \\2ects: 100% (4/4), done.Compressing objects:  50% (1/2)\\15Compressing objects: 100% (2/2)\\15Compressing objects: 100% (2/2), done.T\npacket:     sideband< \\2otal 3 (delta 0), reused 0 (delta 0), pack-reused 0\npacket:     sideband< PACK ...\npacket:  upload-pack> 0000\npacket:     sideband< 0000\n++ return 1\nerror: last command exited with $?=1\nt/t5500-fetch-pack.sh:388: error: not ok 43 - in_vain not triggered before first ACK\n#\n#\t\trm -rf myserver myclient trace &&\n#\t\tgit init myserver &&\n#\t\ttest_commit -C myserver foo &&\n#\t\tgit clone \"file://$(pwd)/myserver\" myclient &&\n#\n#\t\t# MAX_IN_VAIN is 256. Because of batching, the client will send 496\n#\t\t# (16+32+64+128+256) commits, not 256, before giving up. So create 496\n#\t\t# irrelevant commits.\n#\t\ttest_commit_bulk -C myclient 496 &&\n#\n#\t\t# The new commit that the client wants to fetch.\n#\t\ttest_commit -C myserver bar &&\n#\n#\t\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n#\t\ttest_i18ngrep \"Total 3 \" trace\n-- snap --\n\nIn other words, that `test_i18ngrep` assumes incorrectly that \"Total 3\"\nwill be found in the trace, but the sideband is totally allowed to cut\nthrough it and deliver it in different packets.\n\nThis makes t5500 flaky.\n\nCiao,\nJohannes\n\n> +'\n> +\n>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n>  \t(\n>  \t\tgit clone --bare --branch B --single-branch \"file://$(pwd)/.\" no-reflog &&\n> --\n> 2.26.2.303.gf8c07b1a785-goog\n>\n>\n>\n"},{"id":"397246","messageId":"20200506220741.71021-1-jonathantanmy@google.com","threadId":"53314","inReplyTo":"nycvar.QRO.7.76.6.2005062304410.56@tvgsbejvaqbjf.bet","subject":"[PATCH] t5500: count objects through stderr, not trace","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-06T22:07:40Z","receivedAt":"2020-05-06T22:07:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In two tests introduced by 4fa3f00abb (\"fetch-pack: in protocol v2,\nin_vain only after ACK\", 2020-04-28) and 2f0a093dd6 (\"fetch-pack: in\nprotocol v2, reset in_vain upon ACK\", 2020-04-28), the count of objects\ndownloaded is checked by grepping for a specific message in the packet\ntrace. However, this is flaky as that specific message may be delivered\nover 2 or more packet lines.\n\nInstead, grep over stderr, just like the \"fetch creating new shallow\nroot\" test in the same file.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks, Dscho. The commits introducing the flakiness have made it to\nmaster, so this commit is on master.\n---\n t/t5500-fetch-pack.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 52dd1a688c..8c54e34ef1 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '\n '\n \n test_expect_success 'in_vain not triggered before first ACK' '\n-\trm -rf myserver myclient trace &&\n+\trm -rf myserver myclient &&\n \tgit init myserver &&\n \ttest_commit -C myserver foo &&\n \tgit clone \"file://$(pwd)/myserver\" myclient &&\n@@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '\n \t# The new commit that the client wants to fetch.\n \ttest_commit -C myserver bar &&\n \n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n-\ttest_i18ngrep \"Total 3 \" trace\n+\tgit -C myclient fetch --progress origin 2>log &&\n+\ttest_i18ngrep \"remote: Total 3 \" log\n '\n \n test_expect_success 'in_vain resetted upon ACK' '\n-\trm -rf myserver myclient trace &&\n+\trm -rf myserver myclient &&\n \tgit init myserver &&\n \n \t# Linked list of commits on master. The first is common; the rest are\n@@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '\n \t# first. The 256th commit is common between the client and the server,\n \t# and should reset in_vain. This allows negotiation to continue until\n \t# the client reports that first_anotherbranch_commit is common.\n-\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n-\ttest_i18ngrep \"Total 3 \" trace\n+\tgit -C myclient fetch --progress origin master 2>log &&\n+\ttest_i18ngrep \"Total 3 \" log\n '\n \n test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n-- \n2.26.2.526.g744177e7f7-goog\n\n"},{"id":"397247","messageId":"xmqqd07g66l1.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"20200506220741.71021-1-jonathantanmy@google.com","subject":"Re: [PATCH] t5500: count objects through stderr, not trace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T22:28:58Z","receivedAt":"2020-05-06T22:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> In two tests introduced by 4fa3f00abb (\"fetch-pack: in protocol v2,\n> in_vain only after ACK\", 2020-04-28) and 2f0a093dd6 (\"fetch-pack: in\n> protocol v2, reset in_vain upon ACK\", 2020-04-28), the count of objects\n> downloaded is checked by grepping for a specific message in the packet\n> trace. However, this is flaky as that specific message may be delivered\n> over 2 or more packet lines.\n>\n> Instead, grep over stderr, just like the \"fetch creating new shallow\n> root\" test in the same file.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks, Dscho. The commits introducing the flakiness have made it to\n> master, so this commit is on master.\n\nHmph.  Do we coalesce the payload we receive in adjacent two packet\nlines into one before writing it out, or do we know in this codepath\nthat the progress messages are the only thing we are writing out to\nthe standard error stream and we do not need to worry about other\nstuff getting mixed in?  If that is the case, perhaps \"just like we\nalready do in another test\" is a lot weaker justification (it sounds\nas if we are saying \"another test is doing this thing, so if it is\nbroken, we are not making things worse\") than what you actually\nhave (\"the only thing we are writing out to the standard error\nstream in the non-error case is these progress messages, and the\n'Total 3 ' string, even if it is carried in two separate fragments,\nwill be shown without getting mixed with anything else on the\nstandard error stream\").\n\nAssuming that the way I (re)read your justification is correct, the\npatch looks good to me.\n\nThanks, both.\n\n>  t/t5500-fetch-pack.sh | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 52dd1a688c..8c54e34ef1 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '\n>  '\n>  \n>  test_expect_success 'in_vain not triggered before first ACK' '\n> -\trm -rf myserver myclient trace &&\n> +\trm -rf myserver myclient &&\n>  \tgit init myserver &&\n>  \ttest_commit -C myserver foo &&\n>  \tgit clone \"file://$(pwd)/myserver\" myclient &&\n> @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '\n>  \t# The new commit that the client wants to fetch.\n>  \ttest_commit -C myserver bar &&\n>  \n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n> -\ttest_i18ngrep \"Total 3 \" trace\n> +\tgit -C myclient fetch --progress origin 2>log &&\n> +\ttest_i18ngrep \"remote: Total 3 \" log\n>  '\n>  \n>  test_expect_success 'in_vain resetted upon ACK' '\n> -\trm -rf myserver myclient trace &&\n> +\trm -rf myserver myclient &&\n>  \tgit init myserver &&\n>  \n>  \t# Linked list of commits on master. The first is common; the rest are\n> @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '\n>  \t# first. The 256th commit is common between the client and the server,\n>  \t# and should reset in_vain. This allows negotiation to continue until\n>  \t# the client reports that first_anotherbranch_commit is common.\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n> -\ttest_i18ngrep \"Total 3 \" trace\n> +\tgit -C myclient fetch --progress origin master 2>log &&\n> +\ttest_i18ngrep \"Total 3 \" log\n>  '\n>  \n>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n"},{"id":"397248","messageId":"xmqq8si4661k.fsf@gitster.c.googlers.com","threadId":"53314","inReplyTo":"xmqqd07g66l1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] t5500: count objects through stderr, not trace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T22:40:39Z","receivedAt":"2020-05-06T22:40:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Thanks, Dscho. The commits introducing the flakiness have made it to\n>> master, so this commit is on master.\n\nI'll queue directly on top of jt/v2-fetch-neg-fix as that topic was\ndesigned to be later mergeable down to 'maint' so that the 2.26\ntrack can (1) revert the default flip so protocol v0 is restored as\nthe default, and (2) breakage of negotiataion in protocol v2 is\nfixed.\n\nThanks.\n"},{"id":"397309","messageId":"nycvar.QRO.7.76.6.2005071635270.56@tvgsbejvaqbjf.bet","threadId":"53314","inReplyTo":"20200506220741.71021-1-jonathantanmy@google.com","subject":"Re: [PATCH] t5500: count objects through stderr, not trace","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-07T14:35:56Z","receivedAt":"2020-05-07T14:36:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 6 May 2020, Jonathan Tan wrote:\n\n> In two tests introduced by 4fa3f00abb (\"fetch-pack: in protocol v2,\n> in_vain only after ACK\", 2020-04-28) and 2f0a093dd6 (\"fetch-pack: in\n> protocol v2, reset in_vain upon ACK\", 2020-04-28), the count of objects\n> downloaded is checked by grepping for a specific message in the packet\n> trace. However, this is flaky as that specific message may be delivered\n> over 2 or more packet lines.\n>\n> Instead, grep over stderr, just like the \"fetch creating new shallow\n> root\" test in the same file.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks, Dscho. The commits introducing the flakiness have made it to\n> master, so this commit is on master.\n\nThank you for fixing this so quickly. I agree that the patch addresses the\nunderlying problem.\n\nThanks!\nDscho\n\n> ---\n>  t/t5500-fetch-pack.sh | 12 ++++++------\n>  1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 52dd1a688c..8c54e34ef1 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '\n>  '\n>\n>  test_expect_success 'in_vain not triggered before first ACK' '\n> -\trm -rf myserver myclient trace &&\n> +\trm -rf myserver myclient &&\n>  \tgit init myserver &&\n>  \ttest_commit -C myserver foo &&\n>  \tgit clone \"file://$(pwd)/myserver\" myclient &&\n> @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '\n>  \t# The new commit that the client wants to fetch.\n>  \ttest_commit -C myserver bar &&\n>\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin &&\n> -\ttest_i18ngrep \"Total 3 \" trace\n> +\tgit -C myclient fetch --progress origin 2>log &&\n> +\ttest_i18ngrep \"remote: Total 3 \" log\n>  '\n>\n>  test_expect_success 'in_vain resetted upon ACK' '\n> -\trm -rf myserver myclient trace &&\n> +\trm -rf myserver myclient &&\n>  \tgit init myserver &&\n>\n>  \t# Linked list of commits on master. The first is common; the rest are\n> @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '\n>  \t# first. The 256th commit is common between the client and the server,\n>  \t# and should reset in_vain. This allows negotiation to continue until\n>  \t# the client reports that first_anotherbranch_commit is common.\n> -\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C myclient fetch --progress origin master &&\n> -\ttest_i18ngrep \"Total 3 \" trace\n> +\tgit -C myclient fetch --progress origin master 2>log &&\n> +\ttest_i18ngrep \"Total 3 \" log\n>  '\n>\n>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '\n> --\n> 2.26.2.526.g744177e7f7-goog\n>\n>\n"},{"id":"407450","messageId":"nycvar.QRO.7.76.6.2010131644310.50@tvgsbejvaqbjf.bet","threadId":"53314","inReplyTo":"20200506220741.71021-1-jonathantanmy@google.com","subject":"Re: [PATCH] t5500: count objects through stderr, not trace","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-10-13T14:45:38Z","receivedAt":"2020-10-13T14:45:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 6 May 2020, Jonathan Tan wrote:\n\n> In two tests introduced by 4fa3f00abb (\"fetch-pack: in protocol v2,\n> in_vain only after ACK\", 2020-04-28) and 2f0a093dd6 (\"fetch-pack: in\n> protocol v2, reset in_vain upon ACK\", 2020-04-28), the count of objects\n> downloaded is checked by grepping for a specific message in the packet\n> trace. However, this is flaky as that specific message may be delivered\n> over 2 or more packet lines.\n>\n> Instead, grep over stderr, just like the \"fetch creating new shallow\n> root\" test in the same file.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks, Dscho. The commits introducing the flakiness have made it to\n> master, so this commit is on master.\n\nSorry for the blast from the past. Apparently, even with this fix, the\nfirst test is still flaky.\n\nI submitted a patch to work around the bug via\nhttps://github.com/gitgitgadget/git/pull/753; Please review when you have\nsome time.\n\nThanks,\nDscho\n"}]}