{"thread":{"id":"49447","subject":"[PATCH] negotiator/skipping: parse commit before queueing","startedAt":"2018-09-27T23:19:35Z","lastAt":"2018-09-27T23:29:57Z","messageCount":2,"participants":["Jonathan Tan","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359161","messageId":"20180927231929.180599-1-jonathantanmy@google.com","threadId":"49447","inReplyTo":null,"subject":"[PATCH] negotiator/skipping: parse commit before queueing","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T23:19:29Z","receivedAt":"2018-09-27T23:19:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The skipping negotiator pushes entries onto the priority queue without\nensuring that the commit is parsed, resulting in the entry ending up in\nthe wrong position due to a lack of commit time. Fix this by parsing the\ncommit whenever we push an entry onto the priority queue.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis was noticed by Aevar in [1]. With this fix, 163 \"have\" lines are\nproduced instead of the 14002 reported in [1].\n\nI have included a test to demonstrate the issue, but I'm not sure if\nit's worth including it in the source tree.\n\n[1] https://public-inbox.org/git/87o9ciisg6.fsf@evledraar.gmail.com/\n---\n negotiator/skipping.c                |  2 +-\n t/t5552-skipping-fetch-negotiator.sh | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/negotiator/skipping.c b/negotiator/skipping.c\nindex dffbc76c49..9ce0555840 100644\n--- a/negotiator/skipping.c\n+++ b/negotiator/skipping.c\n@@ -64,6 +64,7 @@ static struct entry *rev_list_push(struct data *data, struct commit *commit, int\n \n \tentry = xcalloc(1, sizeof(*entry));\n \tentry->commit = commit;\n+\tparse_commit(commit);\n \tprio_queue_put(&data->rev_list, entry);\n \n \tif (!(mark & COMMON))\n@@ -177,7 +178,6 @@ static const struct object_id *get_rev(struct data *data)\n \t\tif (!(commit->object.flags & COMMON) && !entry->ttl)\n \t\t\tto_send = commit;\n \n-\t\tparse_commit(commit);\n \t\tfor (p = commit->parents; p; p = p->next)\n \t\t\tparent_pushed |= push_parent(data, entry, p->item);\n \ndiff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh\nindex 30857b84a8..f2f938e54e 100755\n--- a/t/t5552-skipping-fetch-negotiator.sh\n+++ b/t/t5552-skipping-fetch-negotiator.sh\n@@ -83,6 +83,28 @@ test_expect_success 'unknown fetch.negotiationAlgorithm values error out' '\n \ttest_i18ngrep ! \"unknown fetch negotiation algorithm\" err\n '\n \n+test_expect_success 'works in absence of tags' '\n+\trm -rf server client trace &&\n+\tgit init server &&\n+\ttest_commit -C server to_fetch &&\n+\n+\tgit init client &&\n+\tfor i in $(test_seq 11)\n+\tdo\n+\t\ttest_commit -C client c$i\n+\tdone &&\n+\tgit -C client tag middle c6 &&\n+\tfor i in $(test_seq 11)\n+\tdo\n+\t\tgit -C client tag -d c$i\n+\tdone &&\n+\n+\ttest_config -C client fetch.negotiationalgorithm skipping &&\n+\ttrace_fetch client \"$(pwd)/server\" &&\n+\thave_sent HEAD HEAD~2 HEAD~5 HEAD~10 &&\n+\thave_not_sent HEAD~1 HEAD~3 HEAD~4 HEAD~6 HEAD~7 HEAD~8 HEAD~9\n+'\n+\n test_expect_success 'when two skips collide, favor the larger one' '\n \trm -rf server client trace &&\n \tgit init server &&\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359163","messageId":"87lg7miknk.fsf@evledraar.gmail.com","threadId":"49447","inReplyTo":"20180927231929.180599-1-jonathantanmy@google.com","subject":"Re: [PATCH] negotiator/skipping: parse commit before queueing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-27T23:29:51Z","receivedAt":"2018-09-27T23:29:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 27 2018, Jonathan Tan wrote:\n\n> The skipping negotiator pushes entries onto the priority queue without\n> ensuring that the commit is parsed, resulting in the entry ending up in\n> the wrong position due to a lack of commit time. Fix this by parsing the\n> commit whenever we push an entry onto the priority queue.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n\nThanks for the prompt fix!\n\n> This was noticed by Aevar in [1]. With this fix, 163 \"have\" lines are\n> produced instead of the 14002 reported in [1].\n>\n> I have included a test to demonstrate the issue, but I'm not sure if\n> it's worth including it in the source tree.\n\nThe test fails before the patch, and passes after, and tests that we do\nthe right thing here. Seems best to include such tests whenever we can.\n"}]}