On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote: > 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@xxxxxxxxx/ > > Signed-off-by: Han Xin <hanxin.hx@xxxxxxxxxxxxx> Thanks a lot for working on this issue! > --- > 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. > 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. > + 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`. > + > +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
Attachment:
signature.asc
Description: PGP signature
