[PATCH] negotiator/skipping: parse commit before queueing

[PATCH] negotiator/skipping: parse commit before queueing

[Jonathan Tan]

The skipping negotiator pushes entries onto the priority queue without
ensuring that the commit is parsed, resulting in the entry ending up in
the wrong position due to a lack of commit time. Fix this by parsing the
commit whenever we push an entry onto the priority queue.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This was noticed by Aevar in [1]. With this fix, 163 "have" lines are
produced instead of the 14002 reported in [1].

I have included a test to demonstrate the issue, but I'm not sure if
it's worth including it in the source tree.

[1] https://public-inbox.org/git/87o9ciisg6.fsf@evledraar.gmail.com/
---
 negotiator/skipping.c                |  2 +-
 t/t5552-skipping-fetch-negotiator.sh | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index dffbc76c49..9ce0555840 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -64,6 +64,7 @@ static struct entry *rev_list_push(struct data *data, struct commit *commit, int
 
 	entry = xcalloc(1, sizeof(*entry));
 	entry->commit = commit;
+	parse_commit(commit);
 	prio_queue_put(&data->rev_list, entry);
 
 	if (!(mark & COMMON))
@@ -177,7 +178,6 @@ static const struct object_id *get_rev(struct data *data)
 		if (!(commit->object.flags & COMMON) && !entry->ttl)
 			to_send = commit;
 
-		parse_commit(commit);
 		for (p = commit->parents; p; p = p->next)
 			parent_pushed |= push_parent(data, entry, p->item);
 
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..f2f938e54e 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -83,6 +83,28 @@ test_expect_success 'unknown fetch.negotiationAlgorithm values error out' '
 	test_i18ngrep ! "unknown fetch negotiation algorithm" err
 '
 
+test_expect_success 'works in absence of tags' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+	for i in $(test_seq 11)
+	do
+		test_commit -C client c$i
+	done &&
+	git -C client tag middle c6 &&
+	for i in $(test_seq 11)
+	do
+		git -C client tag -d c$i
+	done &&
+
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	trace_fetch client "$(pwd)/server" &&
+	have_sent HEAD HEAD~2 HEAD~5 HEAD~10 &&
+	have_not_sent HEAD~1 HEAD~3 HEAD~4 HEAD~6 HEAD~7 HEAD~8 HEAD~9
+'
+
 test_expect_success 'when two skips collide, favor the larger one' '
 	rm -rf server client trace &&
 	git init server &&
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH] negotiator/skipping: parse commit before queueing

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Jonathan Tan wrote:

> The skipping negotiator pushes entries onto the priority queue without
> ensuring that the commit is parsed, resulting in the entry ending up in
> the wrong position due to a lack of commit time. Fix this by parsing the
> commit whenever we push an entry onto the priority queue.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

Thanks for the prompt fix!

> This was noticed by Aevar in [1]. With this fix, 163 "have" lines are
> produced instead of the 14002 reported in [1].
>
> I have included a test to demonstrate the issue, but I'm not sure if
> it's worth including it in the source tree.

The test fails before the patch, and passes after, and tests that we do
the right thing here. Seems best to include such tests whenever we can.