{"thread":{"id":"59375","subject":"[PATCH v2] index-pack: remove fetch_if_missing=0","startedAt":"2023-03-10T18:22:25Z","lastAt":"2023-03-10T18:22:25Z","messageCount":1,"participants":["Kousik Sanagavarapu"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"473373","messageId":"20230310182043.19242-1-five231003@gmail.com","threadId":"59375","inReplyTo":"--in-reply-to=20230225052439.27096-1-five231003@gmail.com","subject":"[PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-10T18:20:43Z","receivedAt":"2023-03-10T18:22:25Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"A collision test is triggered in sha1_object(), whenever there is an\nobject file in our repo. If our repo is a partial clone, then checking\nfor this file existence does not lazy-fetch the object (if the object\nis missing and if there are one or more promisor remotes) when\nfetch_if_missing is set to 0.\n\nThis global was added as a temporary measure to suppress the fetching\nof missing objects and can be removed once the commandshave been taught\nto handle these cases.\n\nHence, use has_object() to check for the existence of an object, which\nhas the default behavior of not lazy-fetching in a partial clone. It is\nworth mentioning that this is the only place where there is potential for\nlazy-fetching and all other cases are properly handled, making it safe to\nremove this global here.\n\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n\nSorry for the late reroll, I was having semester-end exams.\n\nChanges since v1:\n- Changed the commit message to be more clear about the\n  change done here.\n\n- Changed the test according to the previous review.\n\n builtin/index-pack.c     | 11 +----------\n t/t5616-partial-clone.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 6648f2daef..8c0f36a49e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -800,8 +800,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \n \tif (startup_info->have_repository) {\n \t\tread_lock();\n-\t\tcollision_test_needed =\n-\t\t\thas_object_file_with_flags(oid, OBJECT_INFO_QUICK);\n+\t\tcollision_test_needed = has_object(the_repository, oid, 0);\n \t\tread_unlock();\n \t}\n \n@@ -1728,14 +1727,6 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tint report_end_of_input = 0;\n \tint hash_algo = 0;\n \n-\t/*\n-\t * index-pack never needs to fetch missing objects except when\n-\t * REF_DELTA bases are missing (which are explicitly handled). It only\n-\t * accesses the repo to do hash collision checks and to check which\n-\t * REF_DELTA bases need to be fetched.\n-\t */\n-\tfetch_if_missing = 0;\n-\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(index_pack_usage);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex f519d2a87a..46af8698ce 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -644,6 +644,41 @@ test_expect_success 'repack does not loosen promisor objects' '\n \tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n '\n \n+test_expect_success 'index-pack does not lazy-fetch when checking for sha1 collsions' '\n+\trm -rf server promisor-remote client repo trace &&\n+\n+\t# setup\n+\tgit init server &&\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\techo $i >server/file$i &&\n+\t\tgit -C server add file$i &&\n+\t\tgit -C server commit -am \"Commit $i\" || return 1\n+\tdone &&\n+\tgit -C server config --local uploadpack.allowFilter 1 &&\n+\tgit -C server config --local uploadpack.allowAnySha1InWant 1 &&\n+\tHASH=$(git -C server hash-object file3) &&\n+\n+\tgit init promisor-remote &&\n+\tgit -C promisor-remote fetch --keep \"file://$(pwd)/server\" &&\n+\n+\tgit clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tgit -C client remote set-url origin \"file://$(pwd)/promisor-remote\" &&\n+\tgit -C client config extensions.partialClone 1 &&\n+\tgit -C client config remote.origin.promisor 1 &&\n+\n+\tgit init repo &&\n+\techo \"5\" >repo/file5 &&\n+\tgit -C repo config --local uploadpack.allowFilter 1 &&\n+\tgit -C repo config --local uploadpack.allowAnySha1InWant 1 &&\n+\n+\t# verify that no lazy-fetching is done when fetching from another repo\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n+\t\t\t\t\tfetch --keep \"file://$(pwd)/repo\" main &&\n+\n+\t! grep \"want $HASH\" trace\n+'\n+\n test_expect_success 'lazy-fetch in submodule succeeds' '\n \t# setup\n \ttest_config_global protocol.file.allow always &&\n-- \n2.25.1\n\n"}]}