{"thread":{"id":"51987","subject":"[PATCH] fetch: delay fetch_if_missing=0 until after config","startedAt":"2019-10-07T18:18:31Z","lastAt":"2019-11-01T20:43:26Z","messageCount":11,"participants":["Jonathan Tan","Emily Shaffer","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"383598","messageId":"20191007181825.13463-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":null,"subject":"[PATCH] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-07T18:18:25Z","receivedAt":"2019-10-07T18:18:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running \"git fetch\" in a partial clone with no blobs, for example,\nby:\n\n  git clone --filter=blob:none --no-checkout \\\n    https://kernel.googlesource.com/pub/scm/git/git\n  git -C git fetch\n\n\"git fetch\" will fail to load the config blob object, printing \"unable\nto load config blob object\".\n\nThis is because fetch_if_missing is set to 0 before the config is\nprocessed. Git must set fetch_if_missing to 0 before the fetch because\nas part of the fetch, packfile negotiation happens (and we do not want\nto fetch any missing objects when checking existence of objects), but we\ndo not need to set it so early. Move the setting of fetch_if_missing to\nthe earliest possible point in cmd_fetch(), right before any fetching\nhappens.\n---\nThis is not a full solution, but this helps in the use case described in\nthe commit message. The full solution probably will involve teaching the\nfetch mechanism to support arbitrary struct repository objects, and by\nmoving fetch_if_missing into the repository object. (Alternatively, we\ncould add the equivalent of OBJECT_INFO_SKIP_FETCH_OBJECT to functions\nlike parse_commit() that are used by files like negotiator/default.c, or\nsplit up commit parsing into object reading - which already has that\nflag - and commit parsing.)\n---\n builtin/fetch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 24d382b2fb..865ae6677d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1666,8 +1666,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch\");\n \n-\tfetch_if_missing = 0;\n-\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\n@@ -1734,6 +1732,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tfetch_if_missing = 0;\n+\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-- \n2.23.0.581.g78d2f28ef7-goog\n\n"},{"id":"384723","messageId":"20191023210324.GC139951@google.com","threadId":"51987","inReplyTo":"20191007181825.13463-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: delay fetch_if_missing=0 until after config","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-10-23T21:03:24Z","receivedAt":"2019-10-23T21:03:32Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Oct 07, 2019 at 11:18:25AM -0700, Jonathan Tan wrote:\n> When running \"git fetch\" in a partial clone with no blobs, for example,\n> by:\n> \n>   git clone --filter=blob:none --no-checkout \\\n>     https://kernel.googlesource.com/pub/scm/git/git\n>   git -C git fetch\n> \n> \"git fetch\" will fail to load the config blob object, printing \"unable\n> to load config blob object\".\n\nI'm having some trouble figuring out which object is actually missing.\nIs this the .git/config object? (That doesn't make much sense to me...)\nIs it .gitmodules?\n\n> \n> This is because fetch_if_missing is set to 0 before the config is\n> processed. Git must set fetch_if_missing to 0 before the fetch because\n> as part of the fetch, packfile negotiation happens (and we do not want\n> to fetch any missing objects when checking existence of objects), but we\n> do not need to set it so early. Move the setting of fetch_if_missing to\n> the earliest possible point in cmd_fetch(), right before any fetching\n> happens.\n\nDoubts aside about what's actually failing, I definitely agree with the\npremise of not setting this until the last moment we need it. Plus, I\nmay be alone here, but it'd make it easier for me to understand the code\nif I saw a note explaining *why* we don't want to fetch_if_missing in\nthis case.\n\nBy the way, I think I understand that this is OK to go in\nunconditionally because:\n - In the full clone case, it's a no-op; we haven't got anything that's\n   missing, so who cares.\n - In the filter case, it's as you said - we don't want to\n   fetch_if_missing because that will turn someone's partial clone into\n   a a full clone.\n   - This probably applies to bare checkout, too.\n\nOf course if I'm wrong I'd like to know, but that's how I understand it\nat the moment.\n\n> ---\n> This is not a full solution, but this helps in the use case described in\n> the commit message. The full solution probably will involve teaching the\n> fetch mechanism to support arbitrary struct repository objects, and by\n> moving fetch_if_missing into the repository object. (Alternatively, we\n> could add the equivalent of OBJECT_INFO_SKIP_FETCH_OBJECT to functions\n> like parse_commit() that are used by files like negotiator/default.c, or\n> split up commit parsing into object reading - which already has that\n> flag - and commit parsing.)\n\nAh, I remember this was listed as one of the potential intern projects -\nI think we dismissed it as being too tech-debt-y for an intern to feel\ngood about. :(\n\n> ---\n>  builtin/fetch.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 24d382b2fb..865ae6677d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1666,8 +1666,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \n>  \tpacket_trace_identity(\"fetch\");\n>  \n> -\tfetch_if_missing = 0;\n> -\n>  \t/* Record the command line for the reflog */\n>  \tstrbuf_addstr(&default_rla, \"fetch\");\n>  \tfor (i = 1; i < argc; i++)\n> @@ -1734,6 +1732,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>  \n> +\tfetch_if_missing = 0;\n> +\n>  \tif (remote) {\n>  \t\tif (filter_options.choice || has_promisor_remote())\n>  \t\t\tfetch_one_setup_partial(remote);\n> -- \n> 2.23.0.581.g78d2f28ef7-goog\n> \n\nThe diff, though, looks fine for me.\n\n - Emily\n"},{"id":"384726","messageId":"20191023214428.129593-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":"20191007181825.13463-1-jonathantanmy@google.com","subject":"[PATCH v2] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-23T21:44:28Z","receivedAt":"2019-10-23T21:44:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Suppose, from a repository that has \".gitmodules\", we clone with\n--filter=blob:none:\n\n  git clone --filter=blob:none --no-checkout \\\n    https://kernel.googlesource.com/pub/scm/git/git\n\nThen we fetch:\n\n  git -C git fetch\n\nThis will cause a \"unable to load config blob object\", because the\nfetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to\nload \".gitmodules\" (which Git knows to exist because the client has the\ntree of HEAD) while fetch_if_missing is set to 0.\n\nfetch_if_missing is set to 0 too early - \".gitmodules\" here should be\nlazily fetched.  Git must set fetch_if_missing to 0 before the fetch\nbecause as part of the fetch, packfile negotiation happens (and we do\nnot want to fetch any missing objects when checking existence of\nobjects), but we do not need to set it so early. Move the setting of\nfetch_if_missing to the earliest possible point in cmd_fetch(), right\nbefore any fetching happens.\n---\nNo changes from v1 except that I improved the commit message.\n\nThanks, Emily, for taking a look.\n\n> I'm having some trouble figuring out which object is actually missing.\n> Is this the .git/config object? (That doesn't make much sense to me...)\n> Is it .gitmodules?\n\nYes, it is indeed .gitmodules. I improved the commit message to further\nexplain what is going on.\n\n> By the way, I think I understand that this is OK to go in\n> unconditionally because:\n>  - In the full clone case, it's a no-op; we haven't got anything that's\n>    missing, so who cares.\n>  - In the filter case, it's as you said - we don't want to\n>    fetch_if_missing because that will turn someone's partial clone into\n>    a a full clone.\n>    - This probably applies to bare checkout, too.\n\nYes, that is correct. What do you mean by bare checkout? If you mean the\ncheckout that happens after clone (that we can suppress with\n--no-checkout), that indeed happens after fetch_if_missing=0, so we\nshouldn't have a problem there.\n\n builtin/fetch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0c345b5dfe..863c858fde 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1755,8 +1755,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch\");\n \n-\tfetch_if_missing = 0;\n-\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\n@@ -1824,6 +1822,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tfetch_if_missing = 0;\n+\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-- \n2.23.0.866.gb869b98d4c-goog\n\n"},{"id":"384728","messageId":"20191023233026.GC124161@google.com","threadId":"51987","inReplyTo":"20191023214428.129593-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch: delay fetch_if_missing=0 until after config","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-10-23T23:30:26Z","receivedAt":"2019-10-23T23:30:34Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Oct 23, 2019 at 02:44:28PM -0700, Jonathan Tan wrote:\n> Suppose, from a repository that has \".gitmodules\", we clone with\n> --filter=blob:none:\n> \n>   git clone --filter=blob:none --no-checkout \\\n>     https://kernel.googlesource.com/pub/scm/git/git\n> \n> Then we fetch:\n> \n>   git -C git fetch\n> \n> This will cause a \"unable to load config blob object\", because the\n> fetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to\n> load \".gitmodules\" (which Git knows to exist because the client has the\n> tree of HEAD) while fetch_if_missing is set to 0.\n> \n> fetch_if_missing is set to 0 too early - \".gitmodules\" here should be\n> lazily fetched.  Git must set fetch_if_missing to 0 before the fetch\n> because as part of the fetch, packfile negotiation happens (and we do\n> not want to fetch any missing objects when checking existence of\n> objects), but we do not need to set it so early. Move the setting of\n> fetch_if_missing to the earliest possible point in cmd_fetch(), right\n> before any fetching happens.\n\nI think your sign-off is missing from the new commit message, right?\n\nOtherwise it looks fine to me.\n\n> ---\n> No changes from v1 except that I improved the commit message.\n> \n> Thanks, Emily, for taking a look.\n> \n> > I'm having some trouble figuring out which object is actually missing.\n> > Is this the .git/config object? (That doesn't make much sense to me...)\n> > Is it .gitmodules?\n> \n> Yes, it is indeed .gitmodules. I improved the commit message to further\n> explain what is going on.\n> \n> > By the way, I think I understand that this is OK to go in\n> > unconditionally because:\n> >  - In the full clone case, it's a no-op; we haven't got anything that's\n> >    missing, so who cares.\n> >  - In the filter case, it's as you said - we don't want to\n> >    fetch_if_missing because that will turn someone's partial clone into\n> >    a a full clone.\n> >    - This probably applies to bare checkout, too.\n> \n> Yes, that is correct. What do you mean by bare checkout? If you mean the\n> checkout that happens after clone (that we can suppress with\n> --no-checkout), that indeed happens after fetch_if_missing=0, so we\n> shouldn't have a problem there.\n\nI meant bare clone, not checkout, my apologies, but as I understand it\nbetter, they're completely separate concepts - that is, you can\ncertainly have a bare clone which is also a full clone. So, please\ndisregard this comment.\n\n - Emily\n"},{"id":"384735","messageId":"20191023233403.145457-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":"20191007181825.13463-1-jonathantanmy@google.com","subject":"[PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-23T23:34:03Z","receivedAt":"2019-10-23T23:34:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Suppose, from a repository that has \".gitmodules\", we clone with\n--filter=blob:none:\n\n  git clone --filter=blob:none --no-checkout \\\n    https://kernel.googlesource.com/pub/scm/git/git\n\nThen we fetch:\n\n  git -C git fetch\n\nThis will cause a \"unable to load config blob object\", because the\nfetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to\nload \".gitmodules\" (which Git knows to exist because the client has the\ntree of HEAD) while fetch_if_missing is set to 0.\n\nfetch_if_missing is set to 0 too early - \".gitmodules\" here should be\nlazily fetched.  Git must set fetch_if_missing to 0 before the fetch\nbecause as part of the fetch, packfile negotiation happens (and we do\nnot want to fetch any missing objects when checking existence of\nobjects), but we do not need to set it so early. Move the setting of\nfetch_if_missing to the earliest possible point in cmd_fetch(), right\nbefore any fetching happens.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nNo change from v2 except that I've added Signed-off-by.\n---\n builtin/fetch.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0c345b5dfe..863c858fde 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1755,8 +1755,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"fetch\");\n \n-\tfetch_if_missing = 0;\n-\n \t/* Record the command line for the reflog */\n \tstrbuf_addstr(&default_rla, \"fetch\");\n \tfor (i = 1; i < argc; i++)\n@@ -1824,6 +1822,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tfetch_if_missing = 0;\n+\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-- \n2.23.0.866.gb869b98d4c-goog\n\n"},{"id":"384746","messageId":"xmqqo8y6g3yu.fsf@gitster-ct.c.googlers.com","threadId":"51987","inReplyTo":"20191023233403.145457-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T04:30:33Z","receivedAt":"2019-10-24T04:30:38Z","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> Suppose, from a repository that has \".gitmodules\", we clone with\n> --filter=blob:none:\n>\n>   git clone --filter=blob:none --no-checkout \\\n>     https://kernel.googlesource.com/pub/scm/git/git\n>\n> Then we fetch:\n>\n>   git -C git fetch\n>\n> This will cause a \"unable to load config blob object\", because the\n> fetch_config_from_gitmodules() invocation in cmd_fetch() will attempt to\n> load \".gitmodules\" (which Git knows to exist because the client has the\n> tree of HEAD) while fetch_if_missing is set to 0.\n>\n> fetch_if_missing is set to 0 too early - \".gitmodules\" here should be\n> lazily fetched.  Git must set fetch_if_missing to 0 before the fetch\n> because as part of the fetch, packfile negotiation happens (and we do\n> not want to fetch any missing objects when checking existence of\n> objects)...\n\nIs it only me to feel that this is piling band-aids on top of\nband-aids?\n\nPerhaps the addition (and enabling) of lazy-fetch should have been\ndone after \"checking existence\" calls are vetted and sifted into two\ncategories?  Some accesses to objects are \"because we need it\nnow---so let's lazily fetch if that is available as a fallback\noption to us\", as opposed to \"if we do not have it locally right\nnow, we want to know the fact\".  And each callsite should be able to\ndeclare for what reason between the two it is making the access.\n\nThe single fetch-if-missing boolean may have been a quick-and-dirty\nway to get the ball rolling, but perhaps the codebase grew up enough\nso that it is time to wean off of it?\n"},{"id":"384786","messageId":"20191024191804.57275-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":"xmqqo8y6g3yu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-24T19:18:04Z","receivedAt":"2019-10-24T19:18:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > fetch_if_missing is set to 0 too early - \".gitmodules\" here should be\n> > lazily fetched.  Git must set fetch_if_missing to 0 before the fetch\n> > because as part of the fetch, packfile negotiation happens (and we do\n> > not want to fetch any missing objects when checking existence of\n> > objects)...\n> \n> Is it only me to feel that this is piling band-aids on top of\n> band-aids?\n\nTo me, this is moving a band-aid, not adding another one. But to the\nbigger point...\n\n> Perhaps the addition (and enabling) of lazy-fetch should have been\n> done after \"checking existence\" calls are vetted and sifted into two\n> categories?  Some accesses to objects are \"because we need it\n> now---so let's lazily fetch if that is available as a fallback\n> option to us\", as opposed to \"if we do not have it locally right\n> now, we want to know the fact\".  And each callsite should be able to\n> declare for what reason between the two it is making the access.\n> \n> The single fetch-if-missing boolean may have been a quick-and-dirty\n> way to get the ball rolling, but perhaps the codebase grew up enough\n> so that it is time to wean off of it?\n\nThis is one of the alternatives I mentioned in the original email [1]\nafter \"---\". But to elaborate on that, I prefer sifting regions over\nsifting calls.\n\nSifting calls into two categories might work, but it's error-prone in\nthat we would have to do the same line-by-line analysis we did when we\nadded the repository argument to many functions, and we would have to\nmodify functions like parse_commit() to take a flag similar to\nOBJECT_INFO_SKIP_FETCH_OBJECT. Also, we would have to do the same\ncareful inspection for future patches.\n\nInstead, we can control whether a region of code lazy-fetches by moving\nfetch_if_missing to the struct repository object and using a struct\nrepository that has fetch_if_missing==0 when we don't want lazy\nfetching. Besides being less error-prone, the infrastructure for this\nhas already been built (if I remember correctly, for submodules, at\nfirst).\n\nThe microproject to put fetch_if_missing into struct repository and my\nOutreachy project 'Refactor \"git index-pack\" logic into library code'\n[2] are steps towards this goal. (I don't think that the latter is\nstrictly necessary, but it will make things simpler. In particular,\npassing \"-C\" then the_repository->gitdir to index-pack makes a few tests\nfail - not many, but not zero; and even after we resolve that, we would\nneed CLI parameters ensuring that we marshal everything we need from\nthe_repository, including fetch_if_missing. It seems better to libify\nindex-pack first.)\n\n[1] https://public-inbox.org/git/20191007181825.13463-1-jonathantanmy@google.com/\n[2] https://www.outreachy.org/apply/project-selection/#git\n"},{"id":"384823","messageId":"xmqqv9sdeeif.fsf@gitster-ct.c.googlers.com","threadId":"51987","inReplyTo":"20191024191804.57275-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-25T02:38:00Z","receivedAt":"2019-10-25T02:38:08Z","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> To me, this is moving a band-aid, not adding another one. But to the\n\nFair enough.\n\n> Sifting calls into two categories might work, but it's error-prone in\n> that we would have to do the same line-by-line analysis we did when we\n> added the repository argument to many functions, and we would have to\n> modify functions like parse_commit() to take a flag similar to\n> OBJECT_INFO_SKIP_FETCH_OBJECT. Also, we would have to do the same\n> careful inspection for future patches.\n\nAbsolutely.  That is the price to pay for the lazy-fetch feature.\n\n> Instead, we can control whether a region of code lazy-fetches...\n\nThe approach \"from here to there, we can set global to forbid\nlazy-fetch\" may prolong the life support of the quick-and-dirty\nmechanism, but it has to assume you can say \"from here to there\"; it\nwould mean that we cannot go multi-threaded until we get off of it.\n\nThat is why I said it may be time for us to wean us off of the\nquick-and-dirty hack that helped us bootstrapping the initial\nimplementation of lazy clone.\n"},{"id":"384878","messageId":"20191025174129.194010-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":"xmqqv9sdeeif.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-25T17:41:29Z","receivedAt":"2019-10-25T17:41:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Instead, we can control whether a region of code lazy-fetches...\n> \n> The approach \"from here to there, we can set global to forbid\n> lazy-fetch\" may prolong the life support of the quick-and-dirty\n> mechanism, but it has to assume you can say \"from here to there\"; it\n> would mean that we cannot go multi-threaded until we get off of it.\n\nBy \"from here to there\", I meant, for example, creating a struct\nrepository in cmd_fetch() that has fetch_if_missing=0, then passing that\nrepository to fetch_pack() (once fetch_pack() and all functions it calls\nsupport a repository object). In that way, from here (start of\nfetch_pack()) to there (end of fetch_pack()) there will be no lazy\nfetching. As a bonus, if we ever want to support fetching in\nrepositories other than the_repository (e.g. submodules), this change\nwill allow us to support that too. I don't think this is\nquick-and-dirty, and I don't see how this prevents multithreading.\n"},{"id":"385032","messageId":"xmqqh83s8h3m.fsf@gitster-ct.c.googlers.com","threadId":"51987","inReplyTo":"20191025174129.194010-1-jonathantanmy@google.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-29T01:39:57Z","receivedAt":"2019-10-29T01:40:05Z","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>> > Instead, we can control whether a region of code lazy-fetches...\n>> \n>> The approach \"from here to there, we can set global to forbid\n>> lazy-fetch\" may prolong the life support of the quick-and-dirty\n>> mechanism, but it has to assume you can say \"from here to there\"; it\n>> would mean that we cannot go multi-threaded until we get off of it.\n>\n> By \"from here to there\", I meant, for example, creating a struct\n> repository in cmd_fetch() that has fetch_if_missing=0, then passing that\n> repository to fetch_pack() (once fetch_pack() and all functions it calls\n> support a repository object).\n\nI know---but that means the struct cannot be shared among threads\nthat are calling object layer, some of which want to lazily fetch\nmissing objects while others only want to check the existence, at\nthe same time.\n\nCompared to that, judicious use of OBJECT_INFO_SKIP_FETCH_OBJECT and\nother flags by callers can tell the underlying machinery why we are\ninterested in the object, which I think is the right direction to go\nin the longer term.  What I am not certain about is if we are ready\nto move to the right direction for the longer term, or we should still\nbe relying on the big-hammer global bit for expediency a bit longer.\n"},{"id":"385280","messageId":"20191101204321.234006-1-jonathantanmy@google.com","threadId":"51987","inReplyTo":"xmqqh83s8h3m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fetch: delay fetch_if_missing=0 until after config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-01T20:43:21Z","receivedAt":"2019-11-01T20:43:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> >> > Instead, we can control whether a region of code lazy-fetches...\n> >> \n> >> The approach \"from here to there, we can set global to forbid\n> >> lazy-fetch\" may prolong the life support of the quick-and-dirty\n> >> mechanism, but it has to assume you can say \"from here to there\"; it\n> >> would mean that we cannot go multi-threaded until we get off of it.\n> >\n> > By \"from here to there\", I meant, for example, creating a struct\n> > repository in cmd_fetch() that has fetch_if_missing=0, then passing that\n> > repository to fetch_pack() (once fetch_pack() and all functions it calls\n> > support a repository object).\n> \n> I know---but that means the struct cannot be shared among threads\n> that are calling object layer, some of which want to lazily fetch\n> missing objects while others only want to check the existence, at\n> the same time.\n\nThat's true. Mixing lazy-fetch and non-lazy-fetch in a function is rare,\nbut admittedly it does happen - in particular, when fetching, the server\nmay send us objects delta-ed against a missing object, and we need to\nlazy-fetch those missing objects.\n\n> Compared to that, judicious use of OBJECT_INFO_SKIP_FETCH_OBJECT and\n> other flags by callers can tell the underlying machinery why we are\n> interested in the object, which I think is the right direction to go\n> in the longer term.  What I am not certain about is if we are ready\n> to move to the right direction for the longer term, or we should still\n> be relying on the big-hammer global bit for expediency a bit longer.\n\nI've taken a look at this and figured out tests that can demonstrate\nwhat exactly is being lazy-fetched (and hopefully will be good enough to\nprevent future code changes from adding extra lazy-fetches). In this\nway, maybe we can at least reduce the scope of the big-hammer global.\n\nI just sent out a patch [1].\n\n[1] https://public-inbox.org/git/20191101203830.231676-1-jonathantanmy@google.com/\n"}]}