{"thread":{"id":"57995","subject":"Re: An endless loop fetching issue with partial clone, alternates and commit graph","startedAt":"2022-06-14T07:25:30Z","lastAt":"2022-07-13T01:27:06Z","messageCount":50,"participants":["Haiyng Tan","Taylor Blau","Han Xin","Jonathan Tan","Patrick Steinhardt","欣韩","Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Michael J Gruber","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"457173","messageId":"CANe9W27GVn-w1WSZNTxh5SKEMzHGEZQCF48vmbvMi4AUEg12yQ@mail.gmail.com","threadId":"57995","inReplyTo":null,"subject":"Re: An endless loop fetching issue with partial clone, alternates and commit graph","fromName":"Haiyng Tan","fromEmail":"haiyangtand@gmail.com","sentAt":"2022-06-14T07:25:13Z","receivedAt":"2022-06-14T07:25:30Z","isPatch":false,"sender":{"key":"haiyangtand@gmail.com","avatar":null},"body":"On Mon, 13 Jun 2022 00:17:07 +0800, Han Xin wrote:\n> We found an issue that could create an endless loop where alternates\n> objects are used improperly.\n>\n> While do fetching in a partial cloned repository with a commit graph,\n> deref_without_lazy_fetch_extended() will call lookup_commit_in_graph()\n> to find the commit object. We can found the code in commit-graph.c:\n>\n> struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)\n> {\n> …\n> if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> return NULL;\n> if (!repo_has_object_file(repo, id))\n> return NULL;\n\n> If we found the object in the commit graph, but missing it in the repository,\n> we will go into an endless loop:\n> git fetch -> deref_without_lazy_fetch_extended() ->\n> lookup_commit_in_graph() -> repo_has_object_file() ->\n> promisor_remote_get_direct() -> fetch_objects() ->\n> git fetch\n>\n> I know that the reason for this issue is due to improper use of\n> alternates, we can ensure that objects will not be lost by maintaining\n> all the references. But shouldn't we do something about this unusual\n> usage, it will cause a fetch bombardment of the remote git service.\n>\n> We can reproduce this issue with the following test case, it will\n> generate a lot of git processes, please be careful to stop it.\n> ———————————————————————————\n> #!/bin/sh\n>\n> test_description='test for an endless loop fetching’\n>\n> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n> . ./test-lib.sh\n>\n> test_expect_success 'setup’ ‘\n> git init --bare dest.git &&\n> test_commit one &&\n> git checkout -b testbranch &&\n> test_commit two &&\n> git push dest.git --all\n> '\n>\n> test_expect_success 'prepare a alternates repository without testbranch' '\n> git clone -b $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME dest.git alternates &&\n> oid=$(git -C alternates rev-parse refs/remotes/origin/testbranch) &&\n> git -C alternates update-ref -d refs/remotes/origin/testbranch &&\n> git -C alternates gc --prune=now\n> '\n>\n> test_expect_success 'prepare a repository with commit-graph' '\n> git init source &&\n> echo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> git -C source remote add origin \"$(pwd)/dest.git\" &&\n> git -C source config remote.origin.promisor true &&\n> git -C source config remote.origin.partialclonefilter blob:none &&\n> git -C source fetch origin &&\n> (\n> cd source &&\n> test_commit three &&\n> git -c gc.writeCommitGraph=true gc\n> )\n> '\n>\n> test_expect_success 'change alternates' '\n> echo \"$(pwd)/alternates/.git/objects\" >source/.git/objects/info/alternates &&\n> # this will bring an endless loop fetching\n> git -C source fetch origin $oid\n> '\n>\n> test_done\n>\n> ------------------------------------------------------\n>\n> Thanks\n> -Han Xin\n\nI think it's caused by using lazy-fetch in deref_without_lazy_fetch_extended().\nIn lookup_commit_in_graph(), lazy-fetch is initiated by\nrepo_has_object_file() used.\nhas_object() should be used, it's no-lazy-fetch.\n"},{"id":"457241","messageId":"YqlBjET0tf7V9/sg@nand.local","threadId":"57995","inReplyTo":"CANe9W27GVn-w1WSZNTxh5SKEMzHGEZQCF48vmbvMi4AUEg12yQ@mail.gmail.com","subject":"Re: An endless loop fetching issue with partial clone, alternates and commit graph","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T02:18:52Z","receivedAt":"2022-06-15T02:18:58Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[+cc Stolee]\n\nOn Tue, Jun 14, 2022 at 03:25:13PM +0800, Haiyng Tan wrote:\n> I think it's caused by using lazy-fetch in\n> deref_without_lazy_fetch_extended(). In lookup_commit_in_graph(),\n> lazy-fetch is initiated by repo_has_object_file() used. has_object()\n> should be used, it's no-lazy-fetch.\n\nHmm. Are there cases where lookup_commit_in_graph() is expected to\nlazily fetch missing objects from promisor remotes? If so, then this\nwouldn't quite work. If not, then this seems like an appropriate fix to\nme.\n\nThanks,\nTaylor\n"},{"id":"457340","messageId":"cover.1655350442.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"YqlBjET0tf7V9/sg@nand.local","subject":"[RFC PATCH 0/2] Re: An endless loop fetching issue with partial clone, alternates and commit graph","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-16T03:38:31Z","receivedAt":"2022-06-16T03:39:21Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Wed, Jun 15, 2022 at 10:18 AM Taylor Blau wrote:\n>\n> [+cc Stolee]\n>\n> On Tue, Jun 14, 2022 at 03:25:13PM +0800, Haiyng Tan wrote:\n> > I think it's caused by using lazy-fetch in\n> > deref_without_lazy_fetch_extended(). In lookup_commit_in_graph(),\n> > lazy-fetch is initiated by repo_has_object_file() used. has_object()\n> > should be used, it's no-lazy-fetch.\n>\n> Hmm. Are there cases where lookup_commit_in_graph() is expected to\n> lazily fetch missing objects from promisor remotes? If so, then this\n> wouldn't quite work. If not, then this seems like an appropriate fix to\n> me.\n>\n> Thanks,\n> Taylor\n\nWe can see the use of has_object() in RelNotes/2.29.0.txt[1]：\n * A new helper function has_object() has been introduced to make it\n easier to mark object existence checks that do and don't want to\n trigger lazy fetches, and a few such checks are converted using it.\n\nLet's see the difference between has_object() and repo_has_object_file():\n int has_object(struct repository *r, const struct object_id *oid,\n unsigned flags)\n {\n int quick = !(flags & HAS_OBJECT_RECHECK_PACKED);\n unsigned object_info_flags = OBJECT_INFO_SKIP_FETCH_OBJECT |\n (quick ? OBJECT_INFO_QUICK : 0);\n\n if (!startup_info->have_repository)\n return 0;\n return oid_object_info_extended(r, oid, NULL, object_info_flags) >= 0;\n }\n\n int repo_has_object_file_with_flags(struct repository *r,\n const struct object_id *oid, int flags)\n {\n if (!startup_info->have_repository)\n return 0;\n return oid_object_info_extended(r, oid, NULL, flags) >= 0;\n }\n\n int repo_has_object_file(struct repository *r,\n const struct object_id *oid)\n {\n return repo_has_object_file_with_flags(r, oid, 0);\n }\n\nNow we kown that has_object() add OBJECT_INFO_SKIP_FETCH_OBJECT to skip\nfetch object.\n\nI found that Ævar Arnfjörð Bjarmason added deref_without_lazy_fetch()\n4 weeks ago[2]:\n static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n int mark_tags_complete)\n {\n enum object_type type;\n unsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;\n return deref_without_lazy_fetch_extended(oid, mark_tags_complete,\n &type, flags);\n }\n\nBut oi_flags is only used by oid_object_info_extended() and is missed by\nlookup_commit_in_graph():\n static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,\n int mark_tags_complete,\n enum object_type *type,\n unsigned int oi_flags)\n {\n struct object_info info = { .typep = type };\n struct commit *commit;\n\n commit = lookup_commit_in_graph(the_repository, oid);\n if (commit)\n return commit;\n\n while (1) {\n if (oid_object_info_extended(the_repository, oid, &info,\n oi_flags))\n\nSo, an appropriate fix can be that let lookup_commit_in_graph() pickup\noi_flags and pass it to oid_object_info_extended(), then the fetching\nloop will be prevent by the given flag OBJECT_INFO_SKIP_FETCH_OBJECT.\n\n1. https://github.com/git/git/blob/master/Documentation/RelNotes/2.29.0.txt\n2. https://lore.kernel.org/git/2a563b5f18cc9c42cb71a9547344a5435f6bc058.1652731865.git.gitgitgadget@gmail.com/\n\nThanks\n-Han Xin\n\nHan Xin (2):\n commit-graph.c: add \"flags\" to lookup_commit_in_graph()\n fetch-pack.c: pass \"oi_flags\" to lookup_commit_in_graph()\n\n builtin/fetch.c | 4 ++-\n commit-graph.c | 5 ++--\n commit-graph.h | 3 +-\n fetch-pack.c | 10 +++----\n revision.c | 2 +-\n t/t5583-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++++++++++\n upload-pack.c | 5 ++--\n 7 files changed, 64 insertions(+), 12 deletions(-)\n create mode 100644 t/t5583-fetch-with-commit-graph.sh\n\n-- \n2.36.1\n\n"},{"id":"457341","messageId":"9aa52b29862d9a6432d0752eae12365f43ba52c0.1655350442.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1655350442.git.hanxin.hx@bytedance.com","subject":"[RFC PATCH 1/2] commit-graph.c: add \"flags\" to lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-16T03:38:32Z","receivedAt":"2022-06-16T03:39:24Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"When try to do deref_without_lazy_fetch_extended(), \"oi_flags\" will\nbe missed by lookup_commit_in_graph(), then repo_has_object_file()\nmay start a new round objects fetching.\nSo let's add \"flags\" to lookup_commit_in_graph() and use\nrepo_has_object_file_with_flags() to pass the flags.\n\nSigned-off-by: Han Xin \n---\n builtin/fetch.c | 4 +++-\n commit-graph.c | 5 +++--\n commit-graph.h | 3 ++-\n fetch-pack.c | 4 ++--\n revision.c | 2 +-\n upload-pack.c | 5 +++--\n 6 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae..44285d5318 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1179,7 +1179,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\t * annotated tags.\n \t\t\t\t */\n \t\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n-\t\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\t\t\tcommit = lookup_commit_in_graph(\n+\t\t\t\t\t\tthe_repository, &rm->old_oid,\n+\t\t\t\t\t\t0);\n \t\t\t\tif (!commit) {\n \t\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n \t\t\t\t\t\t\t\t\t\t&rm->old_oid,\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d4503336..b09f454bb5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -889,7 +889,8 @@ static int find_commit_pos_in_graph(struct commit *item, struct commit_graph *g,\n \t}\n }\n \n-struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)\n+struct commit *lookup_commit_in_graph(struct repository *repo,\n+\t\t\t\t const struct object_id *id, int flags)\n {\n \tstruct commit *commit;\n \tuint32_t pos;\n@@ -898,7 +899,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!repo_has_object_file_with_flags(repo, id, flags))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2e3ac35237..747a67c0ee 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -46,7 +46,8 @@ int parse_commit_in_graph(struct repository *r, struct commit *item);\n * that we don't return commits whose object has been pruned. Otherwise, this\n * function returns `NULL`.\n */\n-struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id);\n+struct commit *lookup_commit_in_graph(struct repository *repo,\n+\t\t\t\t const struct object_id *id, int flags);\n \n /*\n * It is possible that we loaded commit contents from the commit buffer,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex cb6647d657..4a62fb182e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -123,7 +123,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \tstruct object_info info = { .typep = type };\n \tstruct commit *commit;\n \n-\tcommit = lookup_commit_in_graph(the_repository, oid);\n+\tcommit = lookup_commit_in_graph(the_repository, oid, 0);\n \tif (commit)\n \t\treturn commit;\n \n@@ -714,7 +714,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct commit *commit;\n \n-\t\tcommit = lookup_commit_in_graph(the_repository, &ref->old_oid);\n+\t\tcommit = lookup_commit_in_graph(the_repository, &ref->old_oid, 0);\n \t\tif (!commit) {\n \t\t\tstruct object *o;\n \ndiff --git a/revision.c b/revision.c\nindex 211352795c..df5db51f98 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -379,7 +379,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t * look up the object ID in those graphs. Like this, we can avoid\n \t * parsing commit data from disk.\n \t */\n-\tcommit = lookup_commit_in_graph(revs->repo, oid);\n+\tcommit = lookup_commit_in_graph(revs->repo, oid, 0);\n \tif (commit)\n \t\tobject = &commit->object;\n \telse\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3a851b3606..0fa9c3cf3f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1407,7 +1407,7 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get oid, not '%s'\", line);\n \n-\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n+\t\tcommit = lookup_commit_in_graph(the_repository, &oid, 0);\n \t\tif (commit)\n \t\t\to = &commit->object;\n \t\telse\n@@ -1455,7 +1455,8 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \t\titem->util = oiddup(&oid);\n \n \t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n-\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n+\t\t\tstruct commit *commit =\n+\t\t\t\tlookup_commit_in_graph(the_repository, &oid, 0);\n \t\t\tif (commit)\n \t\t\t\to = &commit->object;\n \t\t}\n-- \n2.36.1\n\n"},{"id":"457342","messageId":"03ec01ab398bc4967f7ac8ccab510cac2d6785be.1655350442.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1655350442.git.hanxin.hx@bytedance.com","subject":"[RFC PATCH 2/2] fetch-pack.c: pass \"oi_flags\" to lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-16T03:38:33Z","receivedAt":"2022-06-16T03:39:35Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"As the custom \"oi_flags\" is missed by lookup_commit_in_graph(), we will\nget another lazy fetch round if we found the commit in commit graph but\nmiss it in the local object repository.\n\nWe can see the issue via[1].\n\n1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n\nSigned-off-by: Han Xin \n---\n fetch-pack.c | 10 +++----\n t/t5583-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 5 deletions(-)\n create mode 100644 t/t5583-fetch-with-commit-graph.sh\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 4a62fb182e..ca1234e456 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -123,7 +123,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *\n \tstruct object_info info = { .typep = type };\n \tstruct commit *commit;\n \n-\tcommit = lookup_commit_in_graph(the_repository, oid, 0);\n+\tcommit = lookup_commit_in_graph(the_repository, oid, oi_flags);\n \tif (commit)\n \t\treturn commit;\n \n@@ -704,6 +704,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tstruct ref *ref;\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n+\tint oi_flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;\n \n \tif (args->refetch)\n \t\treturn;\n@@ -714,13 +715,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct commit *commit;\n \n-\t\tcommit = lookup_commit_in_graph(the_repository, &ref->old_oid, 0);\n+\t\tcommit = lookup_commit_in_graph(the_repository, &ref->old_oid,\n+\t\t\t\t\t\toi_flags);\n \t\tif (!commit) {\n \t\t\tstruct object *o;\n \n-\t\t\tif (!has_object_file_with_flags(&ref->old_oid,\n-\t\t\t\t\t\tOBJECT_INFO_QUICK |\n-\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n+\t\t\tif (!has_object_file_with_flags(&ref->old_oid, oi_flags))\n \t\t\t\tcontinue;\n \t\t\to = parse_object(the_repository, &ref->old_oid);\n \t\t\tif (!o || o->type != OBJ_COMMIT)\ndiff --git a/t/t5583-fetch-with-commit-graph.sh b/t/t5583-fetch-with-commit-graph.sh\nnew file mode 100644\nindex 0000000000..cb2beafa8d\n--- /dev/null\n+++ b/t/t5583-fetch-with-commit-graph.sh\n@@ -0,0 +1,47 @@\n+#!/bin/sh\n+\n+test_description='test for fetching missing object with a full commit-graph'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init --bare dest.git &&\n+\ttest_commit one &&\n+\tgit checkout -b testbranch &&\n+\ttest_commit two &&\n+\tgit push dest.git --all\n+'\n+\n+test_expect_success 'prepare a alternates repository without testbranch' '\n+\tgit clone -b $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME dest.git alternates &&\n+\toid=$(git -C alternates rev-parse refs/remotes/origin/testbranch) &&\n+\tgit -C alternates update-ref -d refs/remotes/origin/testbranch &&\n+\tgit -C alternates gc --prune=now\n+'\n+\n+test_expect_success 'prepare a repository with a full commit-graph' '\n+\tgit init source &&\n+\techo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n+\tgit -C source remote add origin \"$(pwd)/dest.git\" &&\n+\tgit -C source config remote.origin.promisor true &&\n+\tgit -C source config remote.origin.partialclonefilter blob:none &&\n+\tgit -C source fetch origin &&\n+\t(\n+\t\tcd source &&\n+\t\ttest_commit three &&\n+\t\tgit -c gc.writeCommitGraph=true gc\n+\t)\n+'\n+\n+test_expect_success 'change the alternates to that without commit two' '\n+\techo \"$(pwd)/alternates/.git/objects\" >source/.git/objects/info/alternates\n+'\n+\n+test_expect_success 'fetch the missing object' '\n+\tgit -C source fetch origin $oid\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"457465","messageId":"20220617214757.2713875-1-jonathantanmy@google.com","threadId":"57995","inReplyTo":"cover.1655350442.git.hanxin.hx@bytedance.com","subject":"Re: [RFC PATCH 0/2] Re: An endless loop fetching issue with partial clone, alternates and commit graph","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-17T21:47:57Z","receivedAt":"2022-06-17T21:48:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Xin writes:\n> On Wed, Jun 15, 2022 at 10:18 AM Taylor Blau wrote:\n> >\n> > [+cc Stolee]\n> >\n> > On Tue, Jun 14, 2022 at 03:25:13PM +0800, Haiyng Tan wrote:\n> > > I think it's caused by using lazy-fetch in\n> > > deref_without_lazy_fetch_extended(). In lookup_commit_in_graph(),\n> > > lazy-fetch is initiated by repo_has_object_file() used. has_object()\n> > > should be used, it's no-lazy-fetch.\n> >\n> > Hmm. Are there cases where lookup_commit_in_graph() is expected to\n> > lazily fetch missing objects from promisor remotes? If so, then this\n> > wouldn't quite work. If not, then this seems like an appropriate fix to\n> > me.\n> >\n> > Thanks,\n> > Taylor\n\nI think that if a commit is in the commit graph, we would expect the\ncommit to also be present. So changing to has_object() makes sense.\n\n> We can see the use of has_object() in RelNotes/2.29.0.txt[1]：\n> * A new helper function has_object() has been introduced to make it\n> easier to mark object existence checks that do and don't want to\n> trigger lazy fetches, and a few such checks are converted using it.\n\nAlso relevant is the comment on repo_has_object_file() in\nobject-store.h.\n\n> So, an appropriate fix can be that let lookup_commit_in_graph() pickup\n> oi_flags and pass it to oid_object_info_extended(), then the fetching\n> loop will be prevent by the given flag OBJECT_INFO_SKIP_FETCH_OBJECT.\n\nHmm...why not change it to has_object_file() instead, as Haiyng Tan\nmentioned?\n"},{"id":"457489","messageId":"20220618030130.36419-1-hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1655350442.git.hanxin.hx@bytedance.com","subject":"[PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-18T03:01:30Z","receivedAt":"2022-06-18T03:02:01Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"If a commit is in the commit graph, we would expect the commit to also\nbe present. So we should use has_object() instead of\nrepo_has_object_file(), which will help us avoid getting into an endless\nloop of lazy fetch.\n\nWe can see the endless loop issue via this[1].\n\n1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n\nSigned-off-by: Han Xin \n---\n commit-graph.c | 2 +-\n t/t5329-no-lazy-fetch-with-commit-graph.sh | 50 ++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 1 deletion(-)\n create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2b52818731..2dd9bcc7ea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!has_object(repo, id, 0))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\nnew file mode 100755\nindex 0000000000..ea5940b9f1\n--- /dev/null\n+++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n@@ -0,0 +1,50 @@\n+#!/bin/sh\n+\n+test_description='test for no lazy fetch with the commit-graph'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init --bare dest.git &&\n+\ttest_commit one &&\n+\tgit checkout -b tmp &&\n+\ttest_commit two &&\n+\tgit push dest.git --all\n+'\n+\n+test_expect_success 'prepare a alternates repository without commit two' '\n+\tgit clone --bare dest.git alternates &&\n+\toid=$(git -C alternates rev-parse refs/heads/tmp) &&\n+\tgit -C alternates update-ref -d refs/heads/tmp &&\n+\tgit -C alternates gc --prune=now &&\n+\tpack=$(echo alternates/objects/pack/*.pack) &&\n+\tgit verify-pack -v \"$pack\" >have &&\n+\t! grep \"$oid\" have\n+'\n+\n+test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n+\tgit init source &&\n+\techo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n+\tgit -C source remote add origin \"$(pwd)/dest.git\" &&\n+\tgit -C source config remote.origin.promisor true &&\n+\tgit -C source config remote.origin.partialclonefilter blob:none &&\n+\t# the source repository has the whole refs contains refs/heads/tmp\n+\tgit -C source fetch origin &&\n+\t(\n+\t\tcd source &&\n+\t\ttest_commit three &&\n+\t\tgit -c gc.writeCommitGraph=true gc\n+\t)\n+'\n+\n+test_expect_success 'change the alternates of source to that without commit two' '\n+\t# now we have a commit-graph in the source repository but without the commit two\n+\techo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n+'\n+\n+test_expect_success 'fetch the missing commit' '\n+\tgit -C source fetch origin $oid 2>fetch.out &&\n+\tgrep \"$oid\" fetch.out\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"457541","messageId":"YrAcrNApaZDngLL+@ncase","threadId":"57995","inReplyTo":"20220618030130.36419-1-hanxin.hx@bytedance.com","subject":"Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-06-20T07:07:24Z","receivedAt":"2022-06-20T07:34:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote:\n> If a commit is in the commit graph, we would expect the commit to also\n> be present. So we should use has_object() instead of\n> repo_has_object_file(), which will help us avoid getting into an endless\n> loop of lazy fetch.\n> \n> We can see the endless loop issue via this[1].\n> \n> 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n> \n> Signed-off-by: Han Xin \n\nThanks a lot for working on this issue!\n\n> ---\n> commit-graph.c | 2 +-\n> t/t5329-no-lazy-fetch-with-commit-graph.sh | 50 ++++++++++++++++++++++\n> 2 files changed, 51 insertions(+), 1 deletion(-)\n> create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 2b52818731..2dd9bcc7ea 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n> \t\treturn NULL;\n> \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> \t\treturn NULL;\n> -\tif (!repo_has_object_file(repo, id))\n> +\tif (!has_object(repo, id, 0))\n> \t\treturn NULL;\n\nAgreed, this change makes sense to me.\n\n> \tcommit = lookup_commit(repo, id);\n> diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> new file mode 100755\n> index 0000000000..ea5940b9f1\n> --- /dev/null\n> +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> @@ -0,0 +1,50 @@\n> +#!/bin/sh\n> +\n> +test_description='test for no lazy fetch with the commit-graph'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n\nNit: I find it a bit confusing that you only call the first test case\n`setup`, while all the other tests except for the last one are also only\nsetting up the necessary state.\n\n> +\tgit init --bare dest.git &&\n> +\ttest_commit one &&\n> +\tgit checkout -b tmp &&\n> +\ttest_commit two &&\n> +\tgit push dest.git --all\n> +'\n> +\n> +test_expect_success 'prepare a alternates repository without commit two' '\n> \n> +\tgit clone --bare dest.git alternates &&\n> \n> +\toid=$(git -C alternates rev-parse refs/heads/tmp) &&\n> +\tgit -C alternates update-ref -d refs/heads/tmp &&\n> +\tgit -C alternates gc --prune=now &&\n> +\tpack=$(echo alternates/objects/pack/*.pack) &&\n> +\tgit verify-pack -v \"$pack\" >have &&\n> +\t! grep \"$oid\" have\n> +'\n\nInstead of going into the low-level details you could just verify that\n`git cat-file -e $oid` returns `1`.\n\n> +\n> +test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n> +\tgit init source &&\n> +\techo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> +\tgit -C source remote add origin \"$(pwd)/dest.git\" &&\n> +\tgit -C source config remote.origin.promisor true &&\n> +\tgit -C source config remote.origin.partialclonefilter blob:none &&\n> +\t# the source repository has the whole refs contains refs/heads/tmp\n> +\tgit -C source fetch origin &&\n> +\t(\n> +\t\tcd source &&\n> +\t\ttest_commit three &&\n> +\t\tgit -c gc.writeCommitGraph=true gc\n> +\t)\n> +'\n> +\n> +test_expect_success 'change the alternates of source to that without commit two' '\n> +\t# now we have a commit-graph in the source repository but without the commit two\n> +\techo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n> +'\n> +\n> +test_expect_success 'fetch the missing commit' '\n> +\tgit -C source fetch origin $oid 2>fetch.out &&\n> +\tgrep \"$oid\" fetch.out\n> +'\n\nThis test passes even without your fix, albeit a lot slower compared\nto with it. Can we somehow cause it to fail reliably so that the test\nbecomes effective in catching a regression here?\n\n> +test_done\n> -- \n> 2.36.1\n> \n\nPatrick\n"},{"id":"457544","messageId":"CAKgqsWVfjOw-b4hbz1WDH5sevUab_bQVLb703apew3fX7B60rQ@mail.gmail.com","threadId":"57995","inReplyTo":"YrAcrNApaZDngLL+@ncase","subject":"Re: [External] Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"欣韩","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-20T08:53:47Z","receivedAt":"2022-06-20T08:54:09Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Mon, Jun 20, 2022 at 3:34 PM Patrick Steinhardt wrote:\n>\n> On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote:\n> > If a commit is in the commit graph, we would expect the commit to also\n> > be present. So we should use has_object() instead of\n> > repo_has_object_file(), which will help us avoid getting into an endless\n> > loop of lazy fetch.\n> >\n> > We can see the endless loop issue via this[1].\n> >\n> > 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n> >\n> > Signed-off-by: Han Xin \n>\n> Thanks a lot for working on this issue!\n>\n> > ---\n> > commit-graph.c | 2 +-\n> > t/t5329-no-lazy-fetch-with-commit-graph.sh | 50 ++++++++++++++++++++++\n> > 2 files changed, 51 insertions(+), 1 deletion(-)\n> > create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n> >\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 2b52818731..2dd9bcc7ea 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n> > return NULL;\n> > if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> > return NULL;\n> > - if (!repo_has_object_file(repo, id))\n> > + if (!has_object(repo, id, 0))\n> > return NULL;\n>\n> Agreed, this change makes sense to me.\n>\n> > commit = lookup_commit(repo, id);\n> > diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> > new file mode 100755\n> > index 0000000000..ea5940b9f1\n> > --- /dev/null\n> > +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> > @@ -0,0 +1,50 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='test for no lazy fetch with the commit-graph'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n>\n> Nit: I find it a bit confusing that you only call the first test case\n> `setup`, while all the other tests except for the last one are also only\n> setting up the necessary state.\n\nNod.\nThere is indeed some misleading information here.\nI will adjust it in the next patch.\n\n>\n> > + git init --bare dest.git &&\n> > + test_commit one &&\n> > + git checkout -b tmp &&\n> > + test_commit two &&\n> > + git push dest.git --all\n> > +'\n> > +\n> > +test_expect_success 'prepare a alternates repository without commit two' '\n> >\n> > + git clone --bare dest.git alternates &&\n> >\n> > + oid=$(git -C alternates rev-parse refs/heads/tmp) &&\n> > + git -C alternates update-ref -d refs/heads/tmp &&\n> > + git -C alternates gc --prune=now &&\n> > + pack=$(echo alternates/objects/pack/*.pack) &&\n> > + git verify-pack -v \"$pack\" >have &&\n> > + ! grep \"$oid\" have\n> > +'\n>\n> Instead of going into the low-level details you could just verify that\n> `git cat-file -e $oid` returns `1`.\n>\n\nNod.\n\n> > +\n> > +test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n> > + git init source &&\n> > + echo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> > + git -C source remote add origin \"$(pwd)/dest.git\" &&\n> > + git -C source config remote.origin.promisor true &&\n> > + git -C source config remote.origin.partialclonefilter blob:none &&\n> > + # the source repository has the whole refs contains refs/heads/tmp\n> > + git -C source fetch origin &&\n> > + (\n> > + cd source &&\n> > + test_commit three &&\n> > + git -c gc.writeCommitGraph=true gc\n> > + )\n> > +'\n> > +\n> > +test_expect_success 'change the alternates of source to that without commit two' '\n> > + # now we have a commit-graph in the source repository but without the commit two\n> > + echo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n> > +'\n> > +\n> > +test_expect_success 'fetch the missing commit' '\n> > + git -C source fetch origin $oid 2>fetch.out &&\n> > + grep \"$oid\" fetch.out\n> > +'\n>\n> This test passes even without your fix, albeit a lot slower compared\n> to with it. Can we somehow cause it to fail reliably so that the test\n> becomes effective in catching a regression here?\n>\n\nCould you help me find the reason why this testcase passes even\nwithout the fix.\n\nFrom the execution of Github Action, it seems that the problem always exist：\nhttps://github.com/chiyutianyi/git/actions/runs/2527421443.\n\nThanks.\n-Han Xin\n\n> > +test_done\n> > --\n> > 2.36.1\n> >\n>\n> Patrick\n"},{"id":"457545","messageId":"YrA4WdvmN4jrXe/m@ncase","threadId":"57995","inReplyTo":"CAKgqsWVfjOw-b4hbz1WDH5sevUab_bQVLb703apew3fX7B60rQ@mail.gmail.com","subject":"Re: [External] Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-06-20T09:05:29Z","receivedAt":"2022-06-20T09:05:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 20, 2022 at 04:53:47PM +0800, 欣韩 wrote:\n> On Mon, Jun 20, 2022 at 3:34 PM Patrick Steinhardt wrote:\n> >\n> > On Sat, Jun 18, 2022 at 11:01:30AM +0800, Han Xin wrote:\n[snip]\n> > > +test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n> > > + git init source &&\n> > > + echo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> > > + git -C source remote add origin \"$(pwd)/dest.git\" &&\n> > > + git -C source config remote.origin.promisor true &&\n> > > + git -C source config remote.origin.partialclonefilter blob:none &&\n> > > + # the source repository has the whole refs contains refs/heads/tmp\n> > > + git -C source fetch origin &&\n> > > + (\n> > > + cd source &&\n> > > + test_commit three &&\n> > > + git -c gc.writeCommitGraph=true gc\n> > > + )\n> > > +'\n> > > +\n> > > +test_expect_success 'change the alternates of source to that without commit two' '\n> > > + # now we have a commit-graph in the source repository but without the commit two\n> > > + echo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n> > > +'\n> > > +\n> > > +test_expect_success 'fetch the missing commit' '\n> > > + git -C source fetch origin $oid 2>fetch.out &&\n> > > + grep \"$oid\" fetch.out\n> > > +'\n> >\n> > This test passes even without your fix, albeit a lot slower compared\n> > to with it. Can we somehow cause it to fail reliably so that the test\n> > becomes effective in catching a regression here?\n> >\n> \n> Could you help me find the reason why this testcase passes even\n> without the fix.\n> \n> From the execution of Github Action, it seems that the problem always exist：\n> https://github.com/chiyutianyi/git/actions/runs/2527421443.\n> \n> Thanks.\n> -Han Xin\n\nHard to say, I'm not sure either. One thing I noticed though is that in\nyour CI run there's failure in e.g. linux-gcc, but the test run for\nlinux-musl succeeds. Personally I'm using musl libc on my system, as\nwell, so maybe it's a discrepancy between musl- and glibc-based systems?\n\nPatrick\n"},{"id":"457634","messageId":"20220621182322.3444926-1-jonathantanmy@google.com","threadId":"57995","inReplyTo":"20220618030130.36419-1-hanxin.hx@bytedance.com","subject":"Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-21T18:23:21Z","receivedAt":"2022-06-21T18:23:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Xin writes:\n> If a commit is in the commit graph, we would expect the commit to also\n> be present. So we should use has_object() instead of\n> repo_has_object_file(), which will help us avoid getting into an endless\n> loop of lazy fetch.\n> \n> We can see the endless loop issue via this[1].\n> \n> 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n\nAs described in SubmittingPatches:\n\n Try to make sure your explanation can be understood \n without external resources. Instead of giving a URL to a mailing list\n archive, summarize the relevant points of the discussion.\n\n> +test_expect_success 'setup' '\n> +\tgit init --bare dest.git &&\n> +\ttest_commit one &&\n> +\tgit checkout -b tmp &&\n> +\ttest_commit two &&\n> +\tgit push dest.git --all\n> +'\n\nYou can commit directly to the repo by using \"test_commit -C dest.git\".\nAlso, can the repositories be better named? I see a \"dest.git\" (which\nseems to contain all the objects), \"alternates\" (which seems to contain\neverything except refs/heads/tmp), \"source\" (which only contains the\ncommit graph), and the current directory. It would probably be better to\nname them e.g. \"with-commit\", \"without-commit\", \"only-commit-graph\", and\nomit using the current directory altogether.\n\n> +test_expect_success 'prepare a alternates repository without commit two' '\n> +\tgit clone --bare dest.git alternates &&\n> +\toid=$(git -C alternates rev-parse refs/heads/tmp) &&\n> +\tgit -C alternates update-ref -d refs/heads/tmp &&\n> +\tgit -C alternates gc --prune=now &&\n> +\tpack=$(echo alternates/objects/pack/*.pack) &&\n> +\tgit verify-pack -v \"$pack\" >have &&\n> +\t! grep \"$oid\" have\n> +'\n\nOK, except refs/heads/tmp could probably have a better name.\n\n> +test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n> +\tgit init source &&\n> +\techo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> +\tgit -C source remote add origin \"$(pwd)/dest.git\" &&\n> +\tgit -C source config remote.origin.promisor true &&\n> +\tgit -C source config remote.origin.partialclonefilter blob:none &&\n> +\t# the source repository has the whole refs contains refs/heads/tmp\n> +\tgit -C source fetch origin &&\n> +\t(\n> +\t\tcd source &&\n> +\t\ttest_commit three &&\n> +\t\tgit -c gc.writeCommitGraph=true gc\n> +\t)\n> +'\n\nIs the purpose of the fetch only to add a ref? If yes, it's clearer just\nto create that branch instead of fetching.\n\n> +test_expect_success 'change the alternates of source to that without commit two' '\n> +\t# now we have a commit-graph in the source repository but without the commit two\n> +\techo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n> +'\n\nOK.\n\n> +test_expect_success 'fetch the missing commit' '\n> +\tgit -C source fetch origin $oid 2>fetch.out &&\n> +\tgrep \"$oid\" fetch.out\n> +'\n\nIs the bug triggered by fetching the missing commit or by fetching any\ncommit (which triggers the usage of the commit graph)? If any commit,\nthen it's clearer to create an arbitrary commit and then fetch it.\n\nAlso, I thought that the issue was an infinite loop, and the thing being\ntested here looks different from that. If you want to ensure that\nnothing is being fetched, you can use GIT_TRACE=\"$(pwd)/trace\" to\nobserve a fetch-pack command being invoked or\nGIT_TRACE_PACKET=\"$(pwd)/trace\" to observe the packet being sent. If\nyou're worried about an infinite loop, you can set origin to a directory\nthat does not exist (so that the fetch immediately fails).\n"},{"id":"457672","messageId":"CAKgqsWUdRpm8Aa6+oLqHGGXMzXB76f7mM9vf89mG8XVSsQ-1aw@mail.gmail.com","threadId":"57995","inReplyTo":"20220621182322.3444926-1-jonathantanmy@google.com","subject":"Re: Re: [PATCH v1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-22T03:17:23Z","receivedAt":"2022-06-22T03:17:40Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Wed, Jun 22, 2022 at 2:23 AM Jonathan Tan wrote:\n>\n> Han Xin writes:\n> > If a commit is in the commit graph, we would expect the commit to also\n> > be present. So we should use has_object() instead of\n> > repo_has_object_file(), which will help us avoid getting into an endless\n> > loop of lazy fetch.\n> >\n> > We can see the endless loop issue via this[1].\n> >\n> > 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/\n>\n> As described in SubmittingPatches:\n>\n> Try to make sure your explanation can be understood\n> without external resources. Instead of giving a URL to a mailing list\n> archive, summarize the relevant points of the discussion.\n>\n\nNod.\n\n> > +test_expect_success 'setup' '\n> > + git init --bare dest.git &&\n> > + test_commit one &&\n> > + git checkout -b tmp &&\n> > + test_commit two &&\n> > + git push dest.git --all\n> > +'\n>\n> You can commit directly to the repo by using \"test_commit -C dest.git\".\n> Also, can the repositories be better named? I see a \"dest.git\" (which\n> seems to contain all the objects), \"alternates\" (which seems to contain\n> everything except refs/heads/tmp), \"source\" (which only contains the\n> commit graph), and the current directory. It would probably be better to\n> name them e.g. \"with-commit\", \"without-commit\", \"only-commit-graph\", and\n> omit using the current directory altogether.\n\nYes, it makes sense to me.\n\n>\n> > +test_expect_success 'prepare a alternates repository without commit two' '\n> > + git clone --bare dest.git alternates &&\n> > + oid=$(git -C alternates rev-parse refs/heads/tmp) &&\n> > + git -C alternates update-ref -d refs/heads/tmp &&\n> > + git -C alternates gc --prune=now &&\n> > + pack=$(echo alternates/objects/pack/*.pack) &&\n> > + git verify-pack -v \"$pack\" >have &&\n> > + ! grep \"$oid\" have\n> > +'\n>\n> OK, except refs/heads/tmp could probably have a better name.\n\nI'll rethink the naming here.\n\n>\n> > +test_expect_success 'prepare a repository with a commit-graph contains commit two' '\n> > + git init source &&\n> > + echo \"$(pwd)/dest.git/objects\" >source/.git/objects/info/alternates &&\n> > + git -C source remote add origin \"$(pwd)/dest.git\" &&\n> > + git -C source config remote.origin.promisor true &&\n> > + git -C source config remote.origin.partialclonefilter blob:none &&\n> > + # the source repository has the whole refs contains refs/heads/tmp\n> > + git -C source fetch origin &&\n> > + (\n> > + cd source &&\n> > + test_commit three &&\n> > + git -c gc.writeCommitGraph=true gc\n> > + )\n> > +'\n>\n> Is the purpose of the fetch only to add a ref? If yes, it's clearer just\n> to create that branch instead of fetching.\n\nNod.\n\n>\n> > +test_expect_success 'change the alternates of source to that without commit two' '\n> > + # now we have a commit-graph in the source repository but without the commit two\n> > + echo \"$(pwd)/alternates/objects\" >source/.git/objects/info/alternates\n> > +'\n>\n> OK.\n>\n> > +test_expect_success 'fetch the missing commit' '\n> > + git -C source fetch origin $oid 2>fetch.out &&\n> > + grep \"$oid\" fetch.out\n> > +'\n>\n> Is the bug triggered by fetching the missing commit or by fetching any\n> commit (which triggers the usage of the commit graph)? If any commit,\n> then it's clearer to create an arbitrary commit and then fetch it.\n>\n\nYes, using the missing object in the commit-graph seems to be a little\nmisleading.\n\n> Also, I thought that the issue was an infinite loop, and the thing being\n> tested here looks different from that. If you want to ensure that\n> nothing is being fetched, you can use GIT_TRACE=\"$(pwd)/trace\" to\n> observe a fetch-pack command being invoked or\n> GIT_TRACE_PACKET=\"$(pwd)/trace\" to observe the packet being sent. If\n> you're worried about an infinite loop, you can set origin to a directory\n> that does not exist (so that the fetch immediately fails).\n\nMaybe we can use \"ulimit\" ?\n\nThen the test case can be:\n\n test_expect_success 'fetch the missing commit once' '\n ulimit -u 512 &&\n GIT_TRACE=\"$(pwd)/trace\" git -C source fetch origin $oid 2>err &&\n ! grep \"error: cannot fork\" err &&\n test $(grep \"fetch origin\" trace | wc -l) -eq 1\n '\n\nWithout this fix, \"git fetch\" would finally succeed because we didn't\ncheck the return value of promise_remote_get_direct(). We can find\nthe err output like this:\n\n error: cannot fork() for -c: Resource temporarily unavailable\n fatal: promisor-remote: unable to fork off fetch subprocess\n\nAnd we can see a lot of trace logs as follows:\n\n trace: run_command: git -c fetch.negotiationAlgorithm=noop fetch\norigin --no-tags --no-write-fetch-head --recurse-submodules=no\n--filter=blob:none --stdin\n trace: built-in: git fetch origin --no-tags --no-write-fetch-head\n--recurse-submodules=no --filter=blob:none --stdin\n\nThanks.\n-Han Xin\n"},{"id":"457828","messageId":"cover.1656044659.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"20220618030130.36419-1-hanxin.hx@bytedance.com","subject":"[PATCH v2 0/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-24T05:27:55Z","receivedAt":"2022-06-24T05:28:49Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"This patch fixes the following issue:\nWhen we found the commit in the graph in lookup_commit_in_graph(), but\nthe commit is missing from the repository, we will try\npromisor_remote_get_direct() and then enter another loop.\n\nThen we will go into an endless loop:\n git fetch -> deref_without_lazy_fetch() ->\n lookup_commit_in_graph() -> repo_has_object_file() ->\n promisor_remote_get_direct() -> fetch_objects() ->\n git fetch (a new loop round)\n\nChanges since v1:\n\n* add run_with_limited_processses() to test-lib so that we can use it to\n limit forking subprocesses. As we didn't check the return value of\n promise_remote_get_direct(), \"git fetch\" would finally succeed due to:\n\n error: cannot fork() for -c: Resource temporarily unavailable\n fatal: promisor-remote: unable to fork off fetch subprocess\n\n* Rename test repositories, reference name and use GIT_TRACE to observe\n the fetch process.\n\nHan Xin (2):\n test-lib.sh: add limited processes to test-lib\n commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n\n commit-graph.c | 2 +-\n t/t5329-no-lazy-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++\n t/test-lib.sh | 9 +++++\n 3 files changed, 57 insertions(+), 1 deletion(-)\n create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n\nRange-diff against v1:\n1: ebc14bfd5e < -: ---------- commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n-: ---------- > 1: 442a4c351d test-lib.sh: add limited processes to test-lib\n-: ---------- > 2: d3a99a5c5a commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n-- \n2.36.1\n\n"},{"id":"457829","messageId":"442a4c351dea603e226bae89eddc2b3496d93262.1656044659.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656044659.git.hanxin.hx@bytedance.com","subject":"[PATCH v2 1/2] test-lib.sh: add limited processes to test-lib","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-24T05:27:56Z","receivedAt":"2022-06-24T05:28:57Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up\ncommit.\n\nWith run_with_limited_processses() we can limit forking subprocesses and\nfail reliably in some test cases.\n\nSigned-off-by: Han Xin \n---\n t/test-lib.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8ba5ca1534..f920e3b0ae 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n \trun_with_limited_open_files true\n '\n \n+run_with_limited_processses () {\n+\t(ulimit -u 512 && \"$@\")\n+}\n+\n+test_lazy_prereq ULIMIT_PROCESSES '\n+\ttest_have_prereq !HPPA,!MINGW,!CYGWIN &&\n+\trun_with_limited_processses true\n+'\n+\n build_option () {\n \tgit version --build-options |\n \tsed -ne \"s/^$1: //p\"\n-- \n2.36.1\n\n"},{"id":"457830","messageId":"d3a99a5c5ae538b626e04d7069dd2fc316605dfc.1656044659.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656044659.git.hanxin.hx@bytedance.com","subject":"[PATCH v2 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-24T05:27:57Z","receivedAt":"2022-06-24T05:29:02Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"If a commit is in the commit graph, we would expect the commit to also\nbe present. So we should use has_object() instead of\nrepo_has_object_file(), which will help us avoid getting into an endless\nloop of lazy fetch.\n\nWhen we found the commit in the graph in lookup_commit_in_graph(), but\nthe commit is missing from the repository, we will try\npromisor_remote_get_direct() and then enter another loop. While\nsometimes it will finally succeed because it cannot fork subprocess,\nit has exhausted the local process resources and can be harmful to the\nremote service.\n\nSigned-off-by: Han Xin \n---\n commit-graph.c | 2 +-\n t/t5329-no-lazy-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 1 deletion(-)\n create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2b52818731..2dd9bcc7ea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!has_object(repo, id, 0))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\nnew file mode 100755\nindex 0000000000..4d25d2c950\n--- /dev/null\n+++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n@@ -0,0 +1,47 @@\n+#!/bin/sh\n+\n+test_description='test for no lazy fetch with the commit-graph'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: prepare a repository with a commit' '\n+\tgit init with-commit &&\n+\ttest_commit -C with-commit the-commit &&\n+\toid=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n+\tgit init with-commit-graph &&\n+\techo \"$(pwd)/with-commit/.git/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\t# create a ref that points to the commit in alternates\n+\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n+\t# prepare some other objects to commit-graph\n+\ttest_commit -C with-commit-graph somthing &&\n+\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n+\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n+'\n+\n+test_expect_success 'setup: change the alternates to what without the commit' '\n+\tgit init --bare without-commit &&\n+\techo \"$(pwd)/without-commit/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n+'\n+\n+test_expect_success 'setup: prepare another commit to fetch' '\n+\ttest_commit -C with-commit another-commit &&\n+\tanycommit=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success ULIMIT_PROCESSES 'fetch any commit from promisor with the usage of the commit graph' '\n+\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n+\tgit -C with-commit-graph config remote.origin.promisor true &&\n+\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n+\tGIT_TRACE=\"$(pwd)/trace\" run_with_limited_processses \\\n+\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n+\ttest_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n+\ttest $(grep \"fetch origin\" trace | wc -l) -eq 1\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"457854","messageId":"xmqqfsjuvyjz.fsf@gitster.g","threadId":"57995","inReplyTo":"442a4c351dea603e226bae89eddc2b3496d93262.1656044659.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v2 1/2] test-lib.sh: add limited processes to test-lib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-24T16:03:28Z","receivedAt":"2022-06-24T16:03:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Xin writes:\n\n> We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up\n> commit.\n>\n> With run_with_limited_processses() we can limit forking subprocesses and\n> fail reliably in some test cases.\n>\n> Signed-off-by: Han Xin \n> ---\n> t/test-lib.sh | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 8ba5ca1534..f920e3b0ae 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n> \trun_with_limited_open_files true\n> '\n> \n> +run_with_limited_processses () {\n> +\t(ulimit -u 512 && \"$@\")\n\nThe \"-u\" presumably is a way to say that the current user can have\nonly 512 processes at once that is supported by bash and ksh? dash\nseems to use \"-p\" for this but \"-p\" of course means something\ncompletely different to other shells (and is read-only), which is a\nmess X-<.\n\nI suspect that it is OK to make it practically bash-only, but then ...\n\n> +}\n> +\n> +test_lazy_prereq ULIMIT_PROCESSES '\n> +\ttest_have_prereq !HPPA,!MINGW,!CYGWIN &&\n> +\trun_with_limited_processses true\n\n... as this lazy-prereq makes a trial run that would fail when the\nsystem does not allow \"ulimit -u 512\", do we need the platform\nspecific prereq check? I am wondering if the second line alone is\nsufficient.\n\nAlso, 512 is not a number I would exactly call \"limit forking\".\nDoes it have to be so high, I wonder. Of course it cannot be so low\nlike 3 or 8 or even 32, as per-user limitation counts your window\nmanager and shells running in other windows.\n\nWhat you ideally want is an option that lets you limit the number of\nprocesses the shell that issued the ulimit call can spawn\nsimultaneously, but I didn't find it in \"man bash/dash/ksh\".\n\n> +'\n> +\n> build_option () {\n> \tgit version --build-options |\n> \tsed -ne \"s/^$1: //p\"\n"},{"id":"457856","messageId":"xmqqpmiyuhjj.fsf@gitster.g","threadId":"57995","inReplyTo":"d3a99a5c5ae538b626e04d7069dd2fc316605dfc.1656044659.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v2 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-24T16:56:16Z","receivedAt":"2022-06-24T16:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Xin writes:\n\n> If a commit is in the commit graph, we would expect the commit to also\n> be present.\n\nHmph, is that a fundamental requirement, or is that a limitation of\nthe current implementation? Naïvely, I do not quite see why we\ncannot first partially clone from a remote, access objects that\nlocally do not exist and lazily fetch them from the promissor, and\nthen build a reachability graph. I expect that the resulting commit\ngraph records the lazily fetched objects at that point. And then we\nshould be able to \"lose\" the lazily fetched objects that we know we\ncan fetch from the promissor again when we need them in the future.\nAnd we would be in a situation where commits are pruned away, not\nlocally available in our object store, but can be (re)fetched from\nthe promisor, no?\n\n> So we should use has_object() instead of\n> repo_has_object_file(), which will help us avoid getting into an endless\n> loop of lazy fetch.\n\nIt all depends on the reason we call lookup_commit_in_graph(), I\nthink. Is there an easy way to remember the fact that we are\nchecking if object X is here with repo_has_object_file(X), so that\nan on-demand fetch that happens when X does not locally exist would\nnot bother checking with lookup_commit_in_graph()? IOW, temporarily\ndisable the use of commit-graph when we are lazily fetching?\n\n> When we found the commit in the graph in lookup_commit_in_graph(),\n> but the commit is missing from the repository, we will try\n> promisor_remote_get_direct() and then enter another loop. While\n> sometimes it will finally succeed because it cannot fork\n> subprocess,\n\nIs that a mode of \"succeed\"-ing? Or merely a way to exit an endless\nloop that does not make any progress with a failure?\n\n> it has exhausted the local process resources and can be harmful to the\n> remote service.\n>\n> Signed-off-by: Han Xin \n> ---\n\nI think the single-liner change in the patch is a good one, but I am\nhaving a hard time to agree with the reasoning above that explains\nwhy it is a good change.\n\nHere is an attempt to express a reasoning I can understand, can\nagree with, and (I think) better describes why the change is a good\none. Does my understanding of the problem and the solution totally\nmisses the mark?\n\n\tThe commit-graph is used to opportunistically optimize\n\taccesses to certain pieces of information on commit objects,\n\tand lookup_commit_in_graph() tries to say \"no\" when the\n\trequested commit does not locally exist by returning NULL,\n\tin which case the caller can ask for (which may result in\n\ton-demand fetching from a promisor remote) and parse the\n\tcommit object itself.\n\n\tHowever, it uses a wrong helper, repo_has_object_file(), to\n\tdo so. This helper not only checks if an object is\n\timmediately available in the local object store, but also\n\ttries to fetch from a promisor remote. But the fetch\n\tmachinery calls lookup_commit_in_graph(), thus causing an\n\tinfinite loop.\n\n\tWe should make lookup_commit_in_graph() expect that a commit\n\tgiven to it can be legitimately missing from the local\n\tobject store, by using the has_object_file() helper instead.\n\t\n> diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> new file mode 100755\n> index 0000000000..4d25d2c950\n> --- /dev/null\n> +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n\nHmph, does this short-test need a completely new file?\n\n> @@ -0,0 +1,47 @@\n> +#!/bin/sh\n> +\n> +test_description='test for no lazy fetch with the commit-graph'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup: prepare a repository with a commit' '\n> +\tgit init with-commit &&\n> +\ttest_commit -C with-commit the-commit &&\n> +\toid=$(git -C with-commit rev-parse HEAD)\n> +'\n> +\n> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> +\tgit init with-commit-graph &&\n> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> +\t# create a ref that points to the commit in alternates\n> +\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n> +\t# prepare some other objects to commit-graph\n> +\ttest_commit -C with-commit-graph somthing &&\n\nsomthing? something?\n\n> +\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n> +\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n> +'\n> +\n> +test_expect_success 'setup: change the alternates to what without the commit' '\n> +\tgit init --bare without-commit &&\n> +\techo \"$(pwd)/without-commit/objects\" \\\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n\nDoesn't this deliberately _corrupt_ the with-commit-graph repository\nthat depended on the object whose name is $oid in with-commit\nrepository? Do we require a corrupt repository to trigger the \"bug\"?\n\n> +\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n> +'\n> +\n> +test_expect_success 'setup: prepare another commit to fetch' '\n> +\ttest_commit -C with-commit another-commit &&\n> +\tanycommit=$(git -C with-commit rev-parse HEAD)\n\nanycommit? another_commit? Be consistent in naming.\n\n> +'\n> +\n> +test_expect_success ULIMIT_PROCESSES 'fetch any commit from promisor with the usage of the commit graph' '\n\nSo we did all of the above set-up sequences only to skip the most\ninteresting test, if we were testing with \"dash\"? I suspect that it\nmay be cleaner to put the prerequisite to the whole file with the\n\"early test_done\" trick like t0051 and t3008.\n\n> +\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n> +\tgit -C with-commit-graph config remote.origin.promisor true &&\n> +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n> +\tGIT_TRACE=\"$(pwd)/trace\" run_with_limited_processses \\\n> +\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n> +\ttest_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n> +\ttest $(grep \"fetch origin\" trace | wc -l) -eq 1\n> +'\n> +\n> +test_done\n\nThanks.\n"},{"id":"457862","messageId":"CAKgqsWVAy8RTSCwG=LVHPoeF5ECSzeNfK4mPacLo=dTeUkc6SA@mail.gmail.com","threadId":"57995","inReplyTo":"xmqqfsjuvyjz.fsf@gitster.g","subject":"Re: Re: [PATCH v2 1/2] test-lib.sh: add limited processes to test-lib","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-25T01:35:35Z","receivedAt":"2022-06-25T01:35:50Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Sat, Jun 25, 2022 at 12:03 AM Junio C Hamano wrote:\n>\n> Han Xin writes:\n>\n> > We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up\n> > commit.\n> >\n> > With run_with_limited_processses() we can limit forking subprocesses and\n> > fail reliably in some test cases.\n> >\n> > Signed-off-by: Han Xin \n> > ---\n> > t/test-lib.sh | 9 +++++++++\n> > 1 file changed, 9 insertions(+)\n> >\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 8ba5ca1534..f920e3b0ae 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n> > run_with_limited_open_files true\n> > '\n> >\n> > +run_with_limited_processses () {\n> > + (ulimit -u 512 && \"$@\")\n>\n> The \"-u\" presumably is a way to say that the current user can have\n> only 512 processes at once that is supported by bash and ksh? dash\n> seems to use \"-p\" for this but \"-p\" of course means something\n> completely different to other shells (and is read-only), which is a\n> mess X-<.\n>\n> I suspect that it is OK to make it practically bash-only, but then ...\n>\n> > +}\n> > +\n> > +test_lazy_prereq ULIMIT_PROCESSES '\n> > + test_have_prereq !HPPA,!MINGW,!CYGWIN &&\n> > + run_with_limited_processses true\n>\n> ... as this lazy-prereq makes a trial run that would fail when the\n> system does not allow \"ulimit -u 512\", do we need the platform\n> specific prereq check? I am wondering if the second line alone is\n> sufficient.\n>\n\nYes，the second line alone is sufficient.\n\n> Also, 512 is not a number I would exactly call \"limit forking\".\n> Does it have to be so high, I wonder. Of course it cannot be so low\n> like 3 or 8 or even 32, as per-user limitation counts your window\n> manager and shells running in other windows.\n>\n\nIt's hard to say.\nI've tried adjusting it to 256, but the test cases in next patch will always\nfail with the following \"err\":\n\n ./test-lib.sh: fork: Resource temporarily unavailable\n\n> What you ideally want is an option that lets you limit the number of\n> processes the shell that issued the ulimit call can spawn\n> simultaneously, but I didn't find it in \"man bash/dash/ksh\".\n>\n\nMaybe I should use \"lib-bash.sh\" instead of \"test-lib.sh\" just like t9902\nand t9903?\nThe different meanings of \"-p\" in bash and dash really make this tricky.\n\nThanks.\n-Han Xin\n\n> > +'\n> > +\n> > build_option () {\n> > git version --build-options |\n> > sed -ne \"s/^$1: //p\"\n"},{"id":"457863","messageId":"CAKgqsWXwf5h7r4fqOnfTbe6vyR25PzQ+hhEddCQV3cMis2ruEg@mail.gmail.com","threadId":"57995","inReplyTo":"xmqqpmiyuhjj.fsf@gitster.g","subject":"Re: Re: [PATCH v2 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-25T02:25:33Z","receivedAt":"2022-06-25T02:25:47Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Sat, Jun 25, 2022 at 12:56 AM Junio C Hamano wrote:\n>\n> Han Xin writes:\n>\n> > If a commit is in the commit graph, we would expect the commit to also\n> > be present.\n>\n> > When we found the commit in the graph in lookup_commit_in_graph(),\n> > but the commit is missing from the repository, we will try\n> > promisor_remote_get_direct() and then enter another loop. While\n> > sometimes it will finally succeed because it cannot fork\n> > subprocess,\n>\n> Is that a mode of \"succeed\"-ing? Or merely a way to exit an endless\n> loop that does not make any progress with a failure?\n\nFor the user, \"fetch-pack\" does succeed, because in\nderef_without_lazy_fetch(), even if lookup_commit_in_graph() fails to\nlazy fetch the lost commit, the following oid_object_info_extended()\nwill help us complete the previous work.\n\nIn a sense, this infinite loop is based on the fact that infinite processes\ncan be created.\n\nHowever, your attempt to express the reasoning bellow is clearer.\n\n>\n> > it has exhausted the local process resources and can be harmful to the\n> > remote service.\n> >\n> > Signed-off-by: Han Xin \n> > ---\n>\n> I think the single-liner change in the patch is a good one, but I am\n> having a hard time to agree with the reasoning above that explains\n> why it is a good change.\n>\n> Here is an attempt to express a reasoning I can understand, can\n> agree with, and (I think) better describes why the change is a good\n> one. Does my understanding of the problem and the solution totally\n> misses the mark?\n>\n> The commit-graph is used to opportunistically optimize\n> accesses to certain pieces of information on commit objects,\n> and lookup_commit_in_graph() tries to say \"no\" when the\n> requested commit does not locally exist by returning NULL,\n> in which case the caller can ask for (which may result in\n> on-demand fetching from a promisor remote) and parse the\n> commit object itself.\n>\n> However, it uses a wrong helper, repo_has_object_file(), to\n> do so. This helper not only checks if an object is\n> immediately available in the local object store, but also\n> tries to fetch from a promisor remote. But the fetch\n> machinery calls lookup_commit_in_graph(), thus causing an\n> infinite loop.\n>\n> We should make lookup_commit_in_graph() expect that a commit\n> given to it can be legitimately missing from the local\n> object store, by using the has_object_file() helper instead.\n>\n> > diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> > new file mode 100755\n> > index 0000000000..4d25d2c950\n> > --- /dev/null\n> > +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n>\n> Hmph, does this short-test need a completely new file?\n>\n> > @@ -0,0 +1,47 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='test for no lazy fetch with the commit-graph'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup: prepare a repository with a commit' '\n> > + git init with-commit &&\n> > + test_commit -C with-commit the-commit &&\n> > + oid=$(git -C with-commit rev-parse HEAD)\n> > +'\n> > +\n> > +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> > + git init with-commit-graph &&\n> > + echo \"$(pwd)/with-commit/.git/objects\" \\\n> > + >with-commit-graph/.git/objects/info/alternates &&\n> > + # create a ref that points to the commit in alternates\n> > + git -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n> > + # prepare some other objects to commit-graph\n> > + test_commit -C with-commit-graph somthing &&\n>\n> somthing? something?\n\nNod.\n\n>\n> > + git -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n> > + test_path_is_file with-commit-graph/.git/objects/info/commit-graph\n> > +'\n> > +\n> > +test_expect_success 'setup: change the alternates to what without the commit' '\n> > + git init --bare without-commit &&\n> > + echo \"$(pwd)/without-commit/objects\" \\\n> > + >with-commit-graph/.git/objects/info/alternates &&\n>\n> Doesn't this deliberately _corrupt_ the with-commit-graph repository\n> that depended on the object whose name is $oid in with-commit\n> repository? Do we require a corrupt repository to trigger the \"bug\"?\n>\n\nThe \"bug\" depends on the commit exist in the commit-graph but\nmissing in the repository.\n\nI didn't find a better way to make this kind of scene.\n\nThis bug was first found when alternates and commit-graph were\nboth used. Since the promise did not maintain all the references,\nI suspect that the \"auto gc\" during the update process of the promise\ncaused the loss of the unreachable commits in the promise.\n\n> > + test_must_fail git -C with-commit-graph cat-file -e $oid\n> > +'\n> > +\n> > +test_expect_success 'setup: prepare another commit to fetch' '\n> > + test_commit -C with-commit another-commit &&\n> > + anycommit=$(git -C with-commit rev-parse HEAD)\n>\n> anycommit? another_commit? Be consistent in naming.\n>\n\nNod.\n\n> > +'\n> > +\n> > +test_expect_success ULIMIT_PROCESSES 'fetch any commit from promisor with the usage of the commit graph' '\n>\n> So we did all of the above set-up sequences only to skip the most\n> interesting test, if we were testing with \"dash\"? I suspect that it\n> may be cleaner to put the prerequisite to the whole file with the\n> \"early test_done\" trick like t0051 and t3008.\n>\n\nIt make sense to me.\n\nThanks.\n-Han Xin\n"},{"id":"457864","messageId":"CAKgqsWXm6aUjG1i7Z7GzKKbs8+yw=dQSu2LWj3fB19LR5aVh_g@mail.gmail.com","threadId":"57995","inReplyTo":"CAKgqsWXwf5h7r4fqOnfTbe6vyR25PzQ+hhEddCQV3cMis2ruEg@mail.gmail.com","subject":"Re: Re: [PATCH v2 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-25T02:31:19Z","receivedAt":"2022-06-25T02:31:33Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Sat, Jun 25, 2022 at 10:25 AM Han Xin wrote:\n>\n> The \"bug\" depends on the commit exist in the commit-graph but\n> missing in the repository.\n>\n> I didn't find a better way to make this kind of scene.\n>\n> This bug was first found when alternates and commit-graph were\n> both used. Since the promise did not maintain all the references,\n> I suspect that the \"auto gc\" during the update process of the promise\n> caused the loss of the unreachable commits in the promise.\n>\n\nSome mistakes here:\nThis bug was first found when alternates and commit-graph were\nboth used. Since the promise did not maintain all the references,\nI suspect that the \"auto gc\" during the update of the alternates\ncaused the loss of the unreachable commits.\n"},{"id":"457901","messageId":"xmqqpmiuqosw.fsf@gitster.g","threadId":"57995","inReplyTo":"CAKgqsWVAy8RTSCwG=LVHPoeF5ECSzeNfK4mPacLo=dTeUkc6SA@mail.gmail.com","subject":"Re: [PATCH v2 1/2] test-lib.sh: add limited processes to test-lib","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-27T12:22:07Z","receivedAt":"2022-06-27T12:22:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Xin writes:\n\n> Maybe I should use \"lib-bash.sh\" instead of \"test-lib.sh\" just like t9902\n> and t9903?\n> The different meanings of \"-p\" in bash and dash really make this tricky.\n\nI do not think lib-bash.sh is appropriate for this.\n\nt9902/9903 are about command line prompt and completion support\n_FOR_ bash users. By including lib-bash.sh, even if your \"usual\"\nshell is not bash, you can run these two scripts under bash, as long\nas you have it installed.\n\nFor the purpose of testing these bash specific features, that\nframework makes quite a lot of sense. Those who are happy to have\ndash on their system without having to install bash would have no\nreason to see these two tests to pass, as they do not care about\nbash at all.\n\nWhat the test under discussion is doing is quite different. Instead\nof forcing to re-spawn bash when the user's shell is not bash, you'd\nwant to adjust how you invoke \"ulimit\" if it is not bash, something\nlike\n\nrun_with_limited_processes () {\n\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n}\n\ntest_lazy_prereq ULIMIT_PROCESSES '\n\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n\tif test -n \"$BASH_VERSION\"\n\tthen\n\t\tulimit_max_process=\"-u\"\n\telif test -n \"$KSH_VERSION\"\n\tthen\n\t\tulimit_max_process=\"-u\"\n\tfi\n run_with_limited_processes true\n'\n\nperhaps?\n"},{"id":"457982","messageId":"cover.1656381667.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656044659.git.hanxin.hx@bytedance.com","subject":"[PATCH v3 0/2] no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-28T02:02:50Z","receivedAt":"2022-06-28T02:03:13Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"This patch fixes the following issue:\nWhen we found the commit in the graph in lookup_commit_in_graph(), but\nthe commit is missing from the repository, we will try\npromisor_remote_get_direct() and then enter another loop.\n\nThen we will go into an endless loop:\n git fetch -> deref_without_lazy_fetch() ->\n lookup_commit_in_graph() -> repo_has_object_file() ->\n promisor_remote_get_direct() -> fetch_objects() ->\n git fetch (a new loop round)\n\nChanges since v2:\n\n* Remove test_have_prereq() from ULIMIT_PROCESSES as\n \"run_with_limited_processses true\" is enough.\n\n* Teach run_with_limited_processses() to support dash and zsh.\n\n* Skip the whole test file if ulimit is not avaliable.\n\n* Minor grammar/comment etc. fixes throughout.\n\nHan Xin (2):\n test-lib.sh: add limited processes to test-lib\n commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n\n commit-graph.c | 2 +-\n t/t5329-no-lazy-fetch-with-commit-graph.sh | 53 ++++++++++++++++++++++\n t/test-lib.sh | 16 +++++++\n 3 files changed, 70 insertions(+), 1 deletion(-)\n create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n\nRange-diff against v2:\n1: 442a4c351d ! 1: ad0a539759 test-lib.sh: add limited processes to test-lib\n @@ t/test-lib.sh: test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n '\n \n +run_with_limited_processses () {\n -+\t(ulimit -u 512 && \"$@\")\n ++\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n ++\tif test -n \"$BASH_VERSION\"\n ++\tthen\n ++\t\tulimit_max_process=\"-u\"\n ++\telif test -n \"$KSH_VERSION\"\n ++\tthen\n ++\t\tulimit_max_process=\"-u\"\n ++\tfi\n ++\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n +}\n +\n +test_lazy_prereq ULIMIT_PROCESSES '\n -+\ttest_have_prereq !HPPA,!MINGW,!CYGWIN &&\n +\trun_with_limited_processses true\n +'\n +\n2: a7d456db9b ! 2: 3cdb1abd43 commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n @@ Metadata\n ## Commit message ##\n commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n \n - If a commit is in the commit graph, we would expect the commit to also\n - be present. So we should use has_object() instead of\n - repo_has_object_file(), which will help us avoid getting into an endless\n - loop of lazy fetch.\n + The commit-graph is used to opportunistically optimize accesses to\n + certain pieces of information on commit objects, and\n + lookup_commit_in_graph() tries to say \"no\" when the requested commit\n + does not locally exist by returning NULL, in which case the caller\n + can ask for (which may result in on-demand fetching from a promisor\n + remote) and parse the commit object itself.\n \n - When we found the commit in the graph in lookup_commit_in_graph(), but\n - the commit is missing from the repository, we will try\n - promisor_remote_get_direct() and then enter another loop. While\n - sometimes it will finally succeed because it cannot fork subprocess,\n - it has exhausted the local process resources and can be harmful to the\n - remote service.\n + However, it uses a wrong helper, repo_has_object_file(), to do so.\n + This helper not only checks if an object is mmediately available in\n + the local object store, but also tries to fetch from a promisor remote.\n + But the fetch machinery calls lookup_commit_in_graph(), thus causing an\n + infinite loop.\n +\n + We should make lookup_commit_in_graph() expect that a commit given to it\n + can be legitimately missing from the local object store, by using the\n + has_object_file() helper instead.\n \n Signed-off-by: Han Xin \n \n @@ t/t5329-no-lazy-fetch-with-commit-graph.sh (new)\n +\n +. ./test-lib.sh\n +\n ++if ! test_have_prereq ULIMIT_PROCESSES\n ++then\n ++\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n ++\ttest_done\n ++fi\n ++\n +test_expect_success 'setup: prepare a repository with a commit' '\n +\tgit init with-commit &&\n +\ttest_commit -C with-commit the-commit &&\n @@ t/t5329-no-lazy-fetch-with-commit-graph.sh (new)\n +\t# create a ref that points to the commit in alternates\n +\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n +\t# prepare some other objects to commit-graph\n -+\ttest_commit -C with-commit-graph somthing &&\n ++\ttest_commit -C with-commit-graph something &&\n +\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n +\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n +'\n @@ t/t5329-no-lazy-fetch-with-commit-graph.sh (new)\n +\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n +'\n +\n -+test_expect_success 'setup: prepare another commit to fetch' '\n -+\ttest_commit -C with-commit another-commit &&\n ++test_expect_success 'setup: prepare any commit to fetch' '\n ++\ttest_commit -C with-commit any-commit &&\n +\tanycommit=$(git -C with-commit rev-parse HEAD)\n +'\n +\n -+test_expect_success ULIMIT_PROCESSES 'fetch any commit from promisor with the usage of the commit graph' '\n ++test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n +\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n +\tgit -C with-commit-graph config remote.origin.promisor true &&\n +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n-- \n2.36.1\n\n"},{"id":"457983","messageId":"ad0a539759ab79adb7a5b4c87f1a1548012ffbbe.1656381667.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656381667.git.hanxin.hx@bytedance.com","subject":"[PATCH v3 1/2] test-lib.sh: add limited processes to test-lib","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-28T02:02:51Z","receivedAt":"2022-06-28T02:03:37Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up\ncommit.\n\nWith run_with_limited_processses() we can limit forking subprocesses and\nfail reliably in some test cases.\n\nSigned-off-by: Han Xin \n---\n t/test-lib.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8ba5ca1534..655d6d543f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1816,6 +1816,22 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '\n \trun_with_limited_open_files true\n '\n \n+run_with_limited_processses () {\n+\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n+\tif test -n \"$BASH_VERSION\"\n+\tthen\n+\t\tulimit_max_process=\"-u\"\n+\telif test -n \"$KSH_VERSION\"\n+\tthen\n+\t\tulimit_max_process=\"-u\"\n+\tfi\n+\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n+}\n+\n+test_lazy_prereq ULIMIT_PROCESSES '\n+\trun_with_limited_processses true\n+'\n+\n build_option () {\n \tgit version --build-options |\n \tsed -ne \"s/^$1: //p\"\n-- \n2.36.1\n\n"},{"id":"457984","messageId":"3cdb1abd43779844b8e8dc094e2fd2da1adc461a.1656381667.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656381667.git.hanxin.hx@bytedance.com","subject":"[PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-28T02:02:52Z","receivedAt":"2022-06-28T02:03:41Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"The commit-graph is used to opportunistically optimize accesses to\ncertain pieces of information on commit objects, and\nlookup_commit_in_graph() tries to say \"no\" when the requested commit\ndoes not locally exist by returning NULL, in which case the caller\ncan ask for (which may result in on-demand fetching from a promisor\nremote) and parse the commit object itself.\n\nHowever, it uses a wrong helper, repo_has_object_file(), to do so.\nThis helper not only checks if an object is mmediately available in\nthe local object store, but also tries to fetch from a promisor remote.\nBut the fetch machinery calls lookup_commit_in_graph(), thus causing an\ninfinite loop.\n\nWe should make lookup_commit_in_graph() expect that a commit given to it\ncan be legitimately missing from the local object store, by using the\nhas_object_file() helper instead.\n\nSigned-off-by: Han Xin \n---\n commit-graph.c | 2 +-\n t/t5329-no-lazy-fetch-with-commit-graph.sh | 53 ++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 1 deletion(-)\n create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2b52818731..2dd9bcc7ea 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!has_object(repo, id, 0))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\nnew file mode 100755\nindex 0000000000..d7877a5758\n--- /dev/null\n+++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+test_description='test for no lazy fetch with the commit-graph'\n+\n+. ./test-lib.sh\n+\n+if ! test_have_prereq ULIMIT_PROCESSES\n+then\n+\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n+\ttest_done\n+fi\n+\n+test_expect_success 'setup: prepare a repository with a commit' '\n+\tgit init with-commit &&\n+\ttest_commit -C with-commit the-commit &&\n+\toid=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n+\tgit init with-commit-graph &&\n+\techo \"$(pwd)/with-commit/.git/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\t# create a ref that points to the commit in alternates\n+\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n+\t# prepare some other objects to commit-graph\n+\ttest_commit -C with-commit-graph something &&\n+\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n+\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n+'\n+\n+test_expect_success 'setup: change the alternates to what without the commit' '\n+\tgit init --bare without-commit &&\n+\techo \"$(pwd)/without-commit/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n+'\n+\n+test_expect_success 'setup: prepare any commit to fetch' '\n+\ttest_commit -C with-commit any-commit &&\n+\tanycommit=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n+\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n+\tgit -C with-commit-graph config remote.origin.promisor true &&\n+\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n+\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace\" \\\n+\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n+\ttest_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n+\ttest $(grep \"fetch origin\" trace | wc -l) -eq 1\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"457989","messageId":"220628.865yklgr6g.gmgdl@evledraar.gmail.com","threadId":"57995","inReplyTo":"3cdb1abd43779844b8e8dc094e2fd2da1adc461a.1656381667.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-28T07:49:58Z","receivedAt":"2022-06-28T07:53:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 28 2022, Han Xin wrote:\n\n> The commit-graph is used to opportunistically optimize accesses to\n> certain pieces of information on commit objects, and\n> lookup_commit_in_graph() tries to say \"no\" when the requested commit\n> does not locally exist by returning NULL, in which case the caller\n> can ask for (which may result in on-demand fetching from a promisor\n> remote) and parse the commit object itself.\n>\n> However, it uses a wrong helper, repo_has_object_file(), to do so.\n> This helper not only checks if an object is mmediately available in\n> the local object store, but also tries to fetch from a promisor remote.\n> But the fetch machinery calls lookup_commit_in_graph(), thus causing an\n> infinite loop.\n>\n> We should make lookup_commit_in_graph() expect that a commit given to it\n> can be legitimately missing from the local object store, by using the\n> has_object_file() helper instead.\n>\n> Signed-off-by: Han Xin \n> ---\n> commit-graph.c | 2 +-\n> t/t5329-no-lazy-fetch-with-commit-graph.sh | 53 ++++++++++++++++++++++\n> 2 files changed, 54 insertions(+), 1 deletion(-)\n> create mode 100755 t/t5329-no-lazy-fetch-with-commit-graph.sh\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 2b52818731..2dd9bcc7ea 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -907,7 +907,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n> \t\treturn NULL;\n> \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> \t\treturn NULL;\n> -\tif (!repo_has_object_file(repo, id))\n> +\tif (!has_object(repo, id, 0))\n> \t\treturn NULL;\n> \n> \tcommit = lookup_commit(repo, id);\n> diff --git a/t/t5329-no-lazy-fetch-with-commit-graph.sh b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> new file mode 100755\n> index 0000000000..d7877a5758\n> --- /dev/null\n> +++ b/t/t5329-no-lazy-fetch-with-commit-graph.sh\n> @@ -0,0 +1,53 @@\n> +#!/bin/sh\n> +\n> +test_description='test for no lazy fetch with the commit-graph'\n> +\n> +. ./test-lib.sh\n> +\n> +if ! test_have_prereq ULIMIT_PROCESSES\n\nI think the prereq in 1/2 would be better off squashed into this commit.\n\n> +then\n> +\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n> +\ttest_done\n> +fi\n> +\n> +test_expect_success 'setup: prepare a repository with a commit' '\n> +\tgit init with-commit &&\n> +\ttest_commit -C with-commit the-commit &&\n> +\toid=$(git -C with-commit rev-parse HEAD)\n> +'\n> +\n> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> +\tgit init with-commit-graph &&\n> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n\nnit: you can use $PWD instead of $(pwd).\n\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> +\t# create a ref that points to the commit in alternates\n> +\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n> +\t# prepare some other objects to commit-graph\n> +\ttest_commit -C with-commit-graph something &&\n> +\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n> +\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n> +'\n> +\n> +test_expect_success 'setup: change the alternates to what without the commit' '\n> +\tgit init --bare without-commit &&\n\nMaybe run a successful cat-file here...\n> +\techo \"$(pwd)/without-commit/objects\" \\\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> +\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n...to show that it fails after the \"echo\" above?\n> +'\n> +\n> +test_expect_success 'setup: prepare any commit to fetch' '\n> +\ttest_commit -C with-commit any-commit &&\n> +\tanycommit=$(git -C with-commit rev-parse HEAD)\n\nI think this would be better just added before a \\n\\n in the next test.\n> +'\n> +\n> +test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n> +\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n> +\tgit -C with-commit-graph config remote.origin.promisor true &&\n> +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n> +\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace\" \\\n> +\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n\n\n> +\ttest_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n> +\ttest $(grep \"fetch origin\" trace | wc -l) -eq 1\n\n\nUse \"grep\", not \"test_i18ngrep\", and this should use \"test_line_count\".\n\nBut actually better yet: this whole thing looks like it could use\n\"test_subcommand\" instead, couldn't it?\n"},{"id":"458055","messageId":"xmqq35folmgf.fsf@gitster.g","threadId":"57995","inReplyTo":"220628.865yklgr6g.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-28T17:36:16Z","receivedAt":"2022-06-28T17:36:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> +test_description='test for no lazy fetch with the commit-graph'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +if ! test_have_prereq ULIMIT_PROCESSES\n>\n> I think the prereq in 1/2 would be better off squashed into this commit.\n\nGood thinking. It also may make sense to implement it in this file,\nwithout touching test-lib.sh at all.\n\n>> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n>> +\tgit init with-commit-graph &&\n>> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n>> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n>\n> nit: you can use $PWD instead of $(pwd).\n\nWe can, and it would not make any difference on non-Windows. \n\nBut which one should we use to cater to Windows? $(pwd) is a full\npath in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\nMSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n"},{"id":"458080","messageId":"CAKgqsWXawRg6DgvORs709YSsQFqKgiQ=u2LN8Fx3LVXdfbJAag@mail.gmail.com","threadId":"57995","inReplyTo":"220628.865yklgr6g.gmgdl@evledraar.gmail.com","subject":"Re: Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-06-29T02:08:49Z","receivedAt":"2022-06-29T02:09:04Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Tue, Jun 28, 2022 at 3:53 PM Ævar Arnfjörð Bjarmason\n wrote:\n> > + test_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n> > + test $(grep \"fetch origin\" trace | wc -l) -eq 1\n>\n>\n> Use \"grep\", not \"test_i18ngrep\", and this should use \"test_line_count\".\n>\n> But actually better yet: this whole thing looks like it could use\n> \"test_subcommand\" instead, couldn't it?\n\nWhen using test_subcommand() we should give all the args,\nif we remove or add any args later, this test case will always\npass even without this fix. So, is this test case still strict?\n\n run_with_limited_processses env GIT_TRACE2_EVENT=\"$(PWD)/trace.txt\" \\\n git -C with-commit-graph fetch origin $anycommit &&\n test_subcommand ! git -c fetch.negotiationAlgorithm=noop \\\n fetch origin --no-tags --no-write-fetch-head \\\n --recurse-submodules=no --filter=blob:none \\\n --stdin Ævar Arnfjörð Bjarmason writes:\n>\n> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> >> +\tgit init with-commit-graph &&\n> >> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n> >> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> >\n> > nit: you can use $PWD instead of $(pwd).\n>\n> We can, and it would not make any difference on non-Windows.\n>\n> But which one should we use to cater to Windows? $(pwd) is a full\n> path in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\n> MSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n\nIndeed, and since the `alternates` file is supposed to be read by\n`git.exe`, a non-MSYS program, the original was good, and the nit\nsuggested the incorrect form.\n\nThank you for catching this before it was worsimproved,\nDscho\n"},{"id":"458190","messageId":"220630.86v8siclh5.gmgdl@evledraar.gmail.com","threadId":"57995","inReplyTo":"5n35o008-pso2-6440-424p-q387q9n4so41@tzk.qr","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-30T13:43:48Z","receivedAt":"2022-06-30T13:47:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 30 2022, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Tue, 28 Jun 2022, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n>> >> +\tgit init with-commit-graph &&\n>> >> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n>> >> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n>> >\n>> > nit: you can use $PWD instead of $(pwd).\n>>\n>> We can, and it would not make any difference on non-Windows.\n>>\n>> But which one should we use to cater to Windows? $(pwd) is a full\n>> path in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\n>> MSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n>\n> Indeed, and since the `alternates` file is supposed to be read by\n> `git.exe`, a non-MSYS program, the original was good, and the nit\n> suggested the incorrect form.\n\nI looked at t5615-alternate-env.sh which does the equivalent of:\n\n\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$PWD/one.git/objects:$PWD/two.git/objects\" \\\n \tgit cat-file [...]\n\nWe run that test on all our platforms, does the $PWD form work in the\nenvironment variable, but not when we write it to the \"alternates\" file?\nOr is there some other subtlety there that I'm missing?\n\n"},{"id":"458197","messageId":"xmqq5ykignwb.fsf@gitster.g","threadId":"57995","inReplyTo":"220630.86v8siclh5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-30T15:40:52Z","receivedAt":"2022-06-30T15:41:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Jun 30 2022, Johannes Schindelin wrote:\n>\n>> Hi Junio,\n>>\n>> On Tue, 28 Jun 2022, Junio C Hamano wrote:\n>>\n>>> Ævar Arnfjörð Bjarmason writes:\n>>>\n>>> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n>>> >> +\tgit init with-commit-graph &&\n>>> >> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n>>> >> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n>>> >\n>>> > nit: you can use $PWD instead of $(pwd).\n>>>\n>>> We can, and it would not make any difference on non-Windows.\n>>>\n>>> But which one should we use to cater to Windows? $(pwd) is a full\n>>> path in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\n>>> MSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n>>\n>> Indeed, and since the `alternates` file is supposed to be read by\n>> `git.exe`, a non-MSYS program, the original was good, and the nit\n>> suggested the incorrect form.\n>\n> I looked at t5615-alternate-env.sh which does the equivalent of:\n>\n> \tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$PWD/one.git/objects:$PWD/two.git/objects\" \\\n> \tgit cat-file [...]\n>\n> We run that test on all our platforms, does the $PWD form work in the\n> environment variable, but not when we write it to the \"alternates\" file?\n> Or is there some other subtlety there that I'm missing?\n\nI am also curious to see a clear and concise explanation so that we\ndo not have to repeat this discussion later. We have\n\n - When a test checks for an absolute path that a git command generated,\n construct the expected value using $(pwd) rather than $PWD,\n $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on\n Windows, where the shell (MSYS bash) mangles absolute path names.\n For details, see the commit message of 4114156ae9.\n\nin t/README, but even with the log mesasge of 4114156a (Tests on\nWindows: $(pwd) must return Windows-style paths, 2009-03-13) [*1*],\nI have no idea what makes the thing you found in t5615 work and your\nsuggestion to use $PWD in the new one not work.\n\nGIT_ALTERNATE_OBJECT_DIRECTORIES is a PATH_SEP (not necessarily a\ncolon) separated list, and I think the way t5615 uses it is broken\non Windows where PATH_SEP is defined as semicolon without the $PWD\nvs $(pwd) issue. Is the test checking the right thing?\n\n\n[Footnote]\n\n*1*\n\n Tests on Windows: $(pwd) must return Windows-style paths\n\n Many tests pass $(pwd) in some form to git and later test that the output\n of git contains the correct value of $(pwd). For example, the test of\n 'git remote show' sets up a remote that contains $(pwd) and then the\n expected result must contain $(pwd).\n\n Again, MSYS-bash's path mangling kicks in: Plain $(pwd) uses the MSYS style\n absolute path /c/path/to/git. The test case would write this name into\n the 'expect' file. But when git is invoked, MSYS-bash converts this name to\n the Windows style path c:/path/to/git, and git would produce this form in\n the result; the test would fail.\n\n We fix this by passing -W to bash's pwd that produces the Windows-style\n path.\n\n There are a two cases that need an accompanying change:\n\n - In t1504 the value of $(pwd) becomes part of a path list. In this case,\n the lone 'c' in something like /foo:c:/path/to/git:/bar inhibits\n MSYS-bashes path mangling; IOW in this case we want the /c/path/to/git\n form to allow path mangling. We use $PWD instead of $(pwd), which always\n has the latter form.\n\n - In t6200, $(pwd) - the Windows style path - must be used to construct the\n expected result because that is the path form that git sees. (The change\n in the test itself is just for consistency: 'git fetch' always sees the\n Windows-style path, with or without the change.)\n\n Signed-off-by: Johannes Sixt \n\n"},{"id":"458217","messageId":"220630.86mtducaix.gmgdl@evledraar.gmail.com","threadId":"57995","inReplyTo":"cover.1656381667.git.hanxin.hx@bytedance.com","subject":"test name conflict + js/ci-github-workflow-markup regression (was: [PATCH v3 0/2] no lazy fetch in lookup_commit_in_graph())","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-30T17:37:13Z","receivedAt":"2022-06-30T17:43:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 28 2022, Han Xin wrote:\n\n> t/t5329-no-lazy-fetch-with-commit-graph.sh | 53 ++++++++++++++++++++++\n\nThis fails \"make test\" since 5b92477f896 (builtin/gc.c: conditionally\navoid pruning objects via loose, 2022-05-20), i.e. we have another\nt/t5329* test now.\n\nPer $subject the CI output for this is now a bit cryptic, it lands you\non a failed run-build-and-test.sh step with:\n\t\n\t[...]\n\t=> Run tests\n\tcat: 't/test-results/*.exit': No such file or directory\n\t=== Failed test: * ===\n\tThe full logs are in the 'print test failures' step below.\n\tSee also the 'failed-tests-*' artifacts attached to this run.\n\tcat: 't/test-results/*.markup': No such file or directory\n\tError: Process completed with exit code 1.\n\nThe last line of the suggested next step is then:\n\n\tBuild job failed before the tests could have been run\n\nGoing earlier and expanding the \"Run tests\" step we can see the issue:\n\t\n\tduplicate test numbers: t5329\n\tmake[1]: *** [Makefile:86: test-lint-duplicates] Error 1\n\tmake[1]: *** Waiting for unfinished jobs....\n\tmake[1]: Leaving directory '/home/runner/work/git/git/t'\n\tmake: *** [Makefile:3065: test] Error 2\n\t+ res=2\n\t+ rm exit.status\n\t+ end_group\n\t+ test -n t\n\t+ set +x\n\nSince the CI topic in $subject we've ran the \"print failures\" step\nseparately from the \"make\" invocation, and therefore have to guess at\nwhy we fail, whereas before we'd get that output from \"make\" itself.\n\nJohannes, is this something you can fix?\n\nIn any case, for this topic the fix is simple: The test needs to be\nrenamed for a re-roll,\n"},{"id":"458242","messageId":"220630.86edz6c75c.gmgdl@evledraar.gmail.com","threadId":"57995","inReplyTo":"xmqq5ykignwb.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-30T18:47:30Z","receivedAt":"2022-06-30T18:56:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 30 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Thu, Jun 30 2022, Johannes Schindelin wrote:\n>>\n>>> Hi Junio,\n>>>\n>>> On Tue, 28 Jun 2022, Junio C Hamano wrote:\n>>>\n>>>> Ævar Arnfjörð Bjarmason writes:\n>>>>\n>>>> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n>>>> >> +\tgit init with-commit-graph &&\n>>>> >> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n>>>> >> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n>>>> >\n>>>> > nit: you can use $PWD instead of $(pwd).\n>>>>\n>>>> We can, and it would not make any difference on non-Windows.\n>>>>\n>>>> But which one should we use to cater to Windows? $(pwd) is a full\n>>>> path in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\n>>>> MSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n>>>\n>>> Indeed, and since the `alternates` file is supposed to be read by\n>>> `git.exe`, a non-MSYS program, the original was good, and the nit\n>>> suggested the incorrect form.\n>>\n>> I looked at t5615-alternate-env.sh which does the equivalent of:\n>>\n>> \tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$PWD/one.git/objects:$PWD/two.git/objects\" \\\n>> \tgit cat-file [...]\n>>\n>> We run that test on all our platforms, does the $PWD form work in the\n>> environment variable, but not when we write it to the \"alternates\" file?\n>> Or is there some other subtlety there that I'm missing?\n>\n> I am also curious to see a clear and concise explanation so that we\n> do not have to repeat this discussion later. We have\n>\n> - When a test checks for an absolute path that a git command generated,\n> construct the expected value using $(pwd) rather than $PWD,\n> $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on\n> Windows, where the shell (MSYS bash) mangles absolute path names.\n> For details, see the commit message of 4114156ae9.\n>\n> in t/README, but even with the log mesasge of 4114156a (Tests on\n> Windows: $(pwd) must return Windows-style paths, 2009-03-13) [*1*],\n> I have no idea what makes the thing you found in t5615 work and your\n> suggestion to use $PWD in the new one not work.\n>\n> GIT_ALTERNATE_OBJECT_DIRECTORIES is a PATH_SEP (not necessarily a\n> colon) separated list, and I think the way t5615 uses it is broken\n> on Windows where PATH_SEP is defined as semicolon without the $PWD\n> vs $(pwd) issue. Is the test checking the right thing?\n\nWhatever th explanation is CI passed with a $(pwd) -> $PWD repacement in\nthe test being introduced here:\nhttps://github.com/avar/git/runs/7136686130?check_suite_focus=true\n\nDiff here:\nhttps://github.com/avar/git/commit/606d0060a57b7021396919044c7696489b7835cd\n\nSo either $PWD is fine there, or our Windows CI doesn't reflect this\nparticular caveat on some Windows systems, or the test is erroneously\npassing with an invalid value. Knowing which of those it is would be\nvery useful...\n"},{"id":"458314","messageId":"cover.1656593279.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656381667.git.hanxin.hx@bytedance.com","subject":"[PATCH v4 0/1] no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-01T01:34:29Z","receivedAt":"2022-07-01T01:34:44Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"Changes since v3:\n\n* Move run_with_limited_processses() into \n t5330-no-lazy-fetch-with-commit-graph.sh without touching test-lib.sh.\n\n* Squash \"setup: prepare any commit to fetch\" into the main body of the\n test.\n\n* Minor grammar/comment etc. fixes throughout.\n\nHan Xin (1):\n commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n\n commit-graph.c | 2 +-\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 70 ++++++++++++++++++++++\n 2 files changed, 71 insertions(+), 1 deletion(-)\n create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n\nRange-diff against v3:\n1: ad0a539759 < -: ---------- test-lib.sh: add limited processes to test-lib\n2: 3cdb1abd43 ! 1: 96d4bb7150 commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n @@ commit-graph.c: struct commit *lookup_commit_in_graph(struct repository *repo, c\n \n \tcommit = lookup_commit(repo, id);\n \n - ## t/t5329-no-lazy-fetch-with-commit-graph.sh (new) ##\n + ## t/t5330-no-lazy-fetch-with-commit-graph.sh (new) ##\n @@\n +#!/bin/sh\n +\n @@ t/t5329-no-lazy-fetch-with-commit-graph.sh (new)\n +\n +. ./test-lib.sh\n +\n ++run_with_limited_processses () {\n ++\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n ++\tif test -n \"$BASH_VERSION\"\n ++\tthen\n ++\t\tulimit_max_process=\"-u\"\n ++\telif test -n \"$KSH_VERSION\"\n ++\tthen\n ++\t\tulimit_max_process=\"-u\"\n ++\tfi\n ++\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n ++}\n ++\n ++test_lazy_prereq ULIMIT_PROCESSES '\n ++\trun_with_limited_processses true\n ++'\n ++\n +if ! test_have_prereq ULIMIT_PROCESSES\n +then\n +\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n @@ t/t5329-no-lazy-fetch-with-commit-graph.sh (new)\n +\n +test_expect_success 'setup: change the alternates to what without the commit' '\n +\tgit init --bare without-commit &&\n ++\tgit -C with-commit-graph cat-file -e $oid &&\n +\techo \"$(pwd)/without-commit/objects\" \\\n +\t\t>with-commit-graph/.git/objects/info/alternates &&\n +\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n +'\n +\n -+test_expect_success 'setup: prepare any commit to fetch' '\n -+\ttest_commit -C with-commit any-commit &&\n -+\tanycommit=$(git -C with-commit rev-parse HEAD)\n -+'\n -+\n +test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n ++\t# setup promisor and prepare any commit to fetch\n +\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n +\tgit -C with-commit-graph config remote.origin.promisor true &&\n +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n -+\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace\" \\\n ++\ttest_commit -C with-commit any-commit &&\n ++\tanycommit=$(git -C with-commit rev-parse HEAD) &&\n ++\n ++\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n +\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n -+\ttest_i18ngrep ! \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n -+\ttest $(grep \"fetch origin\" trace | wc -l) -eq 1\n ++\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n ++\tgrep \"git fetch origin\" trace.txt >actual &&\n ++\ttest_line_count = 1 actual\n +'\n +\n +test_done\n-- \n2.36.1\n\n"},{"id":"458315","messageId":"96d4bb71505d87ed501c058bbd89bfc13d08b24a.1656593279.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656593279.git.hanxin.hx@bytedance.com","subject":"[PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-01T01:34:30Z","receivedAt":"2022-07-01T01:34:49Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"The commit-graph is used to opportunistically optimize accesses to\ncertain pieces of information on commit objects, and\nlookup_commit_in_graph() tries to say \"no\" when the requested commit\ndoes not locally exist by returning NULL, in which case the caller\ncan ask for (which may result in on-demand fetching from a promisor\nremote) and parse the commit object itself.\n\nHowever, it uses a wrong helper, repo_has_object_file(), to do so.\nThis helper not only checks if an object is mmediately available in\nthe local object store, but also tries to fetch from a promisor remote.\nBut the fetch machinery calls lookup_commit_in_graph(), thus causing an\ninfinite loop.\n\nWe should make lookup_commit_in_graph() expect that a commit given to it\ncan be legitimately missing from the local object store, by using the\nhas_object_file() helper instead.\n\nSigned-off-by: Han Xin \n---\n commit-graph.c | 2 +-\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 70 ++++++++++++++++++++++\n 2 files changed, 71 insertions(+), 1 deletion(-)\n create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d4503336..2b04ef072d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -898,7 +898,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!has_object(repo, id, 0))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\nnew file mode 100755\nindex 0000000000..be33334229\n--- /dev/null\n+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n@@ -0,0 +1,70 @@\n+#!/bin/sh\n+\n+test_description='test for no lazy fetch with the commit-graph'\n+\n+. ./test-lib.sh\n+\n+run_with_limited_processses () {\n+\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n+\tif test -n \"$BASH_VERSION\"\n+\tthen\n+\t\tulimit_max_process=\"-u\"\n+\telif test -n \"$KSH_VERSION\"\n+\tthen\n+\t\tulimit_max_process=\"-u\"\n+\tfi\n+\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n+}\n+\n+test_lazy_prereq ULIMIT_PROCESSES '\n+\trun_with_limited_processses true\n+'\n+\n+if ! test_have_prereq ULIMIT_PROCESSES\n+then\n+\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n+\ttest_done\n+fi\n+\n+test_expect_success 'setup: prepare a repository with a commit' '\n+\tgit init with-commit &&\n+\ttest_commit -C with-commit the-commit &&\n+\toid=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n+\tgit init with-commit-graph &&\n+\techo \"$(pwd)/with-commit/.git/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\t# create a ref that points to the commit in alternates\n+\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n+\t# prepare some other objects to commit-graph\n+\ttest_commit -C with-commit-graph something &&\n+\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n+\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n+'\n+\n+test_expect_success 'setup: change the alternates to what without the commit' '\n+\tgit init --bare without-commit &&\n+\tgit -C with-commit-graph cat-file -e $oid &&\n+\techo \"$(pwd)/without-commit/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n+'\n+\n+test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n+\t# setup promisor and prepare any commit to fetch\n+\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n+\tgit -C with-commit-graph config remote.origin.promisor true &&\n+\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n+\ttest_commit -C with-commit any-commit &&\n+\tanycommit=$(git -C with-commit rev-parse HEAD) &&\n+\n+\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n+\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n+\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n+\tgrep \"git fetch origin\" trace.txt >actual &&\n+\ttest_line_count = 1 actual\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"458410","messageId":"n3p471no-671q-2701-1r72-s0q02ns09053@tzk.qr","threadId":"57995","inReplyTo":"xmqq5ykignwb.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-07-01T19:31:26Z","receivedAt":"2022-07-01T19:31:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 30 Jun 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n> > On Thu, Jun 30 2022, Johannes Schindelin wrote:\n> >\n> >> On Tue, 28 Jun 2022, Junio C Hamano wrote:\n> >>\n> >>> Ævar Arnfjörð Bjarmason writes:\n> >>>\n> >>> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> >>> >> +\tgit init with-commit-graph &&\n> >>> >> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n> >>> >> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> >>> >\n> >>> > nit: you can use $PWD instead of $(pwd).\n> >>>\n> >>> We can, and it would not make any difference on non-Windows.\n> >>>\n> >>> But which one should we use to cater to Windows? $(pwd) is a full\n> >>> path in Windows notation \"C:\\Program Files\\Git\\...\" while $PWD is\n> >>> MSYS style \"/C/Program Files/Git/...\" or something like that, IIRC?\n> >>\n> >> Indeed, and since the `alternates` file is supposed to be read by\n> >> `git.exe`, a non-MSYS program, the original was good, and the nit\n> >> suggested the incorrect form.\n> >\n> > I looked at t5615-alternate-env.sh which does the equivalent of:\n> >\n> > \tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$PWD/one.git/objects:$PWD/two.git/objects\" \\\n> > \tgit cat-file [...]\n> >\n> > We run that test on all our platforms, does the $PWD form work in the\n> > environment variable, but not when we write it to the \"alternates\" file?\n> > Or is there some other subtlety there that I'm missing?\n>\n> I am also curious to see a clear and concise explanation so that we\n> do not have to repeat this discussion later.\n\nUnfortunately, I do not see any way to explain this concisely: MSYS2 does\nhard-to-explain things here, in the hopes to Do The Right Thing (most of\nthe time, anyways).\n\nWhenever you call a non-MSYS program from an MSYS program (and remember,\nan MSYS program is a program that uses the MSYS2 runtime that acts as a\nPOSIX emulation layer), \"magic\" things are done. In our context,\n`bash.exe` is an MSYS program, and the non-MSYS program that is called is\n`git.exe`.\n\nSo what are those \"magic\" things? The command-line arguments and the\nenvironment variables are auto-converted: everything that looks like a\nUnix-style path (or path list, like the `PATH` environment variable) is\nconverted to a Windows-style path or path list (on Windows, the colon\ncannot be the separator in `PATH`, therefore the semicolon is used).\n\nAnd this is where it gets _really_ tricky to explain what is going on:\nwhat _does_ look like a Unix-style path? The exact rules are convoluted\nand hard to explain, but they work _most of the time_. For example,\n`/usr/bin:/hello` is converted to `C:\\Program Files\\Git\\usr\\bin;C:\\Program\nFiles\\Git\\hello` or something like that. But `kernel.org:/home/gitster` is\nnot, because it looks more like an SSH path. Similarly, `C:/Program Files`\nis interpreted as a Windows-style path, even if it could technically be a\nUnix-style path list.\n\nNow, if you call `git.exe -C /blabla `, it works, because\n`git.exe` is a non-MSYS program, therefore that `/blabla` is converted to\na Windows-style path before executing `git.exe`. However, when you write a\nfile via `echo /blabla >file`, that `echo` is either the Bash built-in, or\nit is an MSYS program, and no argument conversion takes place. If you\n_then_ ask `git.exe` to read and interpret the file as a path, it won't\nknow what to do with that Unix-style path.\n\nYou can substitute `$PWD` for `/blabla` in all of this, and it will hold\ntrue just the same.\n\nSo what makes `pwd` special?\n\nWell, `pwd.exe` itself is an MSYS program, so it would still report a path\nthat `git.exe` cannot understand. But in Git's test suite, we specifically\noverride `pwd` to be a shell function that calls `pwd.exe -W`, which does\noutput Windows-style paths.\n\nThe thing that makes that `GIT_*=$PWD git ...` call work is that the\nenvironment is automagically converted because `git` is a non-MSYS\nprogram. The thing that makes `echo $PWD >.git/objects/info/alternates`\nnot work is that `echo` _is_ an MSYS program (or Bash built-in, which is\nthe same thing here, for all practical purposes), so it writes the path\nverbatim into that file, but then we expect `git.exe` to read this file\nand interpret it as a list of paths.\n\nHopefully that clarifies the scenario a bit, even if it is far from a\nconcise explanation (I did edit this mail multiple times for clarity and\nbrevity, though, as I do with pretty much all of my mails).\n\nCiao,\nDscho\n\n> We have\n>\n> - When a test checks for an absolute path that a git command generated,\n> construct the expected value using $(pwd) rather than $PWD,\n> $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on\n> Windows, where the shell (MSYS bash) mangles absolute path names.\n> For details, see the commit message of 4114156ae9.\n>\n> in t/README, but even with the log mesasge of 4114156a (Tests on\n> Windows: $(pwd) must return Windows-style paths, 2009-03-13) [*1*],\n> I have no idea what makes the thing you found in t5615 work and your\n> suggestion to use $PWD in the new one not work.\n>\n> GIT_ALTERNATE_OBJECT_DIRECTORIES is a PATH_SEP (not necessarily a\n> colon) separated list, and I think the way t5615 uses it is broken\n> on Windows where PATH_SEP is defined as semicolon without the $PWD\n> vs $(pwd) issue. Is the test checking the right thing?\n>\n>\n> [Footnote]\n>\n> *1*\n>\n> Tests on Windows: $(pwd) must return Windows-style paths\n>\n> Many tests pass $(pwd) in some form to git and later test that the output\n> of git contains the correct value of $(pwd). For example, the test of\n> 'git remote show' sets up a remote that contains $(pwd) and then the\n> expected result must contain $(pwd).\n>\n> Again, MSYS-bash's path mangling kicks in: Plain $(pwd) uses the MSYS style\n> absolute path /c/path/to/git. The test case would write this name into\n> the 'expect' file. But when git is invoked, MSYS-bash converts this name to\n> the Windows style path c:/path/to/git, and git would produce this form in\n> the result; the test would fail.\n>\n> We fix this by passing -W to bash's pwd that produces the Windows-style\n> path.\n>\n> There are a two cases that need an accompanying change:\n>\n> - In t1504 the value of $(pwd) becomes part of a path list. In this case,\n> the lone 'c' in something like /foo:c:/path/to/git:/bar inhibits\n> MSYS-bashes path mangling; IOW in this case we want the /c/path/to/git\n> form to allow path mangling. We use $PWD instead of $(pwd), which always\n> has the latter form.\n>\n> - In t6200, $(pwd) - the Windows style path - must be used to construct the\n> expected result because that is the path form that git sees. (The change\n> in the test itself is just for consistency: 'git fetch' always sees the\n> Windows-style path, with or without the change.)\n>\n> Signed-off-by: Johannes Sixt \n>\n>\n"},{"id":"458417","messageId":"xmqq1qv48ss7.fsf@gitster.g","threadId":"57995","inReplyTo":"n3p471no-671q-2701-1r72-s0q02ns09053@tzk.qr","subject":"Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-01T20:47:04Z","receivedAt":"2022-07-01T20:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Whenever you call a non-MSYS program from an MSYS program (and remember,\n> an MSYS program is a program that uses the MSYS2 runtime that acts as a\n> POSIX emulation layer), \"magic\" things are done. In our context,\n> `bash.exe` is an MSYS program, and the non-MSYS program that is called is\n> `git.exe`.\n>\n> So what are those \"magic\" things? The command-line arguments and the\n> environment variables are auto-converted: everything that looks like a\n> Unix-style path (or path list, like the `PATH` environment variable) is\n> converted to a Windows-style path or path list (on Windows, the colon\n> cannot be the separator in `PATH`, therefore the semicolon is used).\n>\n> And this is where it gets _really_ tricky to explain what is going on:\n> what _does_ look like a Unix-style path? The exact rules are convoluted\n> and hard to explain, but they work _most of the time_. For example,\n> `/usr/bin:/hello` is converted to `C:\\Program Files\\Git\\usr\\bin;C:\\Program\n> Files\\Git\\hello` or something like that. But `kernel.org:/home/gitster` is\n> not, because it looks more like an SSH path. Similarly, `C:/Program Files`\n> is interpreted as a Windows-style path, even if it could technically be a\n> Unix-style path list.\n>\n> Now, if you call `git.exe -C /blabla `, it works, because\n> `git.exe` is a non-MSYS program, therefore that `/blabla` is converted to\n> a Windows-style path before executing `git.exe`. However, when you write a\n> file via `echo /blabla >file`, that `echo` is either the Bash built-in, or\n> it is an MSYS program, and no argument conversion takes place. If you\n> _then_ ask `git.exe` to read and interpret the file as a path, it won't\n> know what to do with that Unix-style path.\n>\n> You can substitute `$PWD` for `/blabla` in all of this, and it will hold\n> true just the same.\n>\n> So what makes `pwd` special?\n>\n> Well, `pwd.exe` itself is an MSYS program, so it would still report a path\n> that `git.exe` cannot understand. But in Git's test suite, we specifically\n> override `pwd` to be a shell function that calls `pwd.exe -W`, which does\n> output Windows-style paths.\n>\n> The thing that makes that `GIT_*=$PWD git ...` call work is that the\n> environment is automagically converted because `git` is a non-MSYS\n> program. The thing that makes `echo $PWD >.git/objects/info/alternates`\n> not work is that `echo` _is_ an MSYS program (or Bash built-in, which is\n> the same thing here, for all practical purposes), so it writes the path\n> verbatim into that file, but then we expect `git.exe` to read this file\n> and interpret it as a list of paths.\n\n----- 8< --------- 8< --------- 8< --------- 8< --------- 8< -----\n\n> Hopefully that clarifies the scenario a bit, even if it is far from a\n> concise explanation (I did edit this mail multiple times for clarity and\n> brevity, though, as I do with pretty much all of my mails).\n\nCertainly it does help. Thanks.\n\nI wonder if it makes sense to keep a copy of the bulk of your\nresponse in t/ somewhere, and refer to it from t/README, to help\nfellow non-Windows developers to avoid breaking tests on Windows\nwithout knowing.\n"},{"id":"458682","messageId":"165736941632.704481.18414237954289110814.git@grubix.eu","threadId":"57995","inReplyTo":"96d4bb71505d87ed501c058bbd89bfc13d08b24a.1656593279.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-07-09T12:23:36Z","receivedAt":"2022-07-09T12:23:54Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Han Xin venit, vidit, dixit 2022-07-01 03:34:30:\n> The commit-graph is used to opportunistically optimize accesses to\n> certain pieces of information on commit objects, and\n> lookup_commit_in_graph() tries to say \"no\" when the requested commit\n> does not locally exist by returning NULL, in which case the caller\n> can ask for (which may result in on-demand fetching from a promisor\n> remote) and parse the commit object itself.\n> \n> However, it uses a wrong helper, repo_has_object_file(), to do so.\n> This helper not only checks if an object is mmediately available in\n> the local object store, but also tries to fetch from a promisor remote.\n> But the fetch machinery calls lookup_commit_in_graph(), thus causing an\n> infinite loop.\n> \n> We should make lookup_commit_in_graph() expect that a commit given to it\n> can be legitimately missing from the local object store, by using the\n> has_object_file() helper instead.\n> \n> Signed-off-by: Han Xin \n> ---\n> commit-graph.c | 2 +-\n> t/t5330-no-lazy-fetch-with-commit-graph.sh | 70 ++++++++++++++++++++++\n> 2 files changed, 71 insertions(+), 1 deletion(-)\n> create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 92d4503336..2b04ef072d 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -898,7 +898,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n> return NULL;\n> if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> return NULL;\n> - if (!repo_has_object_file(repo, id))\n> + if (!has_object(repo, id, 0))\n> return NULL;\n> \n> commit = lookup_commit(repo, id);\n> diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> new file mode 100755\n> index 0000000000..be33334229\n> --- /dev/null\n> +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> @@ -0,0 +1,70 @@\n> +#!/bin/sh\n> +\n> +test_description='test for no lazy fetch with the commit-graph'\n> +\n> +. ./test-lib.sh\n> +\n> +run_with_limited_processses () {\n> + # bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n> + if test -n \"$BASH_VERSION\"\n> + then\n> + ulimit_max_process=\"-u\"\n> + elif test -n \"$KSH_VERSION\"\n> + then\n> + ulimit_max_process=\"-u\"\n> + fi\n> + (ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n> +}\n\nThis new test fails for me unless I increase max_processes. 1024 works.\n\nI haven't bisected the number of prcesses ... This is higly system\ndependent. I even run a slim environment (i3wm) but having chrome or\nsuch running probably makes quite a difference.\n\n512 is probably OK in CI in an isolated environment but is too low on a\ntypical \"What you mean I'm not working? I'm waiting for the test run!\"\ndevelopper workstation.\n\nConversely, which number would be too high to catch what the test is\nsupposed to catch? Does it incur a big performance penalty to go as high\nas possible?\n\n> +\n> +test_lazy_prereq ULIMIT_PROCESSES '\n> + run_with_limited_processses true\n> +'\n> +\n> +if ! test_have_prereq ULIMIT_PROCESSES\n> +then\n> + skip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n> + test_done\n> +fi\n> +\n> +test_expect_success 'setup: prepare a repository with a commit' '\n> + git init with-commit &&\n> + test_commit -C with-commit the-commit &&\n> + oid=$(git -C with-commit rev-parse HEAD)\n> +'\n> +\n> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> + git init with-commit-graph &&\n> + echo \"$(pwd)/with-commit/.git/objects\" \\\n> + >with-commit-graph/.git/objects/info/alternates &&\n> + # create a ref that points to the commit in alternates\n> + git -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n> + # prepare some other objects to commit-graph\n> + test_commit -C with-commit-graph something &&\n> + git -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n> + test_path_is_file with-commit-graph/.git/objects/info/commit-graph\n> +'\n> +\n> +test_expect_success 'setup: change the alternates to what without the commit' '\n> + git init --bare without-commit &&\n> + git -C with-commit-graph cat-file -e $oid &&\n> + echo \"$(pwd)/without-commit/objects\" \\\n> + >with-commit-graph/.git/objects/info/alternates &&\n> + test_must_fail git -C with-commit-graph cat-file -e $oid\n> +'\n> +\n> +test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n> + # setup promisor and prepare any commit to fetch\n> + git -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n> + git -C with-commit-graph config remote.origin.promisor true &&\n> + git -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n> + test_commit -C with-commit any-commit &&\n> + anycommit=$(git -C with-commit rev-parse HEAD) &&\n> +\n> + run_with_limited_processses env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n> + git -C with-commit-graph fetch origin $anycommit 2>err &&\n\nThat empty line abobe makes me nervous, especially when a test fails for\nvery unclear reasons like here. Is it necessary?\n\nIf the answer is \"to separate setup and test\" then the solution is to\nseparate setup and test ...\n\n> + ! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n> + grep \"git fetch origin\" trace.txt >actual &&\n> + test_line_count = 1 actual\n> +'\n> +\n> +test_done\n> -- \n> 2.36.1\n> \n>\n"},{"id":"458798","messageId":"Ysw9LmBFGbRy9L7c@coredump.intra.peff.net","threadId":"57995","inReplyTo":"165736941632.704481.18414237954289110814.git@grubix.eu","subject":"Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-11T15:09:34Z","receivedAt":"2022-07-11T15:09:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 09, 2022 at 02:23:36PM +0200, Michael J Gruber wrote:\n\n> > +run_with_limited_processses () {\n> > + # bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n> > + if test -n \"$BASH_VERSION\"\n> > + then\n> > + ulimit_max_process=\"-u\"\n> > + elif test -n \"$KSH_VERSION\"\n> > + then\n> > + ulimit_max_process=\"-u\"\n> > + fi\n> > + (ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n> > +}\n> \n> This new test fails for me unless I increase max_processes. 1024 works.\n> \n> I haven't bisected the number of prcesses ... This is higly system\n> dependent. I even run a slim environment (i3wm) but having chrome or\n> such running probably makes quite a difference.\n> \n> 512 is probably OK in CI in an isolated environment but is too low on a\n> typical \"What you mean I'm not working? I'm waiting for the test run!\"\n> developper workstation.\n> \n> Conversely, which number would be too high to catch what the test is\n> supposed to catch? Does it incur a big performance penalty to go as high\n> as possible?\n\nThis bit me, too. It works if I run it standalone:\n\n $ ./t5330-no-lazy-fetch-with-commit-graph.sh \n ok 1 - setup: prepare a repository with a commit\n ok 2 - setup: prepare a repository with commit-graph contains the commit\n ok 3 - setup: change the alternates to what without the commit\n ok 4 - fetch any commit from promisor with the usage of the commit graph\n # passed all 4 test(s)\n\nbut it fails when I run the whole test suite with \"prove -j32\". Or even\neasier, just run it under \"--stress\":\n\n $ ./t5330-no-lazy-fetch-with-commit-graph.sh --stress\n [...]\n + run_with_limited_processses env GIT_TRACE=/home/peff/compile/git/t/trash directory.t5330-no-lazy-fetch-with-commit-graph.stress-30/trace.txt git -C with-commit-graph fetch origin ba19607defe740988a69e98bced331083e02bdd6\nerror: last command exited with $?=128\nnot ok 4 - fetch any commit from promisor with the usage of the commit graph\n\n $ cat trash*.stress-failed/err\n [...]\n error: cannot fork() for index-pack: Resource temporarily unavailable\n fatal: fetch-pack: unable to fork off index-pack\n\n-Peff\n"},{"id":"458817","messageId":"xmqqk08jo147.fsf@gitster.g","threadId":"57995","inReplyTo":"Ysw9LmBFGbRy9L7c@coredump.intra.peff.net","subject":"Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-11T20:17:28Z","receivedAt":"2022-07-11T20:17:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> 512 is probably OK in CI in an isolated environment but is too low on a\n>> typical \"What you mean I'm not working? I'm waiting for the test run!\"\n>> developper workstation.\n>> \n>> Conversely, which number would be too high to catch what the test is\n>> supposed to catch? Does it incur a big performance penalty to go as high\n>> as possible?\n>\n> This bit me, too. It works if I run it standalone:\n>\n> $ ./t5330-no-lazy-fetch-with-commit-graph.sh \n> ok 1 - setup: prepare a repository with a commit\n> ok 2 - setup: prepare a repository with commit-graph contains the commit\n> ok 3 - setup: change the alternates to what without the commit\n> ok 4 - fetch any commit from promisor with the usage of the commit graph\n> # passed all 4 test(s)\n>\n> but it fails when I run the whole test suite with \"prove -j32\". Or even\n> easier, just run it under \"--stress\":\n\nUnderstandable. I am usually on a datacentre VM without graphical\nUI so the process count there is much lower than on a typical\ndeveloper workstation.\n\nI wonder if we can just run the test without any limit? If in an\nunattended CI situation, hopefully they will kick the job out due to\nquota, and on a developer workstation, there may be processes killed\nleft and right, but that is only when the \"infinite respawning\" bug\nreappears.\n\n"},{"id":"458843","messageId":"CAKgqsWVD2108f0PyJGp6mVKp2cGd_V_MiiQO3SAPm+LEHcb2mA@mail.gmail.com","threadId":"57995","inReplyTo":"xmqqk08jo147.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-12T01:52:42Z","receivedAt":"2022-07-12T01:52:57Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Tue, Jul 12, 2022 at 4:17 AM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> >> 512 is probably OK in CI in an isolated environment but is too low on a\n> >> typical \"What you mean I'm not working? I'm waiting for the test run!\"\n> >> developper workstation.\n> >>\n> >> Conversely, which number would be too high to catch what the test is\n> >> supposed to catch? Does it incur a big performance penalty to go as high\n> >> as possible?\n> >\n> > This bit me, too. It works if I run it standalone:\n> >\n> > $ ./t5330-no-lazy-fetch-with-commit-graph.sh\n> > ok 1 - setup: prepare a repository with a commit\n> > ok 2 - setup: prepare a repository with commit-graph contains the commit\n> > ok 3 - setup: change the alternates to what without the commit\n> > ok 4 - fetch any commit from promisor with the usage of the commit graph\n> > # passed all 4 test(s)\n> >\n> > but it fails when I run the whole test suite with \"prove -j32\". Or even\n> > easier, just run it under \"--stress\":\n>\n> Understandable. I am usually on a datacentre VM without graphical\n> UI so the process count there is much lower than on a typical\n> developer workstation.\n>\n> I wonder if we can just run the test without any limit? If in an\n> unattended CI situation, hopefully they will kick the job out due to\n> quota, and on a developer workstation, there may be processes killed\n> left and right, but that is only when the \"infinite respawning\" bug\n> reappears.\n>\n\nThe tricky thing about using ulimit is that it's tied to the entire development\nstation. I have tried to run the test without any limit [1], it did finally be\ncanceled after 6 hours.\n\nRemove the \"ulimit\", once the infinite loop reappears, this test seems like a\nbomb to developers that quickly consumes all resources? While \"1024\"\nworks fine with \"--stress\" , there are reasons to wonder if it's enough, or\nmaybe we can take a value like 10240 that we wouldn't normally reach?\n\n1. https://github.com/chiyutianyi/git/runs/6962635320\n\nThanks\n-Han Xin\n"},{"id":"458844","messageId":"xmqq1quqkiq2.fsf@gitster.g","threadId":"57995","inReplyTo":"CAKgqsWVD2108f0PyJGp6mVKp2cGd_V_MiiQO3SAPm+LEHcb2mA@mail.gmail.com","subject":"Re: [External] Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-12T05:23:01Z","receivedAt":"2022-07-12T05:23:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Xin writes:\n\n>> I wonder if we can just run the test without any limit? If in an\n>> unattended CI situation, hopefully they will kick the job out due to\n>> quota, and on a developer workstation, there may be processes killed\n>> left and right, but that is only when the \"infinite respawning\" bug\n>> reappears.\n>>\n>\n> The tricky thing about using ulimit is that it's tied to the entire development\n> station. I have tried to run the test without any limit [1], it did finally be\n> canceled after 6 hours.\n\nI am not worried so much about developer workstation, which people\nare sitting in front of. They can ^C any runaway test way before 6\nhours just fine.\n\nI am assuming that we do not have to be worried about CI settings\ntoo much, either, as they should already be prepared to catch\nrun-away processes.\n"},{"id":"458845","messageId":"CAKgqsWW+OECFnEy+uib1W6UzB1Sy_MQ5towDwH28fg=ni1v_0Q@mail.gmail.com","threadId":"57995","inReplyTo":"xmqq1quqkiq2.fsf@gitster.g","subject":"Re: Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-12T05:32:43Z","receivedAt":"2022-07-12T05:33:01Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Tue, Jul 12, 2022 at 1:23 PM Junio C Hamano wrote:\n>\n> Han Xin writes:\n>\n> >> I wonder if we can just run the test without any limit? If in an\n> >> unattended CI situation, hopefully they will kick the job out due to\n> >> quota, and on a developer workstation, there may be processes killed\n> >> left and right, but that is only when the \"infinite respawning\" bug\n> >> reappears.\n> >>\n> >\n> > The tricky thing about using ulimit is that it's tied to the entire development\n> > station. I have tried to run the test without any limit [1], it did finally be\n> > canceled after 6 hours.\n>\n> I am not worried so much about developer workstation, which people\n> are sitting in front of. They can ^C any runaway test way before 6\n> hours just fine.\n>\n> I am assuming that we do not have to be worried about CI settings\n> too much, either, as they should already be prepared to catch\n> run-away processes.\n\nOK, let's remove the \"ulimit\" and leave it to the followup checks.\n\nThanks.\n-Han Xin\n"},{"id":"458848","messageId":"Ys0WlWFIuhP8b2hb@coredump.intra.peff.net","threadId":"57995","inReplyTo":"xmqq1quqkiq2.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-12T06:37:09Z","receivedAt":"2022-07-12T06:37:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 11, 2022 at 10:23:01PM -0700, Junio C Hamano wrote:\n\n> > The tricky thing about using ulimit is that it's tied to the entire development\n> > station. I have tried to run the test without any limit [1], it did finally be\n> > canceled after 6 hours.\n> \n> I am not worried so much about developer workstation, which people\n> are sitting in front of. They can ^C any runaway test way before 6\n> hours just fine.\n> \n> I am assuming that we do not have to be worried about CI settings\n> too much, either, as they should already be prepared to catch\n> run-away processes.\n\nAgreed. Also, I think that although it's natural to worry about a bug we\nknow about causing an infinite loop, it's much more likely that a _new_\nbug will cause one. I.e., every test we already carry is a candidate to\naccidentally loop forever in this way. This is just the one we happen to\nhave seen. Once fixed, I don't know that it's at any more risk of\nreocurring than any other problem.\n\n-Peff\n"},{"id":"458849","messageId":"cover.1657604799.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656593279.git.hanxin.hx@bytedance.com","subject":"[PATCH v5 0/1] no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-12T06:50:46Z","receivedAt":"2022-07-12T06:51:07Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"This patch fixes the following issue:\nWhen we found the commit in the graph in lookup_commit_in_graph(), \nbut the commit is missing from the repository, we will try\npromisor_remote_get_direct() and then enter another loop.\n\nThen we will go into an endless loop:\n git fetch -> deref_without_lazy_fetch() ->\n lookup_commit_in_graph() -> repo_has_object_file() ->\n promisor_remote_get_direct() -> fetch_objects() ->\n git fetch (a new loop round)\n\nChanges since v4:\n\n* Remove run_with_limited_processses() as it can be catched by CI\n settings and developer workstation. Keeping it will make a trouble\n when there are too many prcesses or stress is used.\n\nHan Xin (1):\n commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n\n commit-graph.c | 2 +-\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 1 deletion(-)\n create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n\nRange-diff against v4:\n1: 96d4bb7150 ! 1: 3ffeed50de commit-graph.c: no lazy fetch in lookup_commit_in_graph()\n @@ t/t5330-no-lazy-fetch-with-commit-graph.sh (new)\n +\n +. ./test-lib.sh\n +\n -+run_with_limited_processses () {\n -+\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n -+\tif test -n \"$BASH_VERSION\"\n -+\tthen\n -+\t\tulimit_max_process=\"-u\"\n -+\telif test -n \"$KSH_VERSION\"\n -+\tthen\n -+\t\tulimit_max_process=\"-u\"\n -+\tfi\n -+\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n -+}\n -+\n -+test_lazy_prereq ULIMIT_PROCESSES '\n -+\trun_with_limited_processses true\n -+'\n -+\n -+if ! test_have_prereq ULIMIT_PROCESSES\n -+then\n -+\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n -+\ttest_done\n -+fi\n -+\n +test_expect_success 'setup: prepare a repository with a commit' '\n +\tgit init with-commit &&\n +\ttest_commit -C with-commit the-commit &&\n @@ t/t5330-no-lazy-fetch-with-commit-graph.sh (new)\n +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n +\ttest_commit -C with-commit any-commit &&\n +\tanycommit=$(git -C with-commit rev-parse HEAD) &&\n -+\n -+\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n ++\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n +\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n +\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n +\tgrep \"git fetch origin\" trace.txt >actual &&\n-- \n2.36.1\n\n"},{"id":"458850","messageId":"3ffeed50deb2d292cef0a518085d60d22c5dd79b.1657604799.git.hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1657604799.git.hanxin.hx@bytedance.com","subject":"[PATCH v5 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-12T06:50:47Z","receivedAt":"2022-07-12T06:51:09Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"The commit-graph is used to opportunistically optimize accesses to\ncertain pieces of information on commit objects, and\nlookup_commit_in_graph() tries to say \"no\" when the requested commit\ndoes not locally exist by returning NULL, in which case the caller\ncan ask for (which may result in on-demand fetching from a promisor\nremote) and parse the commit object itself.\n\nHowever, it uses a wrong helper, repo_has_object_file(), to do so.\nThis helper not only checks if an object is mmediately available in\nthe local object store, but also tries to fetch from a promisor remote.\nBut the fetch machinery calls lookup_commit_in_graph(), thus causing an\ninfinite loop.\n\nWe should make lookup_commit_in_graph() expect that a commit given to it\ncan be legitimately missing from the local object store, by using the\nhas_object_file() helper instead.\n\nSigned-off-by: Han Xin \n---\n commit-graph.c | 2 +-\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 1 deletion(-)\n create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d4503336..2b04ef072d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -898,7 +898,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n \t\treturn NULL;\n \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n \t\treturn NULL;\n-\tif (!repo_has_object_file(repo, id))\n+\tif (!has_object(repo, id, 0))\n \t\treturn NULL;\n \n \tcommit = lookup_commit(repo, id);\ndiff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\nnew file mode 100755\nindex 0000000000..2cc7fd7a47\n--- /dev/null\n+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n@@ -0,0 +1,47 @@\n+#!/bin/sh\n+\n+test_description='test for no lazy fetch with the commit-graph'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: prepare a repository with a commit' '\n+\tgit init with-commit &&\n+\ttest_commit -C with-commit the-commit &&\n+\toid=$(git -C with-commit rev-parse HEAD)\n+'\n+\n+test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n+\tgit init with-commit-graph &&\n+\techo \"$(pwd)/with-commit/.git/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\t# create a ref that points to the commit in alternates\n+\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n+\t# prepare some other objects to commit-graph\n+\ttest_commit -C with-commit-graph something &&\n+\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n+\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n+'\n+\n+test_expect_success 'setup: change the alternates to what without the commit' '\n+\tgit init --bare without-commit &&\n+\tgit -C with-commit-graph cat-file -e $oid &&\n+\techo \"$(pwd)/without-commit/objects\" \\\n+\t\t>with-commit-graph/.git/objects/info/alternates &&\n+\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n+'\n+\n+test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n+\t# setup promisor and prepare any commit to fetch\n+\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n+\tgit -C with-commit-graph config remote.origin.promisor true &&\n+\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n+\ttest_commit -C with-commit any-commit &&\n+\tanycommit=$(git -C with-commit rev-parse HEAD) &&\n+\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n+\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n+\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n+\tgrep \"git fetch origin\" trace.txt >actual &&\n+\ttest_line_count = 1 actual\n+'\n+\n+test_done\n-- \n2.36.1\n\n"},{"id":"458854","messageId":"Ys0bmytqz9nra+AB@coredump.intra.peff.net","threadId":"57995","inReplyTo":"cover.1657604799.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v5 0/1] no lazy fetch in lookup_commit_in_graph()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-12T06:58:35Z","receivedAt":"2022-07-12T06:58:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 12, 2022 at 02:50:46PM +0800, Han Xin wrote:\n\n> Changes since v4:\n> \n> * Remove run_with_limited_processses() as it can be catched by CI\n> settings and developer workstation. Keeping it will make a trouble\n> when there are too many prcesses or stress is used.\n\nYour v4 is already in 'next', so I think rather than replacing the\npatch, we'd want a commit on top to remove run_with_limited_processes().\n\n-Peff\n"},{"id":"458863","messageId":"20220712080143.11843-1-hanxin.hx@bytedance.com","threadId":"57995","inReplyTo":"cover.1656593279.git.hanxin.hx@bytedance.com","subject":"[PATCH v1] t5330: remove run_with_limited_processses()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-12T08:01:43Z","receivedAt":"2022-07-12T08:02:01Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"run_with_limited_processses() is used to end the loop faster when an\ninfinite loop happen. But \"ulimit\" is tied to the entire development\nstation, and the test will fail due to too many other processes or using\n\"--stress\".\n\nWithout run_with_limited_processses() the infinite loop can also be\nstopped due to global configrations or quotas, and the verification\nstill works fine. So let's remove run_with_limited_processses().\n\nSigned-off-by: Han Xin \n---\n t/t5330-no-lazy-fetch-with-commit-graph.sh | 25 +---------------------\n 1 file changed, 1 insertion(+), 24 deletions(-)\n\ndiff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\nindex be33334229..2cc7fd7a47 100755\n--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh\n+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n@@ -4,28 +4,6 @@ test_description='test for no lazy fetch with the commit-graph'\n \n . ./test-lib.sh\n \n-run_with_limited_processses () {\n-\t# bash and ksh use \"ulimit -u\", dash uses \"ulimit -p\"\n-\tif test -n \"$BASH_VERSION\"\n-\tthen\n-\t\tulimit_max_process=\"-u\"\n-\telif test -n \"$KSH_VERSION\"\n-\tthen\n-\t\tulimit_max_process=\"-u\"\n-\tfi\n-\t(ulimit ${ulimit_max_process-\"-p\"} 512 && \"$@\")\n-}\n-\n-test_lazy_prereq ULIMIT_PROCESSES '\n-\trun_with_limited_processses true\n-'\n-\n-if ! test_have_prereq ULIMIT_PROCESSES\n-then\n-\tskip_all='skipping tests for no lazy fetch with the commit-graph, ulimit processes not available'\n-\ttest_done\n-fi\n-\n test_expect_success 'setup: prepare a repository with a commit' '\n \tgit init with-commit &&\n \ttest_commit -C with-commit the-commit &&\n@@ -59,8 +37,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit\n \tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n \ttest_commit -C with-commit any-commit &&\n \tanycommit=$(git -C with-commit rev-parse HEAD) &&\n-\n-\trun_with_limited_processses env GIT_TRACE=\"$(pwd)/trace.txt\" \\\n+\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n \t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n \t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n \tgrep \"git fetch origin\" trace.txt >actual &&\n-- \n2.36.1\n\n"},{"id":"458868","messageId":"220712.86zghe4q0c.gmgdl@evledraar.gmail.com","threadId":"57995","inReplyTo":"3ffeed50deb2d292cef0a518085d60d22c5dd79b.1657604799.git.hanxin.hx@bytedance.com","subject":"Re: [PATCH v5 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-07-12T09:50:49Z","receivedAt":"2022-07-12T09:52:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 12 2022, Han Xin wrote:\n\n> The commit-graph is used to opportunistically optimize accesses to\n> certain pieces of information on commit objects, and\n> lookup_commit_in_graph() tries to say \"no\" when the requested commit\n> does not locally exist by returning NULL, in which case the caller\n> can ask for (which may result in on-demand fetching from a promisor\n> remote) and parse the commit object itself.\n>\n> However, it uses a wrong helper, repo_has_object_file(), to do so.\n> This helper not only checks if an object is mmediately available in\n> the local object store, but also tries to fetch from a promisor remote.\n> But the fetch machinery calls lookup_commit_in_graph(), thus causing an\n> infinite loop.\n>\n> We should make lookup_commit_in_graph() expect that a commit given to it\n> can be legitimately missing from the local object store, by using the\n> has_object_file() helper instead.\n>\n> Signed-off-by: Han Xin \n> ---\n> commit-graph.c | 2 +-\n> t/t5330-no-lazy-fetch-with-commit-graph.sh | 47 ++++++++++++++++++++++\n> 2 files changed, 48 insertions(+), 1 deletion(-)\n> create mode 100755 t/t5330-no-lazy-fetch-with-commit-graph.sh\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 92d4503336..2b04ef072d 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -898,7 +898,7 @@ struct commit *lookup_commit_in_graph(struct repository *repo, const struct obje\n> \t\treturn NULL;\n> \tif (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))\n> \t\treturn NULL;\n> -\tif (!repo_has_object_file(repo, id))\n> +\tif (!has_object(repo, id, 0))\n> \t\treturn NULL;\n> \n> \tcommit = lookup_commit(repo, id);\n> diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> new file mode 100755\n> index 0000000000..2cc7fd7a47\n> --- /dev/null\n> +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh\n> @@ -0,0 +1,47 @@\n> +#!/bin/sh\n> +\n> +test_description='test for no lazy fetch with the commit-graph'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup: prepare a repository with a commit' '\n> +\tgit init with-commit &&\n> +\ttest_commit -C with-commit the-commit &&\n> +\toid=$(git -C with-commit rev-parse HEAD)\n> +'\n> +\n> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '\n> +\tgit init with-commit-graph &&\n> +\techo \"$(pwd)/with-commit/.git/objects\" \\\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> +\t# create a ref that points to the commit in alternates\n> +\tgit -C with-commit-graph update-ref refs/ref_to_the_commit \"$oid\" &&\n> +\t# prepare some other objects to commit-graph\n> +\ttest_commit -C with-commit-graph something &&\n> +\tgit -c gc.writeCommitGraph=true -C with-commit-graph gc &&\n> +\ttest_path_is_file with-commit-graph/.git/objects/info/commit-graph\n> +'\n> +\n> +test_expect_success 'setup: change the alternates to what without the commit' '\n> +\tgit init --bare without-commit &&\n> +\tgit -C with-commit-graph cat-file -e $oid &&\n> +\techo \"$(pwd)/without-commit/objects\" \\\n> +\t\t>with-commit-graph/.git/objects/info/alternates &&\n> +\ttest_must_fail git -C with-commit-graph cat-file -e $oid\n> +'\n> +\n> +test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n> +\t# setup promisor and prepare any commit to fetch\n> +\tgit -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n> +\tgit -C with-commit-graph config remote.origin.promisor true &&\n> +\tgit -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n> +\ttest_commit -C with-commit any-commit &&\n> +\tanycommit=$(git -C with-commit rev-parse HEAD) &&\n> +\tGIT_TRACE=\"$(pwd)/trace.txt\" \\\n> +\t\tgit -C with-commit-graph fetch origin $anycommit 2>err &&\n> +\t! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n\nThis part seems quite odd, we tested the exit code, so here we're being\nparanoid about not getting a specific \"fatal\" error message.\n\nIt seems more worthwhile to test the warnings we emit, which in this\ncase seem to be duplicated (but that's probably an existing issue...).\n\n\n> +\tgrep \"git fetch origin\" trace.txt >actual &&\n> +\ttest_line_count = 1 actual\n> +'\n\nI wondered if \"test_subcomand\" here would be better, i.e. fewer things\nscraping GIT_TRACE, and using the machine-readable GIT_TRACE2_EVENT\ninstead...\n"},{"id":"458889","messageId":"xmqqsfn6ifbu.fsf@gitster.g","threadId":"57995","inReplyTo":"Ys0WlWFIuhP8b2hb@coredump.intra.peff.net","subject":"Re: [External] Re: [PATCH v4 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-12T14:19:17Z","receivedAt":"2022-07-12T14:19:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... it's much more likely that a _new_\n> bug will cause one. I.e., every test we already carry is a candidate to\n> accidentally loop forever in this way. This is just the one we happen to\n> have seen. Once fixed, I don't know that it's at any more risk of\n> reocurring than any other problem.\n\nThanks. \n\nThis kind of perspective is why I love to have you on the list.\nOnce said, it is so obvious but somehow I (or other people) failed\nto phrase it so clearly.\n\nVery much appreciated.\n"},{"id":"458932","messageId":"CAKgqsWWYGVh_ViPjEn8ezUMysGnxqu9xMkydT+vbuDE_GSWz_w@mail.gmail.com","threadId":"57995","inReplyTo":"220712.86zghe4q0c.gmgdl@evledraar.gmail.com","subject":"Re: Re: [PATCH v5 1/1] commit-graph.c: no lazy fetch in lookup_commit_in_graph()","fromName":"Han Xin","fromEmail":"hanxin.hx@bytedance.com","sentAt":"2022-07-13T01:26:50Z","receivedAt":"2022-07-13T01:27:06Z","isPatch":true,"sender":{"key":"hanxin.hx@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/16610542?v=4"},"body":"On Tue, Jul 12, 2022 at 5:52 PM Ævar Arnfjörð Bjarmason\n wrote:\n> > +test_expect_success 'fetch any commit from promisor with the usage of the commit graph' '\n> > + # setup promisor and prepare any commit to fetch\n> > + git -C with-commit-graph remote add origin \"$(pwd)/with-commit\" &&\n> > + git -C with-commit-graph config remote.origin.promisor true &&\n> > + git -C with-commit-graph config remote.origin.partialclonefilter blob:none &&\n> > + test_commit -C with-commit any-commit &&\n> > + anycommit=$(git -C with-commit rev-parse HEAD) &&\n> > + GIT_TRACE=\"$(pwd)/trace.txt\" \\\n> > + git -C with-commit-graph fetch origin $anycommit 2>err &&\n> > + ! grep \"fatal: promisor-remote: unable to fork off fetch subprocess\" err &&\n>\n> This part seems quite odd, we tested the exit code, so here we're being\n> paranoid about not getting a specific \"fatal\" error message.\n>\n> It seems more worthwhile to test the warnings we emit, which in this\n> case seem to be duplicated (but that's probably an existing issue...).\n>\n\nSo you mean the grep here is redundant?\n\n>\n> > + grep \"git fetch origin\" trace.txt >actual &&\n> > + test_line_count = 1 actual\n> > +'\n>\n> I wondered if \"test_subcomand\" here would be better, i.e. fewer things\n> scraping GIT_TRACE, and using the machine-readable GIT_TRACE2_EVENT\n> instead...\n\nI threw this question up front but got no response.\n\nWhen using test_subcommand() we should give all the args,\nif we remove or add any args later, this test case will always\npass even without this fix. So, is this test case still strict?\n\n GIT_TRACE2_EVENT=\"$(PWD)/trace.txt\" \\\n git -C with-commit-graph fetch origin $anycommit &&\n test_subcommand ! git -c fetch.negotiationAlgorithm=noop \\\n fetch origin --no-tags --no-write-fetch-head \\\n --recurse-submodules=no --filter=blob:none \\\n --stdin