{"thread":{"id":"51796","subject":"[PATCH] cache-tree: do not lazy-fetch merge tree","startedAt":"2019-09-03T19:42:52Z","lastAt":"2019-09-10T18:19:24Z","messageCount":12,"participants":["Jonathan Tan","Derrick Stolee","Junio C Hamano","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381764","messageId":"20190903194247.217964-1-jonathantanmy@google.com","threadId":"51796","inReplyTo":null,"subject":"[PATCH] cache-tree: do not lazy-fetch merge tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-03T19:42:47Z","receivedAt":"2019-09-03T19:42:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cherry-picking (for example), new trees may be constructed. During\nthis process, Git checks whether these trees exist. However, in a\npartial clone, this causes a lazy fetch to occur, which is both\nunnecessary (because Git has already constructed this tree as part of\nthe cherry-picking process) and likely to fail (because the remote\nprobably doesn't have this tree).\n\nDo not lazy fetch in this situation.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nAnother partial clone bug.\n\nThis raises the issue that failed fetches are currently fatal - if they\nweren't fatal, this cherry-pick would have worked (except with some\ndelay as the fetch is attempted, and with a warning message about the\nfetch failing). My personal inclination right now is to leave things as\nit is (fatal failed fetches), but I'm open to other opinions.\n---\n cache-tree.c             |  2 +-\n t/t0410-partial-clone.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c22161f987..9e596893bc 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -407,7 +407,7 @@ static int update_one(struct cache_tree *it,\n \tif (repair) {\n \t\tstruct object_id oid;\n \t\thash_object_file(buffer.buf, buffer.len, tree_type, &oid);\n-\t\tif (has_object_file(&oid))\n+\t\tif (has_object_file_with_flags(&oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\toidcpy(&it->oid, &oid);\n \t\telse\n \t\t\tto_invalidate = 1;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 6415063980..3e434b6a81 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -492,6 +492,20 @@ test_expect_success 'gc stops traversal when a missing but promised object is re\n \t! grep \"$TREE_HASH\" out\n '\n \n+test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\ttest_commit -C repo base &&\n+\ttest_commit -C repo side1 &&\n+\tgit -C repo checkout base &&\n+\ttest_commit -C repo side2 &&\n+\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\n+\tgit -C repo cherry-pick side1\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.23.0.187.g17f5b7556c-goog\n\n"},{"id":"381783","messageId":"69f2b069-3e11-4a2e-9b81-bde18c463d8c@gmail.com","threadId":"51796","inReplyTo":"20190903194247.217964-1-jonathantanmy@google.com","subject":"Re: [PATCH] cache-tree: do not lazy-fetch merge tree","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-04T01:37:06Z","receivedAt":"2019-09-04T01:37:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/3/2019 3:42 PM, Jonathan Tan wrote:\n> When cherry-picking (for example), new trees may be constructed. During\n> this process, Git checks whether these trees exist. However, in a\n> partial clone, this causes a lazy fetch to occur, which is both\n> unnecessary (because Git has already constructed this tree as part of\n> the cherry-picking process) and likely to fail (because the remote\n> probably doesn't have this tree).\n\nIf we have constructed the object already, then why do we not see it\nand avoid fetching it? This must be a slightly strange timing issue\nwith objects being flushed to disk or added to the object cache.\n\nOne approach is to find all of these has_object_file() calls that should\nreally be one with OBJECT_INFO_SKIP_FETCH_OBJECT. Another would be to\nfind out why has_object_file() isn't seeing the object we constructed.\n\n> Do not lazy fetch in this situation.\nI agree that the patch has this effect.\n \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Another partial clone bug.\n> \n> This raises the issue that failed fetches are currently fatal - if they\n> weren't fatal, this cherry-pick would have worked (except with some\n> delay as the fetch is attempted, and with a warning message about the\n> fetch failing). My personal inclination right now is to leave things as\n> it is (fatal failed fetches), but I'm open to other opinions.\n> ---\n>  cache-tree.c             |  2 +-\n>  t/t0410-partial-clone.sh | 14 ++++++++++++++\n>  2 files changed, 15 insertions(+), 1 deletion(-)\n> \n> diff --git a/cache-tree.c b/cache-tree.c\n> index c22161f987..9e596893bc 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -407,7 +407,7 @@ static int update_one(struct cache_tree *it,\n>  \tif (repair) {\n>  \t\tstruct object_id oid;\n>  \t\thash_object_file(buffer.buf, buffer.len, tree_type, &oid);\n> -\t\tif (has_object_file(&oid))\n> +\t\tif (has_object_file_with_flags(&oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n>  \t\t\toidcpy(&it->oid, &oid);\n>  \t\telse\n>  \t\t\tto_invalidate = 1;\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 6415063980..3e434b6a81 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -492,6 +492,20 @@ test_expect_success 'gc stops traversal when a missing but promised object is re\n>  \t! grep \"$TREE_HASH\" out\n>  '\n>  \n> +test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n> +\trm -rf repo &&\n> +\ttest_create_repo repo &&\n> +\ttest_commit -C repo base &&\n> +\ttest_commit -C repo side1 &&\n> +\tgit -C repo checkout base &&\n> +\ttest_commit -C repo side2 &&\n> +\n> +\tgit -C repo config core.repositoryformatversion 1 &&\n> +\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n> +\n> +\tgit -C repo cherry-pick side1\n> +'\n> +\n\nI appreciate this test!\n\nThanks,\n-Stolee\n"},{"id":"381847","messageId":"20190904223529.135623-1-jonathantanmy@google.com","threadId":"51796","inReplyTo":"69f2b069-3e11-4a2e-9b81-bde18c463d8c@gmail.com","subject":"Re: [PATCH] cache-tree: do not lazy-fetch merge tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-04T22:35:29Z","receivedAt":"2019-09-04T22:35:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On 9/3/2019 3:42 PM, Jonathan Tan wrote:\n> > When cherry-picking (for example), new trees may be constructed. During\n> > this process, Git checks whether these trees exist. However, in a\n> > partial clone, this causes a lazy fetch to occur, which is both\n> > unnecessary (because Git has already constructed this tree as part of\n> > the cherry-picking process) and likely to fail (because the remote\n> > probably doesn't have this tree).\n> \n> If we have constructed the object already, then why do we not see it\n> and avoid fetching it? This must be a slightly strange timing issue\n> with objects being flushed to disk or added to the object cache.\n\nThanks for taking a look at this! The answer is that I wasn't precise\nwhen I said \"already constructed\" - I meant that it is in a struct\nstrbuf. It hasn't been written to disk yet, so has_object_file() does\nnot see it.\n\n> One approach is to find all of these has_object_file() calls that should\n> really be one with OBJECT_INFO_SKIP_FETCH_OBJECT. Another would be to\n> find out why has_object_file() isn't seeing the object we constructed.\n\nBy the former, do you mean that we should look at the other\nhas_object_file() calls? I looked at the others in cache-tree.c and I\nthink the one in this patch is the only one that is called on an OID\ngenerated from hash_object_file(). (And I answered the latter in the\nabove paragraph.)\n\nTo avoid confusion, maybe this commit message is better:\n\nWhen cherry-picking (for example), new trees may be constructed. During\nthis process, Git constructs the new tree in a struct strbuf, computes\nthe OID of the new tree, and checks if the new OID already exists on\ndisk. However, in a partial clone, the disk check causes a lazy fetch to\noccur, which is both unnecessary (because we have the tree in the struct\nstrbuf) and likely to fail (because the remote probably doesn't have\nthis tree).\n"},{"id":"381850","messageId":"xmqq7e6nskh0.fsf@gitster-ct.c.googlers.com","threadId":"51796","inReplyTo":"20190904223529.135623-1-jonathantanmy@google.com","subject":"Re: [PATCH] cache-tree: do not lazy-fetch merge tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-04T23:35:55Z","receivedAt":"2019-09-04T23:36:01Z","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> When cherry-picking (for example), new trees may be constructed. During\n> this process, Git constructs the new tree in a struct strbuf, computes\n> the OID of the new tree, and checks if the new OID already exists on\n> disk. However, in a partial clone, the disk check causes a lazy fetch to\n> occur, which is both unnecessary (because we have the tree in the struct\n> strbuf) and likely to fail (because the remote probably doesn't have\n> this tree).\n\nFWIW, this logic dates back to aecf567c (\"cache-tree: create/update\ncache-tree on checkout\", 2014-07-05), when \"checkout\" learned to\nperform opportunistic revalidation of cache-tree data structure,\nwithout writing into the object store.  If we were lazily checked\nout, and created a blob locally that happens to match the original\nwe did not fetch in a directory this piece of code is hashing, the\nresulting hash _may_ name a tree that the other side has that we did\nnot fetch, so taking the \"to_invalidate = 1\" side would make the\nresulting cache-tree less optimal, but because the design choice\nbeing made here is to take that hit in order to avoid network cost,\nas long as that is documented properly (iow, \"probably doesn't have\"\nis not an issue; even if they have it, you do not want to fetch and\nmake the cache-tree entry valid), it is OK.\n\n\n"},{"id":"382091","messageId":"20190909190130.146613-1-jonathantanmy@google.com","threadId":"51796","inReplyTo":"20190903194247.217964-1-jonathantanmy@google.com","subject":"[PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-09T19:01:30Z","receivedAt":"2019-09-09T19:01:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cherry-picking (for example), new trees may be constructed. During\nthis process, Git constructs the new tree in a struct strbuf, computes\nthe OID of the new tree, and checks if the new OID already exists on\ndisk. However, in a partial clone, the disk check causes a lazy fetch to\noccur, which is both unnecessary (because we have the tree in the struct\nstrbuf) and likely to fail (because the remote probably doesn't have\nthis tree).\n\nDo not lazy fetch in this situation.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nAs requested in What's Cooking [1], here's a patch with an updated\ncommit message. Otherwise, the patch is exactly the same.\n\n[1] https://public-inbox.org/git/xmqqd0gcm2zm.fsf@gitster-ct.c.googlers.com/\n---\n cache-tree.c             |  2 +-\n t/t0410-partial-clone.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c22161f987..9e596893bc 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -407,7 +407,7 @@ static int update_one(struct cache_tree *it,\n \tif (repair) {\n \t\tstruct object_id oid;\n \t\thash_object_file(buffer.buf, buffer.len, tree_type, &oid);\n-\t\tif (has_object_file(&oid))\n+\t\tif (has_object_file_with_flags(&oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\toidcpy(&it->oid, &oid);\n \t\telse\n \t\t\tto_invalidate = 1;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 6415063980..3e434b6a81 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -492,6 +492,20 @@ test_expect_success 'gc stops traversal when a missing but promised object is re\n \t! grep \"$TREE_HASH\" out\n '\n \n+test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\ttest_commit -C repo base &&\n+\ttest_commit -C repo side1 &&\n+\tgit -C repo checkout base &&\n+\ttest_commit -C repo side2 &&\n+\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\n+\tgit -C repo cherry-pick side1\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.23.0.162.g0b9fbb3734-goog\n\n"},{"id":"382095","messageId":"xmqqsgp5i6s1.fsf@gitster-ct.c.googlers.com","threadId":"51796","inReplyTo":"20190909190130.146613-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T19:55:26Z","receivedAt":"2019-09-09T19:55:35Z","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> When cherry-picking (for example), new trees may be constructed. During\n> this process, Git constructs the new tree in a struct strbuf, computes\n> the OID of the new tree, and checks if the new OID already exists on\n> disk. However, in a partial clone, the disk check causes a lazy fetch to\n> occur, which is both unnecessary (because we have the tree in the struct\n> strbuf) and likely to fail (because the remote probably doesn't have\n> this tree).\n\nI somehow smell that the above misses the point of the check in the\nfirst place, though.  The reason why we are computing the tree\nobject's name and seeing if we have it locally on disk is to decide\nif we want to record it in the cache tree, *without* writing the\ntree out to our object store, no?\n\nIt is not just unnecessary but also against the goal of the codepath\nto lazily download it, even if the tree is available remotely.  And\nit is irrelevant that there are cases the remote does not have\nit---we have no need to mention that we must be prepared to see the\nlazy fetch to fail.  Even when they do have one, we do not want to\nfetch it and write to our object store.\n\nIsn't that what is going on?  I thought I dug up the original that\nintroduced the has_object_file() call to this codepath to make sure\nwe understand why we make the check (and I expected the person who\nis proposing this change to do the same and record the finding in\nthe proposed log message).\n\nI am running out of time today, and will revisit later this week\n(I'll be down for at least two days starting tomorrow, by the way).\n\nThanks.\n"},{"id":"382099","messageId":"xmqqef0pi3im.fsf@gitster-ct.c.googlers.com","threadId":"51796","inReplyTo":"xmqqsgp5i6s1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T21:05:53Z","receivedAt":"2019-09-09T21:06:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Isn't that what is going on?  I thought I dug up the original that\n> introduced the has_object_file() call to this codepath to make sure\n> we understand why we make the check (and I expected the person who\n> is proposing this change to do the same and record the finding in\n> the proposed log message).\n>\n> I am running out of time today, and will revisit later this week\n> (I'll be down for at least two days starting tomorrow, by the way).\n\nHere is what I came up with.\n\n    The cache-tree datastructure is used to speed up the comparison\n    between the HEAD and the index, and when the index is updated by\n    a cherry-pick (for example), a tree object that would represent\n    the paths in the index in a directory is constructed in-core, to\n    see if such a tree object exists already in the object store.\n\n    When the lazy-fetch mechanism was introduced, we converted this\n    \"does the tree exist?\" check into an \"if it does not, and if we\n    lazily cloned, see if the remote has it\" call by mistake.  Since\n    the whole point of this check is to repair the cache-tree by\n    recording an already existing tree object opportunistically, we\n    shouldn't even try to fetch one from the remote.\n\n    Pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag to make sure we only\n    check for existence in the local object store without triggering the\n    lazy fetch mechanism.\n\n"},{"id":"382104","messageId":"20190909222101.GB31319@sigill.intra.peff.net","threadId":"51796","inReplyTo":"xmqqef0pi3im.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-09T22:21:01Z","receivedAt":"2019-09-09T22:21:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2019 at 02:05:53PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Isn't that what is going on?  I thought I dug up the original that\n> > introduced the has_object_file() call to this codepath to make sure\n> > we understand why we make the check (and I expected the person who\n> > is proposing this change to do the same and record the finding in\n> > the proposed log message).\n> >\n> > I am running out of time today, and will revisit later this week\n> > (I'll be down for at least two days starting tomorrow, by the way).\n> \n> Here is what I came up with.\n> \n>     The cache-tree datastructure is used to speed up the comparison\n>     between the HEAD and the index, and when the index is updated by\n>     a cherry-pick (for example), a tree object that would represent\n>     the paths in the index in a directory is constructed in-core, to\n>     see if such a tree object exists already in the object store.\n> \n>     When the lazy-fetch mechanism was introduced, we converted this\n>     \"does the tree exist?\" check into an \"if it does not, and if we\n>     lazily cloned, see if the remote has it\" call by mistake.  Since\n>     the whole point of this check is to repair the cache-tree by\n>     recording an already existing tree object opportunistically, we\n>     shouldn't even try to fetch one from the remote.\n> \n>     Pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag to make sure we only\n>     check for existence in the local object store without triggering the\n>     lazy fetch mechanism.\n\nAs a third-party observer, that explanation makes sense to me.\n\nI wondered also if this means we should be using OBJECT_INFO_QUICK.\nI.e., do we expect to see a \"miss\" here often, forcing us to re-scan the\npacked directory?\n\nReading dd0c34c46b (cache-tree: protect against \"git prune\".,\n2006-04-24), I think the answer is \"no\".\n\n-Peff\n"},{"id":"382111","messageId":"xmqqwoehgdnk.fsf@gitster-ct.c.googlers.com","threadId":"51796","inReplyTo":"20190909222101.GB31319@sigill.intra.peff.net","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-10T01:09:51Z","receivedAt":"2019-09-10T01:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I wondered also if this means we should be using OBJECT_INFO_QUICK.\n> I.e., do we expect to see a \"miss\" here often, forcing us to re-scan the\n> packed directory?\n\nAs a performance optimization hack, it is OK if we did not notice\nthat the tree object, which corresponds to what is currently\nprepared for a directory in the index, does exist in the object\nstore.  It is not worth rescanning the packs to \"protect\" against\nraces, I think, in the \"repair\" codepath.\n\nWhen the user actually wants to write the index out as a tree, we\nwould write it out as a loose object (or omit doing so if we know\nthere are already copies), but because it is not a crime to create a\nduplicate loose object when we already have a packed copy, I do not\nthink we need to rescan in that context, either.  But I do not think\nthe codepath Jonathan's patch touches is about that operation.\n"},{"id":"382127","messageId":"20190910124920.GI32087@szeder.dev","threadId":"51796","inReplyTo":"20190909190130.146613-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-10T12:49:20Z","receivedAt":"2019-09-10T12:49:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Sep 09, 2019 at 12:01:30PM -0700, Jonathan Tan wrote:\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 6415063980..3e434b6a81 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -492,6 +492,20 @@ test_expect_success 'gc stops traversal when a missing but promised object is re\n>  \t! grep \"$TREE_HASH\" out\n>  '\n>  \n> +test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n> +\trm -rf repo &&\n> +\ttest_create_repo repo &&\n> +\ttest_commit -C repo base &&\n> +\ttest_commit -C repo side1 &&\n> +\tgit -C repo checkout base &&\n> +\ttest_commit -C repo side2 &&\n> +\n> +\tgit -C repo config core.repositoryformatversion 1 &&\n> +\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n> +\n> +\tgit -C repo cherry-pick side1\n> +'\n> +\n\nSidenote, just curious: did you originally intend to add this test\nbefore the test script sources 'lib-httpd.sh', or you were about to\nappend it at the end as usual, but then noticed the warning comment\ntelling you not to do so?\n\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n"},{"id":"382140","messageId":"20190910181534.134890-1-jonathantanmy@google.com","threadId":"51796","inReplyTo":"xmqqef0pi3im.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-10T18:15:34Z","receivedAt":"2019-09-10T18:15:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Isn't that what is going on?  I thought I dug up the original that\n> > introduced the has_object_file() call to this codepath to make sure\n> > we understand why we make the check (and I expected the person who\n> > is proposing this change to do the same and record the finding in\n> > the proposed log message).\n> >\n> > I am running out of time today, and will revisit later this week\n> > (I'll be down for at least two days starting tomorrow, by the way).\n> \n> Here is what I came up with.\n> \n>     The cache-tree datastructure is used to speed up the comparison\n>     between the HEAD and the index, and when the index is updated by\n>     a cherry-pick (for example), a tree object that would represent\n>     the paths in the index in a directory is constructed in-core, to\n>     see if such a tree object exists already in the object store.\n> \n>     When the lazy-fetch mechanism was introduced, we converted this\n>     \"does the tree exist?\" check into an \"if it does not, and if we\n>     lazily cloned, see if the remote has it\" call by mistake.  Since\n>     the whole point of this check is to repair the cache-tree by\n>     recording an already existing tree object opportunistically, we\n>     shouldn't even try to fetch one from the remote.\n> \n>     Pass the OBJECT_INFO_SKIP_FETCH_OBJECT flag to make sure we only\n>     check for existence in the local object store without triggering the\n>     lazy fetch mechanism.\n\nThis commit message looks good to me. Thanks for writing the commit\nmessage - I thought that the justification in the commit message I wrote\nwould be sufficient, but it makes sense to look into why the check was\ndone.\n"},{"id":"382141","messageId":"20190910181918.137494-1-jonathantanmy@google.com","threadId":"51796","inReplyTo":"20190910124920.GI32087@szeder.dev","subject":"Re: [PATCH v2] cache-tree: do not lazy-fetch merge tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-09-10T18:19:18Z","receivedAt":"2019-09-10T18:19:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Sidenote, just curious: did you originally intend to add this test\n> before the test script sources 'lib-httpd.sh', or you were about to\n> append it at the end as usual, but then noticed the warning comment\n> telling you not to do so?\n\nHonestly, I don't remember. I do try to put tests near similar tests, so\nI might have seen that we had HTTP tests at the bottom and non-HTTP\ntests at the top, but in this case, I don't remember if I had that\nthought before putting this test where it is now.\n"}]}