On Mon, Jun 20, 2022 at 3:34 PM Patrick Steinhardt [off-list ref] wrote:

On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote:

quoted

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/ (local)

Signed-off-by: Han Xin <redacted>

Thanks a lot for working on this issue!

quoted

---
 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.

quoted

      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.

Nod.
There is indeed some misleading information here.
I will adjust it in the next patch.

quoted

+     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`.

Nod.

quoted

+
+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?

Could you help me find the reason why this testcase passes even
without the fix.

From the execution of Github Action, it seems that the problem always exist：
https://github.com/chiyutianyi/git/actions/runs/2527421443.

Thanks.
-Han Xin

quoted

+test_done
--
2.36.1

Patrick