git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()

From
Patrick Steinhardt <ps@pks.im>
Date
Jun 20, 2022, 07:07 UTC
Message-ID
<YrAcrNApaZDngLL+@ncase>
In-Reply-To
<20220618030130.36419-1-hanxin.hx@bytedance.com>
On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote:
Show 10 quoted lines
> If a commit is in the commit graph, we would expect the commit to also
> be present. So we should use has_object() instead of
> repo_has_object_file(), which will help us avoid getting into an endless
> loop of lazy fetch.
> 
> We can see the endless loop issue via this[1].
> 
> 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/
> 
> Signed-off-by: Han Xin <hanxin.hx@bytedance.com>
Thanks a lot for working on this issue!
Show 17 quoted lines
> ---
>  commit-graph.c                             |  2 +-
>  t/t5329-no-lazy-fetch-with-commit-graph.sh | 50 ++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index 2b52818731..2dd9bcc7ea 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje
>  		return NULL;
>  	if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
>  		return NULL;
> -	if (!repo_has_object_file(repo, id))
> +	if (!has_object(repo, id, 0))
>  		return NULL;
Agreed, this change makes sense to me.
Show 14 quoted lines
>  	commit = lookup_commit(repo, id);
> diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh
> new file mode 100755
> index 0000000000..ea5940b9f1
> --- /dev/null
> +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh
> @@ -0,0 +1,50 @@
> +#!/bin/sh
> +
> +test_description='test for no lazy fetch with the commit-graph'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
Nit: I find it a bit confusing that you only call the first test case
`setup`, while all the other tests except for the last one are also only
setting up the necessary state.
Show 18 quoted lines
> +	git init --bare dest.git &&
> +	test_commit one &&
> +	git checkout -b tmp &&
> +	test_commit two &&
> +	git push dest.git --all
> +'
> +
> +test_expect_success 'prepare a alternates repository without commit two' '
> 
> +	git clone --bare dest.git alternates &&
> 
> +	oid=$(git -C alternates rev-parse refs/heads/tmp) &&
> +	git -C alternates update-ref -d refs/heads/tmp &&
> +	git -C alternates gc --prune=now &&
> +	pack=$(echo alternates/objects/pack/*.pack) &&
> +	git verify-pack -v "$pack" >have &&
> +	! grep "$oid" have
> +'

Instead of going into the low-level details you could just verify that `git cat-file -e $oid` returns `1`.

Show 25 quoted lines
> +
> +test_expect_success 'prepare a repository with a commit-graph contains commit two' '
> +	git init source &&
> +	echo "$(pwd)/dest.git/objects" >source/.git/objects/info/alternates &&
> +	git -C source remote add origin "$(pwd)/dest.git" &&
> +	git -C source config remote.origin.promisor true &&
> +	git -C source config remote.origin.partialclonefilter blob:none &&
> +	# the source repository has the whole refs contains refs/heads/tmp
> +	git -C source fetch origin &&
> +	(
> +		cd source &&
> +		test_commit three &&
> +		git -c gc.writeCommitGraph=true gc
> +	)
> +'
> +
> +test_expect_success 'change the alternates of source to that without commit two' '
> +	# now we have a commit-graph in the source repository but without the commit two
> +	echo "$(pwd)/alternates/objects" >source/.git/objects/info/alternates
> +'
> +
> +test_expect_success 'fetch the missing commit' '
> +	git -C source fetch origin $oid 2>fetch.out &&
> +	grep "$oid" fetch.out
> +'

This test passes even without your fix, albeit a lot slower compared to with it. Can we somehow cause it to fail reliably so that the test becomes effective in catching a regression here?

> +test_done
> -- 
> 2.36.1
> 
Patrick
Previous: Han XinNext: 欣韩
Message 8 of 50 in “Re: An endless loop fetching issue with partial clone, alternates and commit graph”
  1. Haiyng TanJun 14, 2022
  2. Taylor BlauJun 15, 2022
  3. 0/2 Re: An endless loop fetching issue with partial clone, alternates and commit graphHan Xin, Jun 16, 2022
  4. 1/2 commit-graph.c: add "flags" to lookup_commit_in_graph()Han Xin, Jun 16, 2022
  5. 2/2 fetch-pack.c: pass "oi_flags" to lookup_commit_in_graph()Han Xin, Jun 16, 2022
  6. Jonathan TanJun 17, 2022
  7. commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jun 18, 2022
  8. Patrick SteinhardtJun 20, 2022
  9. 欣韩Jun 20, 2022
  10. Patrick SteinhardtJun 20, 2022
  11. Jonathan TanJun 21, 2022
  12. Han XinJun 22, 2022
  13. 0/2 commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jun 24, 2022
  14. 1/2 test-lib.sh: add limited processes to test-libHan Xin, Jun 24, 2022
  15. Junio C HamanoJun 24, 2022
  16. Han XinJun 25, 2022
  17. Junio C HamanoJun 27, 2022
  18. 2/2 commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jun 24, 2022
  19. Junio C HamanoJun 24, 2022
  20. Han XinJun 25, 2022
  21. Han XinJun 25, 2022
  22. 0/2 no lazy fetch in lookup_commit_in_graph()Han Xin, Jun 28, 2022
  23. 1/2 test-lib.sh: add limited processes to test-libHan Xin, Jun 28, 2022
  24. 2/2 commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jun 28, 2022
  25. Ævar Arnfjörð BjarmasonJun 28, 2022
  26. Junio C HamanoJun 28, 2022
  27. Johannes SchindelinJun 30, 2022
  28. Ævar Arnfjörð BjarmasonJun 30, 2022
  29. Junio C HamanoJun 30, 2022
  30. Ævar Arnfjörð BjarmasonJun 30, 2022
  31. Johannes SchindelinJul 1, 2022
  32. Junio C HamanoJul 1, 2022
  33. Han XinJun 29, 2022
  34. test name conflict + js/ci-github-workflow-markup regression (was: [PATCH v3 0/2] no lazy fetch in lookup_commit_in_graph())Ævar Arnfjörð Bjarmason, Jun 30, 2022
  35. 0/1 no lazy fetch in lookup_commit_in_graph()Han Xin, Jul 1, 2022
  36. 1/1 commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jul 1, 2022
  37. Michael J GruberJul 9, 2022
  38. Jeff KingJul 11, 2022
  39. Junio C HamanoJul 11, 2022
  40. Han XinJul 12, 2022
  41. Junio C HamanoJul 12, 2022
  42. Han XinJul 12, 2022
  43. Jeff KingJul 12, 2022
  44. Junio C HamanoJul 12, 2022
  45. 0/1 no lazy fetch in lookup_commit_in_graph()Han Xin, Jul 12, 2022
  46. 1/1 commit-graph.c: no lazy fetch in lookup_commit_in_graph()Han Xin, Jul 12, 2022
  47. Ævar Arnfjörð BjarmasonJul 12, 2022
  48. Han XinJul 13, 2022
  49. Jeff KingJul 12, 2022
  50. t5330: remove run_with_limited_processses()Han Xin, Jul 12, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.