{"thread":{"id":"59307","subject":"[PATCH] index-pack: remove fetch_if_missing=0","startedAt":"2023-02-25T05:25:08Z","lastAt":"2023-03-19T06:17:10Z","messageCount":20,"participants":["Kousik Sanagavarapu","Jonathan Tan","Junio C Hamano","Sean Allred"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472713","messageId":"20230225052439.27096-1-five231003@gmail.com","threadId":"59307","inReplyTo":null,"subject":"[PATCH] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-25T05:24:39Z","receivedAt":"2023-02-25T05:25:08Z","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 has the behavior of lazy-fetching the object\nbecause we have one or more promisor remotes.\n\nThis behavior is controlled by setting fetch_if_missing to 0, but this\nglobal was added in the first place as a temporary measure to suppress\nthe fetching of missing objects and can be removed once the commands\nhave been taught to 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.\n\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n builtin/index-pack.c     | 11 +----------\n t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 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 037941b95d..4658ce0866 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -644,6 +644,34 @@ 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 collisions' '\n+\trm -rf server promisor-remote client &&\n+\trm -rf object-count &&\n+\n+\tgit init server &&\n+\tfor i in 1 2 3 4\n+\tdo\n+\t\techo $i >$(pwd)/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\" $HASH &&\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+\t# make sure that index-pack is run from within the repository\n+\tgit -C client index-pack $(pwd)/client/.git/objects/pack/*.pack &&\n+\ttest_path_is_missing $(pwd)/client/file3\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.25.1\n\n"},{"id":"472800","messageId":"20230227165646.6777-1-five231003@gmail.com","threadId":"59307","inReplyTo":"20230225052439.27096-1-five231003@gmail.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-27T16:56:45Z","receivedAt":"2023-02-27T16:57:04Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Waiting for review.\n\nThanks\n"},{"id":"472827","messageId":"20230227221451.2433306-1-jonathantanmy@google.com","threadId":"59307","inReplyTo":"20230225052439.27096-1-five231003@gmail.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-02-27T22:14:51Z","receivedAt":"2023-02-27T22:14:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n> A collision test is triggered in sha1_object(), whenever there is an\n> object file in our repo. If our repo is a partial clone, then checking\n> for this file existence has the behavior of lazy-fetching the object\n> because we have one or more promisor remotes.\n\nHmm...this is not true, because (as you said)...\n \n> This behavior is controlled by setting fetch_if_missing to 0,\n\n...this makes it so that we don't fetch in this situation.\n\n> but this\n> global was added in the first place as a temporary measure to suppress\n> the fetching of missing objects and can be removed once the commands\n> have been taught to handle these cases.\n\nYes, that's true.\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\nI think that the author of such a commit (you) should also independently\nverify that this comment is true (and if it is, then yes, all the\nremaining cases are handled and we can remove this assignment to\nfetch_if_missing). I believe this comment to be true, but I haven't\nchecked the code in a while so I'm not sure myself.\n\n> +test_expect_success 'index-pack does not lazy-fetch when checking for sha1 collisions' '\n> +\trm -rf server promisor-remote client &&\n> +\trm -rf object-count &&\n> +\n> +\tgit init server &&\n> +\tfor i in 1 2 3 4\n> +\tdo\n> +\t\techo $i >$(pwd)/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\" $HASH &&\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> +\t# make sure that index-pack is run from within the repository\n> +\tgit -C client index-pack $(pwd)/client/.git/objects/pack/*.pack &&\n> +\ttest_path_is_missing $(pwd)/client/file3\n> +'\n\nHow does this check that no lazy fetch has occurred? It seems to me\nthat you're just checking the existence of a file in the worktree,\nwhich does not indicate the presence or absence of a lazy fetch.\n\nI think the way to test needs to be more complicated: you need\nto create a partial clone, fetch into it from another repo, and\nthen verify that no fetches were made to the original partial\nclone.\n\n"},{"id":"472839","messageId":"20230228035448.10700-1-five231003@gmail.com","threadId":"59307","inReplyTo":"20230227221451.2433306-1-jonathantanmy@google.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-28T03:54:48Z","receivedAt":"2023-02-28T03:55:20Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Tue, 28 Feb 2023 at 03:44, Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> Kousik Sanagavarapu <five231003@gmail.com> writes:\n> > A collision test is triggered in sha1_object(), whenever there is an\n> > object file in our repo. If our repo is a partial clone, then checking\n> > for this file existence has the behavior of lazy-fetching the object\n> > because we have one or more promisor remotes.\n>\n> Hmm...this is not true, because (as you said)...\n>\n> > This behavior is controlled by setting fetch_if_missing to 0,\n>\n> ...this makes it so that we don't fetch in this situation.\n\nYes, that statement is false if fetch_if_missing is set to 0. But my original\nthought in writing it was so that the anyone who is reading the commit message\nunderstands the motivation as to why we are setting fetch_if_missing to 0.\n\n> [...]\n>\n> > @@ -1728,14 +1727,6 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n> >       int report_end_of_input = 0;\n> >       int hash_algo = 0;\n> >\n> > -     /*\n> > -      * index-pack never needs to fetch missing objects except when\n> > -      * REF_DELTA bases are missing (which are explicitly handled). It only\n> > -      * accesses the repo to do hash collision checks and to check which\n> > -      * REF_DELTA bases need to be fetched.\n> > -      */\n> > -     fetch_if_missing = 0;\n>\n> I think that the author of such a commit (you) should also independently\n> verify that this comment is true (and if it is, then yes, all the\n> remaining cases are handled and we can remove this assignment to\n> fetch_if_missing). I believe this comment to be true, but I haven't\n> checked the code in a while so I'm not sure myself.\n\nIt seems indeed that this is the only place where lazy-fetching is possible.\nI checked this by looking up the calls for oid_object_info_extended() or\nany other function in object-file.c which depends on it.\n\nIn builtin/index-pack.c, we have (in the order that these functions appear)\n\n- check_object()\n    Call to oid_object_info(), but we return early with\n    0 if we don't have an object.\n\n- sha1_object()\n    Call to has_object_file_with_flags() (which this patch replaces with\n    has_object()), where lazy-fetching is possible.\n    \n    Calls to oid_object_info() and read_object_file(), which trigger only\n    when the above has_object_file_with_flags() succeeds.\n\n- fix_unresolved_deltas()\n    Call to oid_object_info_extended(), we prefetch delta bases.\n\n    Call to read_object_file(), but we only read data from ref_delta_entry.\n    In case it was a delta base, we already prefetched it.\n\nThere are cases where we fsck objects, but lazy-fetching is already handled\nin fsck (although by setting fetch_if_missing to 0).\n\nDo we need to be explicit about this in the commit message? That sha1_object()\nis the only place where there is a chance to lazy-fetch if it is a partial clone?\n\n> > +test_expect_success 'index-pack does not lazy-fetch when checking for sha1 collisions' '\n> > +     rm -rf server promisor-remote client &&\n> > +     rm -rf object-count &&\n> > +\n> > +     git init server &&\n> > +     for i in 1 2 3 4\n> > +     do\n> > +             echo $i >$(pwd)/server/file$i &&\n> > +             git -C server add file$i &&\n> > +             git -C server commit -am \"Commit $i\" || return 1\n> > +     done &&\n> > +     git -C server config --local uploadpack.allowFilter 1 &&\n> > +     git -C server config --local uploadpack.allowAnySha1InWant 1 &&\n> > +     HASH=$(git -C server hash-object file3) &&\n> > +\n> > +     git init promisor-remote &&\n> > +     git -C promisor-remote fetch --keep \"file://$(pwd)/server\" $HASH &&\n> > +\n> > +     git clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n> > +     git -C client remote set-url origin \"file://$(pwd)/promisor-remote\" &&\n> > +     git -C client config extensions.partialClone 1 &&\n> > +     git -C client config remote.origin.promisor 1 &&\n> > +\n> > +     # make sure that index-pack is run from within the repository\n> > +     git -C client index-pack $(pwd)/client/.git/objects/pack/*.pack &&\n> > +     test_path_is_missing $(pwd)/client/file3\n> > +'\n>\n> How does this check that no lazy fetch has occurred? It seems to me\n> that you're just checking the existence of a file in the worktree,\n> which does not indicate the presence or absence of a lazy fetch.\n\nWhat I had in mind was if the file was lazy-fetched (because of the failure\nof has_object_file_with_flags() and fetch_if_missing not set to 0), then\nit would be unpacked and we would find it in the worktree. Since, we\nprevent this exact behavior by using has_object(), we should not find\nsuch a file in our repo.\n\n> I think the way to test needs to be more complicated: you need\n> to create a partial clone, fetch into it from another repo, and\n> then verify that no fetches were made to the original partial\n> clone.\n\nSo, after the fetch, during the pack indexing phase, we look for\nany additional fetches made. This makes more sense and it would\nbe way more clear, to anyone reading, than what I wrote.\n\nWill do a reroll. If there needs to be a change in the commit message\nas well, please let me know.\n\nThanks for the review\n"},{"id":"473376","messageId":"20230310183029.19429-1-five231003@gmail.com","threadId":"59307","inReplyTo":"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:30:29Z","receivedAt":"2023-03-10T18:30:55Z","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"},{"id":"473381","messageId":"xmqqzg8k4ad9.fsf@gitster.g","threadId":"59307","inReplyTo":"20230310183029.19429-1-five231003@gmail.com","subject":"Re: [PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-10T20:30:58Z","receivedAt":"2023-03-10T20:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> This global was added as a temporary measure to suppress the fetching\n> of missing objects and can be removed once the commandshave been taught\n> to handle these cases.\n\nTwo requests.\n\n * Could you substantiate this claim for future readers of \"git\n   log\"?  A reference to an old mailing list discussion or a log\n   message of the commit that added the temporary measure that says\n   the above plan would be perfect.\n\n * What exactly does \"once the commands have been taught\"?  Which\n   commands?  Could you clarify?\n\n> Hence, use has_object() to check for the existence of an object, which\n> has the default behavior of not lazy-fetching in a partial clone. It is\n> worth mentioning that this is the only place where there is potential for\n> lazy-fetching and all other cases are properly handled, making it safe to\n> remove this global here.\n\nThis paragraph is very well explained.\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\nOK.  The comment describes the design choice we made to flip the\nfetch_if_missing flag off.  The old world-view was that we would\nnotice a breakage by non-functioning index-pack when a lazy clone is\nmissing objects that we need by disabling auto-fetching, and we\ninstead explicitly handle any missing and necessary objects by lazy\nfetching (like \"when we lack REF_DELTA bases\").  It does sound like\na conservative thing to do, compared to the opposite approach we are\ntaking with this patch, i.e. we would not fail if we tried to access\nobjects we do not need to, because we have lazy fetching enabled,\nand we just ended up with bloated object store nobody may notice.\n\nTo protect us from future breakage that can come from the new\napproach, it is a very good thing that you added new tests to ensure\nno unnecessary lazy fetching is done (I am not offhand sure if that\ntest is sufficient, though).\n\n> -\tfetch_if_missing = 0;\n\nLooking good to me.  Jonathan, who reviewed the previous round, do\nyou have any comments?\n\nThanks, all.  Will queue.\n\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 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"},{"id":"473382","messageId":"20230310211321.4135748-1-jonathantanmy@google.com","threadId":"59307","inReplyTo":"xmqqzg8k4ad9.fsf@gitster.g","subject":"Re: [PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-10T21:13:21Z","receivedAt":"2023-03-10T21:13:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> > Hence, use has_object() to check for the existence of an object, which\n> > has the default behavior of not lazy-fetching in a partial clone. It is\n> > worth mentioning that this is the only place where there is potential for\n> > lazy-fetching and all other cases are properly handled, making it safe to\n> > remove this global here.\n> \n> This paragraph is very well explained.\n\nIt might be good if the \"all other cases\" were enumerated here in the\ncommit message (since the consequence of missing a case might be an\ninfinite loop of fetching).\n\n> OK.  The comment describes the design choice we made to flip the\n> fetch_if_missing flag off.  The old world-view was that we would\n> notice a breakage by non-functioning index-pack when a lazy clone is\n> missing objects that we need by disabling auto-fetching, and we\n> instead explicitly handle any missing and necessary objects by lazy\n> fetching (like \"when we lack REF_DELTA bases\").  It does sound like\n> a conservative thing to do, compared to the opposite approach we are\n> taking with this patch, i.e. we would not fail if we tried to access\n> objects we do not need to, because we have lazy fetching enabled,\n> and we just ended up with bloated object store nobody may notice.\n> \n> To protect us from future breakage that can come from the new\n> approach, it is a very good thing that you added new tests to ensure\n> no unnecessary lazy fetching is done (I am not offhand sure if that\n> test is sufficient, though).\n\nI don't think the test is sufficient - I'll explain that below.\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\nThe file5 isn't committed?\n\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\nIt seems to me that this test clones a repo and then attempts to fetch\nfrom another repo: so far, so good. But I don't think this tests what\nwe want: firstly, the file5 isn't committed, so it is never fetched. And\neven if it was, we only check that file3 was never fetched from \"$(pwd)/\nserver\". But file3 has nothing to do with the subsequent fetch: we are\nonly fetching file5. It is the hash of file5 that we are checking for\ncollisions, and thus it is file5 that we want to verify is not fetched.\n\nSo I think the way to do this is to have 3 repositories like the author\nis doing now (server, client, and repo), and do it as follows:\n - create \"server\", one commit will do\n - clone \"server\" into \"client\" (partial clone)\n - clone \"server\" into \"another-remote\" (not partial clone)\n - add a file (\"new-file\") to \"server\", commit it, and pull from \"another-remote\"\n - fetch from \"another-remote\" into \"client\"\n\nThis way, \"client\" will need to verify that the hash of \"new-file\" has\nno collisions with any object it currently has. If there is no bug,\n\"new-file\" will never be fetched from \"server\", and if there is a bug,\n\"new-file\" will be fetched.\n\nOne problem is that if there is a bug, such a test will cause an\ninfinite loop (we fetch \"new-file\", so we want to check it for\ncollisions, and because of the bug, we fetch \"new-file\" again, which we\ncheck for collisions, and so on) which might be problematic for things\nlike CI. But we might be able to treat timeouts as the same as test\nfailures, so this should be OK.\n"},{"id":"473385","messageId":"xmqqttys4746.fsf@gitster.g","threadId":"59307","inReplyTo":"20230310211321.4135748-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-10T21:41:13Z","receivedAt":"2023-03-10T21:41:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>> > Hence, use has_object() to check for the existence of an object, which\n>> > has the default behavior of not lazy-fetching in a partial clone. It is\n>> > worth mentioning that this is the only place where there is potential for\n>> > lazy-fetching and all other cases are properly handled, making it safe to\n>> > remove this global here.\n>> \n>> This paragraph is very well explained.\n>\n> It might be good if the \"all other cases\" were enumerated here in the\n> commit message (since the consequence of missing a case might be an\n> infinite loop of fetching).\n>\n>> OK.  The comment describes the design choice we made to flip the\n>> fetch_if_missing flag off.  The old world-view was that we would\n>> notice a breakage by non-functioning index-pack when a lazy clone is\n>> missing objects that we need by disabling auto-fetching, and we\n>> instead explicitly handle any missing and necessary objects by lazy\n>> fetching (like \"when we lack REF_DELTA bases\").  It does sound like\n>> a conservative thing to do, compared to the opposite approach we are\n>> taking with this patch, i.e. we would not fail if we tried to access\n>> objects we do not need to, because we have lazy fetching enabled,\n>> and we just ended up with bloated object store nobody may notice.\n>> \n>> To protect us from future breakage that can come from the new\n>> approach, it is a very good thing that you added new tests to ensure\n>> no unnecessary lazy fetching is done (I am not offhand sure if that\n>> test is sufficient, though).\n>\n> I don't think the test is sufficient - I'll explain that below.\n\nI admit I haven't thought about it any longer than anybody who\ntouched this topic, but should \"fetch_if_missing=0\" really be\ntreated as \"it was a dirty hack in the past, now we do not need it,\nas all callers into the object layer avoids lazy fetching when they\ndo not have to, so let's remove it\"?  It looks to me more and more\nthat the old world-view to disable lazy fetching by default and have\nindividual calls to the object layer opt into fetching as needed may\ngive us a better resulting code, or is it just me?  The possible\nerror modes in new code that fails to follow the world-view with and\nwithout this change are:\n\n * If the lazy fetching is disabled by default (i.e. without this\n   patch), a new code can by mistake call has_object(), which does\n   not lazy fetch, when it does need to have the object and should\n   be using something like has_object_file_with_flags(), and dies\n   loudly.\n\n * If the lazy fetching is enabled by default, on the other hand, a\n   new code can by mistake call has_object_file_with_flags(), which\n   does lazy fetch, when it does not need to have the object.  It\n   does not die, it just lazily fetches objects it does not need.\n   The (performance) \"bug\" will stay hidden until somebody complains.\n\nIn short, the world-view of the current code seems to give us\ntighter control over what gets lazy fetched, simply because we do\nnot allow lazy fetching without thinking.\n\nDo we have other uses of fetch_if_missing (i.e. disable lazy\nfetching)?\n\n    $ git grep -l fetch_if_missing\n    Documentation/technical/partial-clone.txt\n    builtin/fetch-pack.c\n    builtin/fsck.c\n    builtin/pack-objects.c\n    builtin/prune.c\n    builtin/rev-list.c\n    cache.h\n    midx.c\n    object-file.c\n    revision.c\n\nAs the default is 1, all these hits (outside the header, doc, and\nobject-file.c) are to disable lazy fetching.  Judging from the list\nof \"family\" that want tighter control over what gets fetched, I have\na feeling that pack-index may want to stay to be in the family.\n\nOr am I missing some big picture goal to eventually getting rid of\nthis mechanism and always allowing lazy fetching?\n\nThanks.\n\n"},{"id":"473395","messageId":"20230311025906.4170554-1-jonathantanmy@google.com","threadId":"59307","inReplyTo":"xmqqttys4746.fsf@gitster.g","subject":"Re: [PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-03-11T02:59:06Z","receivedAt":"2023-03-11T02:59:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Do we have other uses of fetch_if_missing (i.e. disable lazy\n> fetching)?\n> \n>     $ git grep -l fetch_if_missing\n>     Documentation/technical/partial-clone.txt\n>     builtin/fetch-pack.c\n>     builtin/fsck.c\n>     builtin/pack-objects.c\n>     builtin/prune.c\n>     builtin/rev-list.c\n>     cache.h\n>     midx.c\n>     object-file.c\n>     revision.c\n> \n> As the default is 1, all these hits (outside the header, doc, and\n> object-file.c) are to disable lazy fetching.  Judging from the list\n> of \"family\" that want tighter control over what gets fetched, I have\n> a feeling that pack-index may want to stay to be in the family.\n\nI think this \"family\" concept is a good way to think of it. I did\nuse to think that it would be better to be consistent throughout Git\nand choose one world-view, and if I had to choose, it would be the one\nwithout fetch_if_missing=0. But now it does make sense to me to have\ntwo families:\n\n (a) The more low-level code that the lazy fetching itself relies on\n     (and maybe things like builtin/fsck.c as well) where we really need\n     to be careful about what we fetch, and it would be better to err\n     on the side of not fetching. The test cases for these would need to\n     cover both the partial clone cases and the regular cases.\n\n     For these cases, the consequence of lazy-fetching when we shouldn't\n     might be as bad as an infinite loop, so it makes sense to default\n     not lazy-fetching here.\n\n (b) The more high-level code, in which I think that it is better to err\n     on the side of fetching. The test cases would generally not need to\n     cover the partial clone cases (except when there are specific\n     optimizations needed, such as in checkout where we bulk prefetch\n     missing objects).\n\n     For these cases, the consequences of lazy-fetching when we shouldn't\n     are generally performance-related, so it might not be so bad to let\n     development happen in these areas of code without great\n     consideration to whether a lazy-fetch would happen if an object\n     didn't exist. (I do think it would be ideal for all new code to pay\n     attention to when they read objects, which would help not only in\n     partial clone but also in a potential future in which we have non-\n     disk object stores, but we're probably not there yet as a project.)\n\nAnd indeed, pack-index would go in (a).\n"},{"id":"473398","messageId":"20230311060012.22031-1-five231003@gmail.com","threadId":"59307","inReplyTo":"xmqqzg8k4ad9.fsf@gitster.g","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-11T06:00:12Z","receivedAt":"2023-03-11T06:00:34Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Sat, 11 Mar 2023 at 02:01, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> [...]\n>\n> Two requests.\n>\n>  * Could you substantiate this claim for future readers of \"git\n>    log\"?  A reference to an old mailing list discussion or a log\n>    message of the commit that added the temporary measure that says\n>    the above plan would be perfect.\n>\n>  * What exactly does \"once the commands have been taught\"?  Which\n>    commands?  Could you clarify?\n>\n\nWill do the change.\n\n> > Hence, use has_object() to check for the existence of an object, which\n> > has the default behavior of not lazy-fetching in a partial clone. It is\n> > worth mentioning that this is the only place where there is potential for\n> > lazy-fetching and all other cases are properly handled, making it safe to\n> > remove this global here.\n>\n> This paragraph is very well explained.\n>\n\nThanks.\n\n> > @@ -1728,14 +1727,6 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n> >       int report_end_of_input = 0;\n> >       int hash_algo = 0;\n> > \n> > -     /*\n> > -      * index-pack never needs to fetch missing objects except when\n> > -      * REF_DELTA bases are missing (which are explicitly handled). It only\n> > -      * accesses the repo to do hash collision checks and to check which\n> > -      * REF_DELTA bases need to be fetched.\n> > -      */\n>\n> OK.  The comment describes the design choice we made to flip the\n> fetch_if_missing flag off.  The old world-view was that we would\n> notice a breakage by non-functioning index-pack when a lazy clone is\n> missing objects that we need by disabling auto-fetching, and we\n> instead explicitly handle any missing and necessary objects by lazy\n> fetching (like \"when we lack REF_DELTA bases\").  It does sound like\n> a conservative thing to do, compared to the opposite approach we are\n> taking with this patch, i.e. we would not fail if we tried to access\n> objects we do not need to, because we have lazy fetching enabled,\n> and we just ended up with bloated object store nobody may notice.\n>\n> To protect us from future breakage that can come from the new\n> approach, it is a very good thing that you added new tests to ensure\n> no unnecessary lazy fetching is done (I am not offhand sure if that\n> test is sufficient, though).\n>\n> > -     fetch_if_missing = 0;\n>\n> Looking good to me.  Jonathan, who reviewed the previous round, do\n> you have any comments?\n>\n> Thanks, all.  Will queue.\n>\n\nIt does seem that the tests need to be changed significantly. Will do.\n\n> > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> > index 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> >       grep \"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> > +     rm -rf server promisor-remote client repo trace &&\n> > +\n> > +     # setup\n> > +     git init server &&\n> > +     for i in 1 2 3 4\n> > +     do\n> > +             echo $i >server/file$i &&\n> > +             git -C server add file$i &&\n> > +             git -C server commit -am \"Commit $i\" || return 1\n> > +     done &&\n> > +     git -C server config --local uploadpack.allowFilter 1 &&\n> > +     git -C server config --local uploadpack.allowAnySha1InWant 1 &&\n> > +     HASH=$(git -C server hash-object file3) &&\n> > +\n> > +     git init promisor-remote &&\n> > +     git -C promisor-remote fetch --keep \"file://$(pwd)/server\" &&\n> > +\n> > +     git clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n> > +     git -C client remote set-url origin \"file://$(pwd)/promisor-remote\" &&\n> > +     git -C client config extensions.partialClone 1 &&\n> > +     git -C client config remote.origin.promisor 1 &&\n> > +\n> > +     git init repo &&\n> > +     echo \"5\" >repo/file5 &&\n> > +     git -C repo config --local uploadpack.allowFilter 1 &&\n> > +     git -C repo config --local uploadpack.allowAnySha1InWant 1 &&\n> > +\n> > +     # verify that no lazy-fetching is done when fetching from another repo\n> > +     GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client \\\n> > +                                     fetch --keep \"file://$(pwd)/repo\" main &&\n> > +\n> > +     ! grep \"want $HASH\" trace\n> > +'\n> > +\n> >  test_expect_success 'lazy-fetch in submodule succeeds' '\n> >       # setup\n> >       test_config_global protocol.file.allow always &&\n\nThanks\n"},{"id":"473399","messageId":"20230311062219.22325-1-five231003@gmail.com","threadId":"59307","inReplyTo":"20230310211321.4135748-1-jonathantanmy@google.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-11T06:22:19Z","receivedAt":"2023-03-11T06:22:43Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Sat, 11 Mar 2023 at 02:43, Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n> > > Hence, use has_object() to check for the existence of an object, which\n> > > has the default behavior of not lazy-fetching in a partial clone. It is\n> > > worth mentioning that this is the only place where there is potential for\n> > > lazy-fetching and all other cases are properly handled, making it safe to\n> > > remove this global here.\n> >\n> > This paragraph is very well explained.\n>\n> It might be good if the \"all other cases\" were enumerated here in the\n> commit message (since the consequence of missing a case might be an\n> infinite loop of fetching).\n>\n\nI will make the change.\n\n> > OK.  The comment describes the design choice we made to flip the\n> > fetch_if_missing flag off.  The old world-view was that we would\n> > notice a breakage by non-functioning index-pack when a lazy clone is\n> > missing objects that we need by disabling auto-fetching, and we\n> > instead explicitly handle any missing and necessary objects by lazy\n> > fetching (like \"when we lack REF_DELTA bases\").  It does sound like\n> > a conservative thing to do, compared to the opposite approach we are\n> > taking with this patch, i.e. we would not fail if we tried to access\n> > objects we do not need to, because we have lazy fetching enabled,\n> > and we just ended up with bloated object store nobody may notice.\n> >\n> > To protect us from future breakage that can come from the new\n> > approach, it is a very good thing that you added new tests to ensure\n> > no unnecessary lazy fetching is done (I am not offhand sure if that\n> > test is sufficient, though).\n>\n> I don't think the test is sufficient - I'll explain that below.\n>\n> > > +test_expect_success 'index-pack does not lazy-fetch when checking for sha1 collsions' '\n> > > +   rm -rf server promisor-remote client repo trace &&\n> > > +\n> > > +   # setup\n> > > +   git init server &&\n> > > +   for i in 1 2 3 4\n> > > +   do\n> > > +           echo $i >server/file$i &&\n> > > +           git -C server add file$i &&\n> > > +           git -C server commit -am \"Commit $i\" || return 1\n> > > +   done &&\n> > > +   git -C server config --local uploadpack.allowFilter 1 &&\n> > > +   git -C server config --local uploadpack.allowAnySha1InWant 1 &&\n> > > +   HASH=$(git -C server hash-object file3) &&\n> > > +\n> > > +   git init promisor-remote &&\n> > > +   git -C promisor-remote fetch --keep \"file://$(pwd)/server\" &&\n> > > +\n> > > +   git clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n> > > +   git -C client remote set-url origin \"file://$(pwd)/promisor-remote\" &&\n> > > +   git -C client config extensions.partialClone 1 &&\n> > > +   git -C client config remote.origin.promisor 1 &&\n> > > +\n> > > +   git init repo &&\n> > > +   echo \"5\" >repo/file5 &&\n> > > +   git -C repo config --local uploadpack.allowFilter 1 &&\n> > > +   git -C repo config --local uploadpack.allowAnySha1InWant 1 &&\n>\n> The file5 isn't committed?\n\nThat is a blunder.\n\n>\n> [...]\n>\n> So I think the way to do this is to have 3 repositories like the author\n> is doing now (server, client, and repo), and do it as follows:\n>  - create \"server\", one commit will do\n>  - clone \"server\" into \"client\" (partial clone)\n>  - clone \"server\" into \"another-remote\" (not partial clone)\n>  - add a file (\"new-file\") to \"server\", commit it, and pull from \"another-remote\"\n>  - fetch from \"another-remote\" into \"client\"\n>\n> This way, \"client\" will need to verify that the hash of \"new-file\" has\n> no collisions with any object it currently has. If there is no bug,\n> \"new-file\" will never be fetched from \"server\", and if there is a bug,\n> \"new-file\" will be fetched.\n>\n\nSo, we can lose the \"promisor-remote\" in the original test and make the\n\"server\" itself a promisor-remote?\n\nThanks for the review\n\n> One problem is that if there is a bug, such a test will cause an\n> infinite loop (we fetch \"new-file\", so we want to check it for\n> collisions, and because of the bug, we fetch \"new-file\" again, which we\n> check for collisions, and so on) which might be problematic for things\n> like CI. But we might be able to treat timeouts as the same as test\n> failures, so this should be OK.\n"},{"id":"473405","messageId":"m0mt4j5a2n.fsf@epic96565.epic.com","threadId":"59307","inReplyTo":"20230225052439.27096-1-five231003@gmail.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2023-03-11T20:01:28Z","receivedAt":"2023-03-11T20:04:26Z","isPatch":true,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nKousik Sanagavarapu <five231003@gmail.com> writes:\n\n> Hence, use has_object() to check for the existence of an object, which\n> has the default behavior of not lazy-fetching in a partial clone.\n\nAny chance this fixes behavior[1] I was seeing where pushing from a\ndepth=1, treeless clone was fetching content?\n\n[1]: https://lore.kernel.org/git/m0lekp4rjv.fsf@epic96565.epic.com/T/#m83ba30c1bf91106fca61efff6619c491543034e4\n\n--\nSean Allred\n"},{"id":"473407","messageId":"xmqqttyrxbw1.fsf@gitster.g","threadId":"59307","inReplyTo":"m0mt4j5a2n.fsf@epic96565.epic.com","subject":"Re: [PATCH] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-11T20:37:34Z","receivedAt":"2023-03-11T20:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean Allred <allred.sean@gmail.com> writes:\n\n> Kousik Sanagavarapu <five231003@gmail.com> writes:\n>\n>> Hence, use has_object() to check for the existence of an object, which\n>> has the default behavior of not lazy-fetching in a partial clone.\n>\n> Any chance this fixes behavior[1] I was seeing ...\n\nIf I understand correctly, Kousik's change is meant to be a clean-up\nthat does not change any behaviour, so if it changes some behaviour\nfor you, it means the patch is buggy.\n\nBut I think you can build a version of Git without the patch\n(earlier this week would have been a good time to do so to test the\nrelease candidate #2 for the upcoming 2.40) to see if you have the\nbehaviour still, and then apply this patch to see if it fixes it,\nand then report your findings here.  It would make a great\ncontribution to the community.\n\nThanks.\n"},{"id":"473416","messageId":"20230312171613.6968-1-five231003@gmail.com","threadId":"59307","inReplyTo":"xmqqttys4746.fsf@gitster.g","subject":"Re: [PATCH v2] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-12T17:16:13Z","receivedAt":"2023-03-12T17:16:23Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Sat, 11 Mar 2023 at 03:11, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I admit I haven't thought about it any longer than anybody who\n> touched this topic, but should \"fetch_if_missing=0\" really be\n> treated as \"it was a dirty hack in the past, now we do not need it,\n> as all callers into the object layer avoids lazy fetching when they\n> do not have to, so let's remove it\"?  It looks to me more and more\n> that the old world-view to disable lazy fetching by default and have\n> individual calls to the object layer opt into fetching as needed may\n> give us a better resulting code, or is it just me?\n\nI think having a single function to check for object existence, which is\ncompatible with partial clones is better, because the end goal is to\ncompletely integrate the concept of partial clones with git's codebase\nand not have code that worries \"Oh, there maybe bugs here because\nwhat if the user has a partial clone\", everytime the code does an object\nexistence check (that is, a call to has_object_file() or any of its\nrelated functions) and just have fetch_if_missing set to 1 or 0, according\nto the particular command.\n\nIt is also true that has_object() is not that \"single function\", because\nin cases where we are missing an object in partial clone and want it,\nhas_object() has no way of fetching it. With or without flags (it only\nsupports one flag which, when set, rechecks packed storage), it does not\nlazy-fetch in a partial clone. But there are cases where we need such\nobjects, such as the commands that come into \"family (b)\" [1].\n\nSo, why not use oid_object_info_extended() directly, instead of wrapping\nit with some other function, whenever we are checking for an object's\nexistence. We can skip lazy-fetches whenever we want with\nOBJECT_INFO_SKIP_FETCH_OBJECT and can also prefetch with\nOBJECT_INFO_FOR_PREFETCH [2].\n\n[1] https://lore.kernel.org/git/20230311025906.4170554-1-jonathantanmy@google.com/\n\n[2] pack-index itself is one example of where this is done.\n\n    When we don't have REF_DELTA bases, we bulk prefetch them\n\n\t    if (has_promisor_remote()) {\n                     /*\n                      * Prefetch the delta bases.\n                      */\n                     struct oid_array to_fetch = OID_ARRAY_INIT;\n\t\t     for (i = 0; i < nr_ref_deltas; i++) {\n                             struct ref_delta_entry *d = sorted_by_pos[i];\n                             if (!oid_object_info_extended(the_repository, &d->oid,\n                                                           NULL,\n                                                           OBJECT_INFO_FOR_PREFETCH))\n                                     continue;\n                             oid_array_append(&to_fetch, &d->oid);\n                     }\n                     promisor_remote_get_direct(the_repository,\n                                                to_fetch.oid, to_fetch.nr);\n                     oid_array_clear(&to_fetch);\n             }\n\n    Instead of going object-by-object, which is basically like an\n    infinite loop in large repos and partial clones are widely used\n    in large repos.\n"},{"id":"473467","messageId":"20230313181518.6322-1-five231003@gmail.com","threadId":"59307","inReplyTo":"20230310183029.19429-1-five231003@gmail.com","subject":"[PATCH v3] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-13T18:15:18Z","receivedAt":"2023-03-13T18:15:47Z","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 [1] and can be removed once the remaining commands:\n - fetch-pack\n - fsck\n - pack-objects\n - prune\n - rev-list\ncan handle lazy-fetching without fetch_if_missing.\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 [2] are properly handled, making it safe\nto remove this global here.\n\n[1] See 8b4c0103a9 (sha1_file: support lazily fetching missing objects,\n\t\t   2017-12-08)\n[2] These cases are:\n    - When we check objects, but we return with 0 early if the object\n      doesn't exist.\n    - We prefetch delta bases in a partial clone, if we don't have them\n      (as the comment outlines).\n    - There are some cases where we fsck objects, but lazy-fetching is\n      already handled in fsck.\n\nHelped-by: Jonathan Tan <jonathantanmy@google.com>\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n builtin/index-pack.c     | 11 +----------\n t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 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..fdb34a0b50 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -644,6 +644,34 @@ 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 client another-remote &&\n+\n+\tgit init server &&\n+\techo \"line\" >server/file &&\n+\tgit -C server add file &&\n+\tgit -C server commit -am \"file\" &&\n+\tgit -C server config --local uploadpack.allowFilter 1 &&\n+\tgit -C server config --local uploadpack.allowAnySha1InWant 1 &&\n+\n+\tgit clone --no-checkout --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tgit -C client config extensions.partialClone 1 &&\n+\tgit -C client config remote.origin.promisor 1 &&\n+\n+\tgit clone \"file://$(pwd)/server\" another-remote &&\n+\n+\techo \"new line\" >server/new-file &&\n+\tgit -C server add new-file &&\n+\tgit -C server commit -am \"new-file\" &&\n+\n+\tgit -C another-remote pull &&\n+\n+\t# Try to fetch so that \"client\" will have to do a collision-check.\n+\t# This should, however, not fetch \"new-file\" because \"client\" is a\n+\t# partial clone.\n+\tgit -C client fetch \"file://$(pwd)/another-remote\" main\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"},{"id":"473471","messageId":"xmqqmt4gtq8z.fsf@gitster.g","threadId":"59307","inReplyTo":"20230313181518.6322-1-five231003@gmail.com","subject":"Re: [PATCH v3] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-13T19:17:48Z","receivedAt":"2023-03-13T19:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> A collision test is triggered in sha1_object(), whenever there is an\n> object file in our repo. If our repo is a partial clone, then checking\n> for this file existence does not lazy-fetch the object (if the object\n> is missing and if there are one or more promisor remotes) when\n> fetch_if_missing is set to 0.\n> ...\n> Hence, use has_object() to check for the existence of an object, which\n> has the default behavior of not lazy-fetching in a partial clone. It is\n> worth mentioning that this is the only place where there is potential for\n> lazy-fetching and all other cases [2] are properly handled, making it safe\n> to remove this global here.\n>\n> [1] See 8b4c0103a9 (sha1_file: support lazily fetching missing objects,\n> \t\t   2017-12-08)\n\nThanks for this reference.\n\nThe way I read the \"lazy fetching is by default not suppressed, and\nthis is a temporary measure\" described in the log message is quite\nopposite from where this patch wants to go, though.  \n\nI think the commit envisioned the world where all the accesses to\nthe object layer are aware of the characteristics of a lazy clone\n(e.g. has_object() reporting \"we do not have that object locally\n(yet)\" is not an immediate sign of a repository corruption) and when\nto lazily fetch and when to tolerate locally missing objects is\ncontrolled more tightly, and to reach that world one step at a time,\nintroduced the global, so that for now everybody lazily fetches, but\nthe commands individual patches concentrates to \"fix\" can turn off\nthe \"by default all missing objects are lazily fetched\" so that they\ncan either allow certain objects to be locally missing, or fetch\nthem from promisor remotes when they need the contents of such\nobjects.  By fixing each commands one by one, eventually we would be\nable to wean ourselves away from this \"by default everything is\nlazily fetched\" global---in other words, in the ideal endgame, the\nfetch_if_missing should be set to 0 everywhere.\n\nSo, if this patch was made in reaction to the \"it was a temporary\nmeasure\" in 8b4c0103 (sha1_file: support lazily fetching missing\nobjects, 2017-12-08), I think it goes in the completely opposite\ndirection.  If the patch shared the cause with 8b4c0103 and wanted\nto help realize the ideal world, it instead should have left this\ncommand who can already work with fetch_if_missing=0 alone and fixed\nsomebody else who still depends on fetch_if_missing=1, I think.\n\nNow it is a separate issue to argue if \"everybody knows exactly when\nto trigger lazy fetching and fetch_if_missing is set to false\neverywhere\" is really the ideal endgame.  I do not think \"Future\npatches will update some commands to either tolerate missing objects\nor be more efficient in fetching them.\" proposed by the commit from\nlate 2017 has seen that much advance recently.\n\nBut for commands that need to deal with many missing objects,\nenumerating the objects that are missing locally and need fetching\nfirst and then requesting them in a batch should be vastly more\nefficient than the default lazy fetch logic that lets the caller\nrequest a single object, realize it is missing locally, make a\nconnection to fetch that single object and disconnect.  So I have to\nsuspect that ...\n\n> This global was added as a temporary measure to suppress the fetching\n> of missing objects [1] and can be removed once the remaining commands:\n>  - fetch-pack\n>  - fsck\n>  - pack-objects\n>  - prune\n>  - rev-list\n> can handle lazy-fetching without fetch_if_missing.\n\n... this \"can handle\" may be a misguided direction to go in.  They\nwere taught not to lazy fetch because blindly lazy fetching was bad,\nweren't they?\n"},{"id":"473472","messageId":"xmqqilf4tq86.fsf@gitster.g","threadId":"59307","inReplyTo":"20230313181518.6322-1-five231003@gmail.com","subject":"Re: [PATCH v3] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-13T19:18:17Z","receivedAt":"2023-03-13T19:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> A collision test is triggered in sha1_object(), whenever there is an\n> object file in our repo. If our repo is a partial clone, then checking\n> for this file existence does not lazy-fetch the object (if the object\n> is missing and if there are one or more promisor remotes) when\n> fetch_if_missing is set to 0.\n> ...\n> Hence, use has_object() to check for the existence of an object, which\n> has the default behavior of not lazy-fetching in a partial clone. It is\n> worth mentioning that this is the only place where there is potential for\n> lazy-fetching and all other cases [2] are properly handled, making it safe\n> to remove this global here.\n>\n> [1] See 8b4c0103a9 (sha1_file: support lazily fetching missing objects,\n> \t\t   2017-12-08)\n\nThanks for the reference.\n\nThe way I read the \"lazy fetching is by default not suppressed, and\nthis is a temporary measure\" described in the log message is quite\nopposite from where this patch wants to go, though.  \n\nI think the commit envisioned the world where all the accesses to\nthe object layer are aware of the characteristics of a lazy clone\n(e.g. has_object() reporting \"we do not have that object locally\n(yet)\" is not an immediate sign of a repository corruption) and when\nto lazily fetch and when to tolerate locally missing objects is\ncontrolled more tightly, and to reach that world one step at a time,\nintroduced the global, so that for now everybody lazily fetches, but\nthe commands individual patches concentrates to \"fix\" can turn off\nthe \"by default all missing objects are lazily fetched\" so that they\ncan either allow certain objects to be locally missing, or fetch\nthem from promisor remotes when they need the contents of such\nobjects.  By fixing each commands one by one, eventually we would be\nable to wean ourselves away from this \"by default everything is\nlazily fetched\" global---in other words, in the ideal endgame, the\nfetch_if_missing should be set to 0 everywhere.\n\nSo, if this patch was made in reaction to the \"it was a temporary\nmeasure\" in 8b4c0103 (sha1_file: support lazily fetching missing\nobjects, 2017-12-08), I think it goes in the completely opposite\ndirection.  If the patch shared the cause with 8b4c0103 and wanted\nto help realize the ideal world, it instead should have left this\ncommand who can already work with fetch_if_missing=0 alone and fixed\nsomebody else who still depends on fetch_if_missing=1, I think.\n\nNow it is a separate issue to argue if \"everybody knows exactly when\nto trigger lazy fetching and fetch_if_missing is set to false\neverywhere\" is really the ideal endgame.  I do not think \"Future\npatches will update some commands to either tolerate missing objects\nor be more efficient in fetching them.\" proposed by the commit from\nlate 2017 has seen that much advance recently.\n\nBut for commands that need to deal with many missing objects,\nenumerating the objects that are missing locally and need fetching\nfirst and then requesting them in a batch should be vastly more\nefficient than the default lazy fetch logic that lets the caller\nrequest a single object, realize it is missing locally, make a\nconnection to fetch that single object and disconnect.  So I have to\nsuspect that ...\n\n> This global was added as a temporary measure to suppress the fetching\n> of missing objects [1] and can be removed once the remaining commands:\n>  - fetch-pack\n>  - fsck\n>  - pack-objects\n>  - prune\n>  - rev-list\n> can handle lazy-fetching without fetch_if_missing.\n\n... this \"can handle\" may be a misguided direction to go in.  They\nwere taught not to lazy fetch because blindly lazy fetching was bad,\nweren't they?\n"},{"id":"473657","messageId":"20230317175601.4250-1-five231003@gmail.com","threadId":"59307","inReplyTo":"20230313181518.6322-1-five231003@gmail.com","subject":"[PATCH v4] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-17T17:56:01Z","receivedAt":"2023-03-17T17:57:07Z","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\nThough this global lets us control lazy-fetching in regions of code,\nit prevents multi-threading [1].\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, even\nwhen fetch_if_missing is set to 1. It is worth mentioning that this is\nthe only place where there is potential for lazy-fetching and all other\ncases [2] are properly handled, making it safe to remove this global\nhere.\n\n[1] See https://lore.kernel.org/git/xmqqv9sdeeif.fsf@gitster-ct.c.googlers.com/\n    and the discussion that follows from it.\n\n[2] These cases are:\n    - When we check objects, but we return with 0 early if the object\n      doesn't exist.\n    - We prefetch delta bases in a partial clone, if we don't have them\n      (as the comment outlines).\n    - There are some cases where we fsck objects, but lazy-fetching is\n      already handled in fsck.\n\nHelped-by: Jonathan Tan <jonathantanmy@google.com>\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n\nChanges since v3:\n- Changed the commit message to give a stronger reason as to\n  why we should reduce the use of this global and in this\n  process remove it from builtin/index-pack.c\n\n- Also made an addition to the test which I overlooked last time\n  and without which the does not make sense\n\n builtin/index-pack.c     | 11 +----------\n t/t5616-partial-clone.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 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..41fa7130f1 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -644,6 +644,39 @@ 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\n+\t\t     when checking for sha1 collsions' '\n+\trm -rf server client another-remote &&\n+\n+\tgit init server &&\n+\techo \"line\" >server/file &&\n+\tgit -C server add file &&\n+\tgit -C server commit -am \"file\" &&\n+\tgit -C server config --local uploadpack.allowFilter 1 &&\n+\tgit -C server config --local uploadpack.allowAnySha1InWant 1 &&\n+\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t\t\t\"file://$(pwd)/server\" client &&\n+\tgit -C client config extensions.partialClone 1 &&\n+\tgit -C client config remote.origin.promisor 1 &&\n+\n+\tgit clone \"file://$(pwd)/server\" another-remote &&\n+\n+\techo \"new line\" >server/new-file &&\n+\tgit -C server add new-file &&\n+\tgit -C server commit -am \"new-file\" &&\n+\tHASH=$(git -C server hash-object new-file) &&\n+\n+\tgit -C another-remote pull &&\n+\n+\t# Try to fetch so that \"client\" will have to do a collision-check.\n+\t# This should, however, not fetch \"new-file\" because \"client\" is a\n+\t# partial clone.\n+\tGIT_TRACE_PACKET=git -C client fetch \\\n+\t\t\t\t\"file://$(pwd)/another-remote\" main &&\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"},{"id":"473676","messageId":"xmqqlejvf0j6.fsf@gitster.g","threadId":"59307","inReplyTo":"20230317175601.4250-1-five231003@gmail.com","subject":"Re: [PATCH v4] index-pack: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-17T22:58:21Z","receivedAt":"2023-03-17T23:00:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> A collision test is triggered in sha1_object(), whenever there is an\n> object file in our repo. If our repo is a partial clone, then checking\n> for this file existence does not lazy-fetch the object (if the object\n> is missing and if there are one or more promisor remotes) when\n> fetch_if_missing is set to 0.\n>\n> Though this global lets us control lazy-fetching in regions of code,\n> it prevents multi-threading [1].\n\nSorry, but I really do not see the point.\n\nWe already have read_lock/read_unlock to prevent multiple threads\nfrom stomping on the in-core object database structure either way.\n\nIf somebody needs to dynamically change the value of fetch_if_missing\nafter the program started and spawned multiple threads, yes, the update\nto the single variable would become a problem point in multi-threading.\n\nBut that is not what we are doing, and you already discovered that\nthis was done as \"a temporary measure\" to selectively let some\nprograms use 0 and others use 1 for lazy-fetching, at a very early\npart of these programs.\n\nIf we are to reduce this global, perhaps we should teach more\ncodepaths not to lazy fetch by default.  Once everybody gets\nconverted like so, then index-pack can lose the assignment of 0 to\nthe variable, as the global variable would be initialized to 0 and\nnobody will flip it to 1 to \"temporarily opt into lazy fetching by\ndefault until it gets fixed\".  At that point, we can lose the global\nvariable.\n\nSo \"we want to reduce the use of this global\" is not a good reason\nto do this change at all, without a convincing argument that says\nwhy everybody should do automatic lazy fetching of objects.  If\neverybody should avoid doing automatic lazy fetching, a good first\nstep to reduce the use of this global is not to touch index-pack\nthat has already been fixed not to do so, no?\n\n\n"},{"id":"473704","messageId":"92c321c4-7968-e993-4157-f0d06edb9283@gmail.com","threadId":"59307","inReplyTo":"xmqqlejvf0j6.fsf@gitster.g","subject":"Re: [PATCH v4] index-pack: remove fetch_if_missing=0","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-03-19T06:17:01Z","receivedAt":"2023-03-19T06:17:10Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On 18/03/23 04:28, Junio C Hamano wrote:\n\n> Kousik Sanagavarapu <five231003@gmail.com> writes:\n>\n>> A collision test is triggered in sha1_object(), whenever there is an\n>> object file in our repo. If our repo is a partial clone, then checking\n>> for this file existence does not lazy-fetch the object (if the object\n>> is missing and if there are one or more promisor remotes) when\n>> fetch_if_missing is set to 0.\n>>\n>> Though this global lets us control lazy-fetching in regions of code,\n>> it prevents multi-threading [1].\n> Sorry, but I really do not see the point.\n>\n> We already have read_lock/read_unlock to prevent multiple threads\n> from stomping on the in-core object database structure either way.\n>\n> If somebody needs to dynamically change the value of fetch_if_missing\n> after the program started and spawned multiple threads, yes, the update\n> to the single variable would become a problem point in multi-threading.\n>\n> But that is not what we are doing, and you already discovered that\n> this was done as \"a temporary measure\" to selectively let some\n> programs use 0 and others use 1 for lazy-fetching, at a very early\n> part of these programs.\n>\n> If we are to reduce this global, perhaps we should teach more\n> codepaths not to lazy fetch by default.  Once everybody gets\n> converted like so, then index-pack can lose the assignment of 0 to\n> the variable, as the global variable would be initialized to 0 and\n> nobody will flip it to 1 to \"temporarily opt into lazy fetching by\n> default until it gets fixed\".  At that point, we can lose the global\n> variable.\n>\n> So \"we want to reduce the use of this global\" is not a good reason\n> to do this change at all, without a convincing argument that says\n> why everybody should do automatic lazy fetching of objects.  If\n> everybody should avoid doing automatic lazy fetching, a good first\n> step to reduce the use of this global is not to touch index-pack\n> that has already been fixed not to do so, no?\n\nThanks for the review.\n\n\nAlso, thanks for pointing out the direction of work in this area.\n\nReally helpful.\n\n"}]}