{"thread":{"id":"52841","subject":"[PATCH 0/2] Document two partial clone bugs, fix one","startedAt":"2020-02-19T16:21:58Z","lastAt":"2020-02-22T17:26:01Z","messageCount":15,"participants":["Derrick Stolee via GitGitGadget","Jonathan Tan","Eric Sunshine","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"392068","messageId":"pull.556.git.1582129312.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":null,"subject":"[PATCH 0/2] Document two partial clone bugs, fix one","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T16:21:50Z","receivedAt":"2020-02-19T16:21:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While playing with partial clone, I discovered a few bugs and document them\nwith tests in patch 1. One seems to be a server-side bug that happens in a\nsomewhat rare situation, but not terribly unlikely. The other is a\nclient-side bug that leads to quadratic amounts of data transfer; I fix this\nbug in patch 2.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n  partial-clone: demonstrate bugs in partial fetch\n  partial-clone: avoid fetching when looking for objects\n\n builtin/fetch.c          | 10 +++++-----\n t/t5616-partial-clone.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 5 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-556%2Fderrickstolee%2Fpartial-clone-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-556/derrickstolee/partial-clone-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/556\n-- \ngitgitgadget\n"},{"id":"392069","messageId":"dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":"pull.556.git.1582129312.gitgitgadget@gmail.com","subject":"[PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T16:21:51Z","receivedAt":"2020-02-19T16:21:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhile testing partial clone, I noticed some odd behavior. I was testing\na way of running 'git init', followed by manually configuring the remote\nfor partial clone, and then running 'git fetch'. Astonishingly, I saw\nthe 'git fetch' process start asking the server for multiple rounds of\npack-file downloads! When tweaking the situation a little more, I\ndiscovered that I could cause the remote to hang up with an error.\n\nAdd two tests that demonstrate these two issues.\n\nIn the first test, we find that when fetching with blob filters from\na repository that previously did not have any tags, the 'git fetch\n--tags origin' command fails because the server sends \"multiple\nfilter-specs cannot be combined\".\n\nIn the second test, we see that a 'git fetch origin' request with\nseveral ref updates results in multiple pack-file downloads. This must\nbe due to Git trying to fault-in the objects pointed by the refs. What\nmakes this matter particularly nasty is that this goes through the\ndo_oid_object_info_extended() method, so there are no \"haves\" in the\nnegotiation. This leads the remote to send every reachable commit and\ntree from each new ref, providing a quadratic amount of data transfer!\nThis test is fixed if we revert 6462d5eb9a (fetch: remove\nfetch_if_missing=0, 2019-11-05), but that revert causes other test\nfailures. The real fix will need more care.\n\nThe tests are ordered in this way because if I swap the test order the\ntag test will succeed instead of fail. I believe this is because somehow\nwe need the srv.bare repo to not have any tags when we clone, but then\nhave tags in our next fetch.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t5616-partial-clone.sh | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex fea56cda6d3..ed2ef45c37a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n+test_expect_failure 'verify fetch succeeds when asking for new tags' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n+\tfor i in I J K\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i\n+\tdone &&\n+\tgit -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n+\tgit -C tag-test fetch --tags origin\n+'\n+\n+test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 2 pack-list &&\n+\tfor i in A B C\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i\n+\tdone &&\n+\tgit -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n+\tgit -C pack-test fetch origin &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 3 pack-list\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \ngitgitgadget\n\n"},{"id":"392070","messageId":"937a882261d4d4552a144e5f0efad8abd8002ab4.1582129312.git.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":"pull.556.git.1582129312.gitgitgadget@gmail.com","subject":"[PATCH 2/2] partial-clone: avoid fetching when looking for objects","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-19T16:21:52Z","receivedAt":"2020-02-19T16:21:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen using partial-clone, do_oid_object_info_extended() can trigger a\nfetch for missing objects. This can be extremely expensive when asking\nfor a tag or commit, as we are completely removed from the context of\nthe missing object and thus supply no \"haves\" in the request.\n\n6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a\nglobal variable that prevented these fetches in favor of a bitflag.\nHowever, some object existence checks were not updated to use this flag.\n\nUpdate find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in\naddition to OBJECT_INFO_QUICK. The _QUICK option only prevents\nrepreparing the pack-file structures. We need to be extremely careful\nabout supplying _SKIP_FETCH_OBJECT when we expect an object to not exist\ndue to updated refs.\n\nThis resolves a broken test in t5616-partial-clone.sh.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/fetch.c          | 10 +++++-----\n t/t5616-partial-clone.sh |  2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d06..fd69c4f69d7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -335,6 +335,7 @@ static void find_non_local_tags(const struct ref *refs,\n \tstruct string_list_item *remote_ref_item;\n \tconst struct ref *ref;\n \tstruct refname_hash_entry *item = NULL;\n+\tconst int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;\n \n \trefname_hash_init(&existing_refs);\n \trefname_hash_init(&remote_refs);\n@@ -353,10 +354,9 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t */\n \t\tif (ends_with(ref->name, \"^{}\")) {\n \t\t\tif (item &&\n-\t\t\t    !has_object_file_with_flags(&ref->old_oid,\n-\t\t\t\t\t\t\tOBJECT_INFO_QUICK) &&\n+\t\t\t    !has_object_file_with_flags(&ref->old_oid, quick_flags) &&\n \t\t\t    !oidset_contains(&fetch_oids, &ref->old_oid) &&\n-\t\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t\t\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t\t\t    !oidset_contains(&fetch_oids, &item->oid))\n \t\t\t\tclear_item(item);\n \t\t\titem = NULL;\n@@ -370,7 +370,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t * fetch.\n \t\t */\n \t\tif (item &&\n-\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t\t    !oidset_contains(&fetch_oids, &item->oid))\n \t\t\tclear_item(item);\n \n@@ -391,7 +391,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t * checked to see if it needs fetching.\n \t */\n \tif (item &&\n-\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t    !oidset_contains(&fetch_oids, &item->oid))\n \t\tclear_item(item);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex ed2ef45c37a..c70516734d5 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -385,7 +385,7 @@ test_expect_failure 'verify fetch succeeds when asking for new tags' '\n \tgit -C tag-test fetch --tags origin\n '\n \n-test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n+test_expect_success 'verify fetch downloads only one pack when updating refs' '\n \tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n \tls pack-test/.git/objects/pack/*pack >pack-list &&\n \ttest_line_count = 2 pack-list &&\n-- \ngitgitgadget\n"},{"id":"392088","messageId":"20200219181042.2192-1-jonathantanmy@google.com","threadId":"52841","inReplyTo":"937a882261d4d4552a144e5f0efad8abd8002ab4.1582129312.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] partial-clone: avoid fetching when looking for objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-02-19T18:10:42Z","receivedAt":"2020-02-19T18:10:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> When using partial-clone, do_oid_object_info_extended() can trigger a\n> fetch for missing objects. This can be extremely expensive when asking\n> for a tag or commit, as we are completely removed from the context of\n> the missing object and thus supply no \"haves\" in the request.\n> \n> 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a\n> global variable that prevented these fetches in favor of a bitflag.\n> However, some object existence checks were not updated to use this flag.\n> \n> Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in\n> addition to OBJECT_INFO_QUICK. The _QUICK option only prevents\n> repreparing the pack-file structures. We need to be extremely careful\n> about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist\n> due to updated refs.\n> \n> This resolves a broken test in t5616-partial-clone.sh.\n> \n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n\nThanks for catching this.\n\nI wonder if the commit message in this patch could be better worded -\nthe first paragraph seems to say that fetching missing commits and tags\nare expensive, but that is not the problem here; the problem is that the\nclient lazily fetches refs advertised by the server, thinking that it is\nlacking them due to partial clone, even when there is no expectation\nthat the client have them (so the commits and tags are not truly\nmissing).\n\nSo I would reword the first paragraph as:\n\n  When using partial clone, find_non_local_tags() in builtin/fetch.c\n  checks each remote tag to see if its object also exists locally. There\n  is no expectation that the object exist locally, but this function\n  nevertheless triggers a lazy fetch if the object does not exist. This\n  can be extremely expensive when asking for a commit, as we are\n  completely removed from the context of the non-existent object and\n  thus supply no \"haves\" in the request.\n\nAll this rests on my thinking that \"missing\" has the connotation (or\nactual meaning) that we expect the object to be there. If we think that\n\"missing\" can also mean that the remote has it but the local doesn't,\nthen you can ignore what I just said :-)\n\nOther than that, both patches look good to me.\n"},{"id":"392089","messageId":"CAPig+cSRQXRtSscKUUJBA0NShRQfA=NHohnA2GMRmgbpWr_MvA@mail.gmail.com","threadId":"52841","inReplyTo":"dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-19T18:38:12Z","receivedAt":"2020-02-19T18:38:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 19, 2020 at 11:22 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> [...]\n> The tests are ordered in this way because if I swap the test order the\n> tag test will succeed instead of fail. I believe this is because somehow\n> we need the srv.bare repo to not have any tags when we clone, but then\n> have tags in our next fetch.\n\nThis ordering requirement might deserve an in-code comment in the test\nscript itself.\n\nMore below...\n\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n> +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n> +       git clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n> +       for i in I J K\n> +       do\n> +               test_commit -C src $i &&\n> +               git -C src branch $i\n> +       done &&\n\nIf test_commit() or git-branch fail, those failures will go unnoticed.\nYou can fix this by bailing from the loop, like this:\n\n    for i in I J K\n    do\n        test_commit -C src $i &&\n        git -C src branch $i || return 1\n    done &&\n\nSame comment applies to the other new test.\n\n> +       git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n> +       git -C tag-test fetch --tags origin\n> +'\n> +\n> +test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n> +       git clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n> +       ls pack-test/.git/objects/pack/*pack >pack-list &&\n> +       test_line_count = 2 pack-list &&\n> +       for i in A B C\n> +       do\n> +               test_commit -C src $i &&\n> +               git -C src branch $i\n> +       done &&\n> +       git -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n> +       git -C pack-test fetch origin &&\n> +       ls pack-test/.git/objects/pack/*pack >pack-list &&\n> +       test_line_count = 3 pack-list\n> +'\n"},{"id":"392107","messageId":"30d990a4-ed72-93b8-3226-a576246134ee@gmail.com","threadId":"52841","inReplyTo":"CAPig+cSRQXRtSscKUUJBA0NShRQfA=NHohnA2GMRmgbpWr_MvA@mail.gmail.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-02-19T20:42:47Z","receivedAt":"2020-02-19T20:42:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/19/2020 1:38 PM, Eric Sunshine wrote:\n> On Wed, Feb 19, 2020 at 11:22 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> [...]\n>> The tests are ordered in this way because if I swap the test order the\n>> tag test will succeed instead of fail. I believe this is because somehow\n>> we need the srv.bare repo to not have any tags when we clone, but then\n>> have tags in our next fetch.\n> \n> This ordering requirement might deserve an in-code comment in the test\n> script itself.\n\nCan do.\n\n> More below...\n> \n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n>> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n>> +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n>> +       git clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n>> +       for i in I J K\n>> +       do\n>> +               test_commit -C src $i &&\n>> +               git -C src branch $i\n>> +       done &&\n> \n> If test_commit() or git-branch fail, those failures will go unnoticed.\n> You can fix this by bailing from the loop, like this:\n> \n>     for i in I J K\n>     do\n>         test_commit -C src $i &&\n>         git -C src branch $i || return 1\n>     done &&\n> \n> Same comment applies to the other new test.\n\nThanks!\n\n-Stolee\n"},{"id":"392110","messageId":"xmqqimk2e1vl.fsf@gitster-ct.c.googlers.com","threadId":"52841","inReplyTo":"dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T20:52:14Z","receivedAt":"2020-02-19T20:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index fea56cda6d3..ed2ef45c37a 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -374,6 +374,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n>  \tgrep \"want $(cat hash)\" trace\n>  '\n>  \n> +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n> +\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n> +\tfor i in I J K\n> +\tdo\n> +\t\ttest_commit -C src $i &&\n> +\t\tgit -C src branch $i\n> +\tdone &&\n> +\tgit -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n> +\tgit -C tag-test fetch --tags origin\n> +'\n\nIs this about an ultra-recent regresssion?  When applied directly on\ntop of v2.25.0, this one seems to pass already without any change.\n\n> +test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n> +\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n> +\tls pack-test/.git/objects/pack/*pack >pack-list &&\n> +\ttest_line_count = 2 pack-list &&\n> +\tfor i in A B C\n> +\tdo\n> +\t\ttest_commit -C src $i &&\n> +\t\tgit -C src branch $i\n> +\tdone &&\n> +\tgit -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n> +\tgit -C pack-test fetch origin &&\n> +\tls pack-test/.git/objects/pack/*pack >pack-list &&\n> +\ttest_line_count = 3 pack-list\n> +'\n> +\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n"},{"id":"392112","messageId":"CAPig+cT+MO_JmZf_qfxu8ZkPLWDOpZSe8rZxt-7LK4d6+RJ6JA@mail.gmail.com","threadId":"52841","inReplyTo":"xmqqimk2e1vl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-19T20:59:48Z","receivedAt":"2020-02-19T21:00:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n> > +     git clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n> > +     for i in I J K\n> > +     do\n> > +             test_commit -C src $i &&\n> > +             git -C src branch $i\n> > +     done &&\n> > +     git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n> > +     git -C tag-test fetch --tags origin\n> > +'\n>\n> Is this about an ultra-recent regresssion?  When applied directly on\n> top of v2.25.0, this one seems to pass already without any change.\n\nTrue, although both fail when applied atop \"master\".\n"},{"id":"392115","messageId":"3666d3af-2c5e-40ae-99e5-b002c9011732@gmail.com","threadId":"52841","inReplyTo":"pull.556.git.1582129312.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Document two partial clone bugs, fix one","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-02-19T21:10:14Z","receivedAt":"2020-02-19T21:10:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/19/2020 11:21 AM, Derrick Stolee via GitGitGadget wrote:\n> While playing with partial clone, I discovered a few bugs and document them\n> with tests in patch 1. One seems to be a server-side bug that happens in a\n> somewhat rare situation, but not terribly unlikely. The other is a\n> client-side bug that leads to quadratic amounts of data transfer; I fix this\n> bug in patch 2.\n\nWhile I was able to demonstrate these bugs, after looking at my real-world\nexample with these fixes I found _yet another_ set of issues.\n\nMy \"real world\" test is the following: run \"git init\" and then populate\nthe config file with these contents:\n\n[core]\n        repositoryformatversion = 1\n        filemode = true\n        bare = false\n        logallrefupdates = true\n[remote \"origin\"]\n        url = <url-that-supports-partial-clone>\n        fetch = +refs/heads/*:refs/remotes/origin/*\n        promisor = true\n        partialclonefilter = blob:none\n[branch \"master\"]\n        remote = origin\n        merge = refs/heads/master\n\nThen run \"git fetch origin\".\n\nFirst, we check if the repository contains a .gitmodule file, which\ntriggers a download of HEAD:.gitmodules. First HEAD, then its blob.\n(This may be a bit of a red herring: I may have forgotten to delete\nthe contents of my refs/heads/master when setting up the test.)\n\n#0  promisor_remote_get_direct (repo=repo@entry=0x555555a71680 <the_repo>, oids=oids@entry=0x7fffffffda70, oid_nr=oid_nr@entry=1) at promisor-remote.c:237\n#1  0x000055555572bbfa in oid_object_info_extended (r=r@entry=0x555555a71680 <the_repo>, oid=oid@entry=0x7fffffffda70, oi=oi@entry=0x7fffffffd8e0, flags=flags@entry=0) at sha1-file.c:1483\n#2  0x000055555572bd7c in read_object (r=r@entry=0x555555a71680 <the_repo>, oid=oid@entry=0x7fffffffda70, type=type@entry=0x7fffffffda64, size=size@entry=0x7fffffffda68) at sha1-file.c:1537\n#3  0x000055555572be15 in read_object_file_extended (r=r@entry=0x555555a71680 <the_repo>, oid=oid@entry=0x7fffffffda70, type=type@entry=0x7fffffffda64, size=size@entry=0x7fffffffda68, \n    lookup_replace=lookup_replace@entry=1) at sha1-file.c:1579\n#4  0x000055555572c04c in repo_read_object_file (size=0x7fffffffda68, type=0x7fffffffda64, oid=0x7fffffffda70, r=0x555555a71680 <the_repo>) at object-store.h:192\n#5  read_object_with_reference (r=r@entry=0x555555a71680 <the_repo>, oid=oid@entry=0x7fffffffdc20, required_type_name=<optimized out>, size=size@entry=0x7fffffffdae8, \n    actual_oid_return=actual_oid_return@entry=0x7fffffffdaf0) at sha1-file.c:1619\n#6  0x0000555555755a53 in get_tree_entry (r=r@entry=0x555555a71680 <the_repo>, tree_oid=tree_oid@entry=0x7fffffffdc20, name=name@entry=0x5555557d5ffe \".gitmodules\", oid=0x7fffffffdd40, \n    mode=mode@entry=0x7fffffffdcb0) at tree-walk.c:573\n#7  0x000055555572f628 in get_oid_with_context_1 (repo=repo@entry=0x555555a71680 <the_repo>, name=name@entry=0x5555557d5ff9 \"HEAD:.gitmodules\", flags=flags@entry=0, prefix=prefix@entry=0x0, \n    oid=oid@entry=0x7fffffffdd40, oc=oc@entry=0x7fffffffdcb0) at sha1-name.c:1899\n#8  0x000055555572fea3 in get_oid_with_context (oc=0x7fffffffdcb0, oid=0x7fffffffdd40, flags=0, str=0x5555557d5ff9 \"HEAD:.gitmodules\", repo=0x555555a71680 <the_repo>) at sha1-name.c:1946\n#9  repo_get_oid (r=r@entry=0x555555a71680 <the_repo>, name=name@entry=0x5555557d5ff9 \"HEAD:.gitmodules\", oid=oid@entry=0x7fffffffdd40) at sha1-name.c:1602\n#10 0x000055555573d962 in config_from_gitmodules (fn=fn@entry=0x55555573da30 <gitmodules_fetch_config>, repo=0x555555a71680 <the_repo>, data=data@entry=0x7fffffffdda0) at submodule-config.c:648\n#11 0x000055555573ebed in config_from_gitmodules (data=0x7fffffffdda0, repo=<optimized out>, fn=0x55555573da30 <gitmodules_fetch_config>) at submodule-config.c:637\n#12 fetch_config_from_gitmodules (max_children=max_children@entry=0x555555a32894 <submodule_fetch_jobs_config>, recurse_submodules=recurse_submodules@entry=0x555555a3288c <recurse_submodules>)\n    at submodule-config.c:799\n#13 0x00005555555a6920 in cmd_fetch (argc=2, argv=0x7fffffffe2e8, prefix=0x0) at builtin/fetch.c:1762\n#14 0x0000555555570b9d in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:444\n#15 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674\n#16 0x0000555555571d05 in run_argv (argv=0x7fffffffe030, argcp=0x7fffffffe03c) at git.c:741\n#17 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872\n#18 0x00005555555707c8 in main (argc=6, argv=0x7fffffffe2c8) at common-main.c:52\n\nTHEN, we start walking the refs to see if we have the objects\nlocally:\n\n#0  promisor_remote_get_direct (repo=repo@entry=0x555555a71680 <the_repo>, oids=oids@entry=0x555555af7ed0, oid_nr=oid_nr@entry=1) at promisor-remote.c:237\n#1  0x000055555572bbfa in oid_object_info_extended (r=0x555555a71680 <the_repo>, oid=<optimized out>, oi=0x555555a71940 <blank_oi>, oi@entry=0x0, flags=flags@entry=4) at sha1-file.c:1483\n#2  0x000055555572c420 in repo_has_object_file_with_flags (flags=<optimized out>, oid=<optimized out>, r=<optimized out>) at sha1-file.c:1935\n#3  repo_has_object_file (r=<optimized out>, oid=<optimized out>) at sha1-file.c:1942\n#4  0x00005555556e682f in ref_resolves_to_object (refname=0x555555af7f40 \"refs/remotes/origin/next\", oid=<optimized out>, flags=<optimized out>) at refs.c:261\n#5  0x00005555556eb059 in files_ref_iterator_advance (ref_iterator=0x555555b27ed0) at refs/files-backend.c:754\n#6  0x00005555556f1082 in ref_iterator_advance (ref_iterator=0x555555b27ed0) at refs/iterator.c:13\n#7  do_for_each_repo_ref_iterator (r=0x555555a71680 <the_repo>, iter=0x555555b27ed0, fn=fn@entry=0x5555556e5930 <do_for_each_ref_helper>, cb_data=cb_data@entry=0x7fffffffdc80) at refs/iterator.c:417\n#8  0x00005555556e7b79 in do_for_each_ref (refs=<optimized out>, prefix=prefix@entry=0x5555557cb095 \"\", fn=fn@entry=0x5555555a3cb0 <add_one_refname>, trim=trim@entry=0, flags=flags@entry=0, \n    cb_data=cb_data@entry=0x55555572bbfa <oid_object_info_extended+778>) at refs.c:1566\n#9  0x00005555556e8918 in refs_for_each_ref (cb_data=0x55555572bbfa <oid_object_info_extended+778>, fn=0x5555555a3cb0 <add_one_refname>, refs=<optimized out>) at refs.c:1572\n#10 for_each_ref (fn=fn@entry=0x5555555a3cb0 <add_one_refname>, cb_data=cb_data@entry=0x7fffffffdd40) at refs.c:1577\n#11 0x00005555555a46d6 in find_non_local_tags (refs=0x970e93150326b500, refs@entry=0x555555b2ae40, head=head@entry=0x7fffffffde20, tail=tail@entry=0x7fffffffde28) at builtin/fetch.c:344\n#12 0x00005555555a7a87 in get_ref_map (rs=0x7fffffffde90, rs=0x7fffffffde90, autotags=<synthetic pointer>, tags=<optimized out>, remote_refs=0x555555b2ae40, remote=<optimized out>) at builtin/fetch.c:523\n#13 do_fetch (rs=0x7fffffffde90, transport=<optimized out>) at builtin/fetch.c:1367\n#14 fetch_one (prune_tags_ok=<optimized out>, argv=<optimized out>, argc=<optimized out>, remote=<optimized out>) at builtin/fetch.c:1738\n#15 cmd_fetch (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin/fetch.c:1827\n#16 0x0000555555570b9d in run_builtin (argv=<optimized out>, argc=<optimized out>, p=<optimized out>) at git.c:444\n#17 handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674\n#18 0x0000555555571d05 in run_argv (argv=0x7fffffffe050, argcp=0x7fffffffe05c) at git.c:741\n#19 cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872\n#20 0x00005555555707c8 in main (argc=5, argv=0x7fffffffe2e8) at common-main.c:52\n\nThis is running through has_object_file(), but the worst part is that switching\nit to has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT) will cause\nwarnings when we do not have the object. Something else must be done here.\n\nSince this is more complicated to fix, I'm going to set this part aside\nfor now. I may come back with a test case that demonstrates the problem.\n\nThanks,\n-Stolee\n"},{"id":"392116","messageId":"xmqqblpue0oq.fsf@gitster-ct.c.googlers.com","threadId":"52841","inReplyTo":"CAPig+cT+MO_JmZf_qfxu8ZkPLWDOpZSe8rZxt-7LK4d6+RJ6JA@mail.gmail.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T21:17:57Z","receivedAt":"2020-02-19T21:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> > +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n>> > +     git clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n>> > +     for i in I J K\n>> > +     do\n>> > +             test_commit -C src $i &&\n>> > +             git -C src branch $i\n>> > +     done &&\n>> > +     git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n>> > +     git -C tag-test fetch --tags origin\n>> > +'\n>>\n>> Is this about an ultra-recent regresssion?  When applied directly on\n>> top of v2.25.0, this one seems to pass already without any change.\n>\n> True, although both fail when applied atop \"master\".\n\nI flipped the first one (i.e. test #24) to expect success and run\nbisect between 3f7553ac (\"Merge branch 'jt/t5616-robustify'\",\n2020-02-12) and the tip of 'master'.\n\nInteresting that bisecting it points at 684ceae3 (\"fetch: default to\nprotocol version 2\", 2019-12-23).\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 9a9178fd28..099406c2f1 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -384,6 +384,32 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n+test_expect_success 'verify fetch succeeds when asking for new tags' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n+\tfor i in I J K\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i\n+\tdone &&\n+\tgit -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n+\tgit -C tag-test fetch --tags origin\n+'\n+\n+test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 2 pack-list &&\n+\tfor i in A B C\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i\n+\tdone &&\n+\tgit -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n+\tgit -C pack-test fetch origin &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 3 pack-list\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.25.1-440-g39558b81cc\n\n"},{"id":"392117","messageId":"c57b9bff-dc6d-2117-f708-46a02d06d5ef@gmail.com","threadId":"52841","inReplyTo":"xmqqblpue0oq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-02-19T21:20:15Z","receivedAt":"2020-02-19T21:20:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/19/2020 4:17 PM, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n>> On Wed, Feb 19, 2020 at 3:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>>> +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n>>>> +     git clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n>>>> +     for i in I J K\n>>>> +     do\n>>>> +             test_commit -C src $i &&\n>>>> +             git -C src branch $i\n>>>> +     done &&\n>>>> +     git -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n>>>> +     git -C tag-test fetch --tags origin\n>>>> +'\n>>>\n>>> Is this about an ultra-recent regresssion?  When applied directly on\n>>> top of v2.25.0, this one seems to pass already without any change.\n>>\n>> True, although both fail when applied atop \"master\".\n> \n> I flipped the first one (i.e. test #24) to expect success and run\n> bisect between 3f7553ac (\"Merge branch 'jt/t5616-robustify'\",\n> 2020-02-12) and the tip of 'master'.\n> \n> Interesting that bisecting it points at 684ceae3 (\"fetch: default to\n> protocol version 2\", 2019-12-23).\n\nThanks for tracking this down. I had originally been working on\ntop of master, but then rebased onto v2.25.0 to test this on our\nVFS for Git/Scalar fork [1]. I have since noticed that the test\npasses in that case.\n\nThanks,\n-Stolee\n\n[1] https://github.com/microsoft/git/pull/247\n"},{"id":"392281","messageId":"pull.556.v2.git.1582321648.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":"pull.556.git.1582129312.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Document two partial clone bugs, fix one","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-21T21:47:26Z","receivedAt":"2020-02-21T21:47:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While playing with partial clone, I discovered a few bugs and document them\nwith tests in patch 1. One seems to be a server-side bug that happens in a\nsomewhat rare situation, but not terribly unlikely. The other is a\nclient-side bug that leads to quadratic amounts of data transfer; I fix this\nbug in patch 2.\n\nUPDATES in V2:\n\n * Added \"|| return 1\" inside the for loops.\n   \n   \n * Added an in-test comment about the test ordering.\n   \n   \n * Required protocol.version=2 in the tags test due to the bisect Junio\n   performed.\n   \n   \n * Updated the commit message via Jonathan Tan's suggestion.\n   \n   \n\nYou can ignore the stack traces I sent earlier, as those seem to be from\nstates I cannot get into without being destructive to my .git directory.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n  partial-clone: demonstrate bugs in partial fetch\n  partial-clone: avoid fetching when looking for objects\n\n builtin/fetch.c          | 10 +++++-----\n t/t5616-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 5 deletions(-)\n\n\nbase-commit: d0654dc308b0ba76dd8ed7bbb33c8d8f7aacd783\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-556%2Fderrickstolee%2Fpartial-clone-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-556/derrickstolee/partial-clone-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/556\n\nRange-diff vs v1:\n\n 1:  dbc1bdcae16 ! 1:  04965a8c7a4 partial-clone: demonstrate bugs in partial fetch\n     @@ -14,7 +14,8 @@\n          In the first test, we find that when fetching with blob filters from\n          a repository that previously did not have any tags, the 'git fetch\n          --tags origin' command fails because the server sends \"multiple\n     -    filter-specs cannot be combined\".\n     +    filter-specs cannot be combined\". This only happens when using\n     +    protocol v2.\n      \n          In the second test, we see that a 'git fetch origin' request with\n          several ref updates results in multiple pack-file downloads. This must\n     @@ -41,15 +42,20 @@\n       \tgrep \"want $(cat hash)\" trace\n       '\n       \n     ++# The following two tests must be in this order, or else\n     ++# the first will not fail. It is important that the srv.bare\n     ++# repository did not have tags during clone, but has tags\n     ++# in the fetch.\n     ++\n      +test_expect_failure 'verify fetch succeeds when asking for new tags' '\n      +\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n      +\tfor i in I J K\n      +\tdo\n      +\t\ttest_commit -C src $i &&\n     -+\t\tgit -C src branch $i\n     ++\t\tgit -C src branch $i || return 1\n      +\tdone &&\n      +\tgit -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n     -+\tgit -C tag-test fetch --tags origin\n     ++\tgit -C tag-test -c protocol.version=2 fetch --tags origin\n      +'\n      +\n      +test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n     @@ -59,7 +65,7 @@\n      +\tfor i in A B C\n      +\tdo\n      +\t\ttest_commit -C src $i &&\n     -+\t\tgit -C src branch $i\n     ++\t\tgit -C src branch $i || return 1\n      +\tdone &&\n      +\tgit -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n      +\tgit -C pack-test fetch origin &&\n 2:  937a882261d ! 2:  7c4c9f0f8e1 partial-clone: avoid fetching when looking for objects\n     @@ -2,10 +2,13 @@\n      \n          partial-clone: avoid fetching when looking for objects\n      \n     -    When using partial-clone, do_oid_object_info_extended() can trigger a\n     -    fetch for missing objects. This can be extremely expensive when asking\n     -    for a tag or commit, as we are completely removed from the context of\n     -    the missing object and thus supply no \"haves\" in the request.\n     +    When using partial clone, find_non_local_tags() in builtin/fetch.c\n     +    checks each remote tag to see if its object also exists locally. There\n     +    is no expectation that the object exist locally, but this function\n     +    nevertheless triggers a lazy fetch if the object does not exist. This\n     +    can be extremely expensive when asking for a commit, as we are\n     +    completely removed from the context of the non-existent object and\n     +    thus supply no \"haves\" in the request.\n      \n          6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a\n          global variable that prevented these fetches in favor of a bitflag.\n     @@ -68,7 +71,7 @@\n       --- a/t/t5616-partial-clone.sh\n       +++ b/t/t5616-partial-clone.sh\n      @@\n     - \tgit -C tag-test fetch --tags origin\n     + \tgit -C tag-test -c protocol.version=2 fetch --tags origin\n       '\n       \n      -test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n\n-- \ngitgitgadget\n"},{"id":"392282","messageId":"04965a8c7a48b2fbb5049b640487015fdd7bbd22.1582321648.git.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":"pull.556.v2.git.1582321648.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] partial-clone: demonstrate bugs in partial fetch","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-21T21:47:27Z","receivedAt":"2020-02-21T21:47:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhile testing partial clone, I noticed some odd behavior. I was testing\na way of running 'git init', followed by manually configuring the remote\nfor partial clone, and then running 'git fetch'. Astonishingly, I saw\nthe 'git fetch' process start asking the server for multiple rounds of\npack-file downloads! When tweaking the situation a little more, I\ndiscovered that I could cause the remote to hang up with an error.\n\nAdd two tests that demonstrate these two issues.\n\nIn the first test, we find that when fetching with blob filters from\na repository that previously did not have any tags, the 'git fetch\n--tags origin' command fails because the server sends \"multiple\nfilter-specs cannot be combined\". This only happens when using\nprotocol v2.\n\nIn the second test, we see that a 'git fetch origin' request with\nseveral ref updates results in multiple pack-file downloads. This must\nbe due to Git trying to fault-in the objects pointed by the refs. What\nmakes this matter particularly nasty is that this goes through the\ndo_oid_object_info_extended() method, so there are no \"haves\" in the\nnegotiation. This leads the remote to send every reachable commit and\ntree from each new ref, providing a quadratic amount of data transfer!\nThis test is fixed if we revert 6462d5eb9a (fetch: remove\nfetch_if_missing=0, 2019-11-05), but that revert causes other test\nfailures. The real fix will need more care.\n\nThe tests are ordered in this way because if I swap the test order the\ntag test will succeed instead of fail. I believe this is because somehow\nwe need the srv.bare repo to not have any tags when we clone, but then\nhave tags in our next fetch.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t5616-partial-clone.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex fea56cda6d3..022f1018ace 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -374,6 +374,37 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n+# The following two tests must be in this order, or else\n+# the first will not fail. It is important that the srv.bare\n+# repository did not have tags during clone, but has tags\n+# in the fetch.\n+\n+test_expect_failure 'verify fetch succeeds when asking for new tags' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n+\tfor i in I J K\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i || return 1\n+\tdone &&\n+\tgit -C srv.bare fetch --tags origin +refs/heads/*:refs/heads/* &&\n+\tgit -C tag-test -c protocol.version=2 fetch --tags origin\n+'\n+\n+test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n+\tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 2 pack-list &&\n+\tfor i in A B C\n+\tdo\n+\t\ttest_commit -C src $i &&\n+\t\tgit -C src branch $i || return 1\n+\tdone &&\n+\tgit -C srv.bare fetch origin +refs/heads/*:refs/heads/* &&\n+\tgit -C pack-test fetch origin &&\n+\tls pack-test/.git/objects/pack/*pack >pack-list &&\n+\ttest_line_count = 3 pack-list\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \ngitgitgadget\n\n"},{"id":"392283","messageId":"7c4c9f0f8e1052d2ae78c6336909711126de2547.1582321648.git.gitgitgadget@gmail.com","threadId":"52841","inReplyTo":"pull.556.v2.git.1582321648.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] partial-clone: avoid fetching when looking for objects","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-21T21:47:28Z","receivedAt":"2020-02-21T21:47:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen using partial clone, find_non_local_tags() in builtin/fetch.c\nchecks each remote tag to see if its object also exists locally. There\nis no expectation that the object exist locally, but this function\nnevertheless triggers a lazy fetch if the object does not exist. This\ncan be extremely expensive when asking for a commit, as we are\ncompletely removed from the context of the non-existent object and\nthus supply no \"haves\" in the request.\n\n6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a\nglobal variable that prevented these fetches in favor of a bitflag.\nHowever, some object existence checks were not updated to use this flag.\n\nUpdate find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in\naddition to OBJECT_INFO_QUICK. The _QUICK option only prevents\nrepreparing the pack-file structures. We need to be extremely careful\nabout supplying _SKIP_FETCH_OBJECT when we expect an object to not exist\ndue to updated refs.\n\nThis resolves a broken test in t5616-partial-clone.sh.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/fetch.c          | 10 +++++-----\n t/t5616-partial-clone.sh |  2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d06..fd69c4f69d7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -335,6 +335,7 @@ static void find_non_local_tags(const struct ref *refs,\n \tstruct string_list_item *remote_ref_item;\n \tconst struct ref *ref;\n \tstruct refname_hash_entry *item = NULL;\n+\tconst int quick_flags = OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT;\n \n \trefname_hash_init(&existing_refs);\n \trefname_hash_init(&remote_refs);\n@@ -353,10 +354,9 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t */\n \t\tif (ends_with(ref->name, \"^{}\")) {\n \t\t\tif (item &&\n-\t\t\t    !has_object_file_with_flags(&ref->old_oid,\n-\t\t\t\t\t\t\tOBJECT_INFO_QUICK) &&\n+\t\t\t    !has_object_file_with_flags(&ref->old_oid, quick_flags) &&\n \t\t\t    !oidset_contains(&fetch_oids, &ref->old_oid) &&\n-\t\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t\t\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t\t\t    !oidset_contains(&fetch_oids, &item->oid))\n \t\t\t\tclear_item(item);\n \t\t\titem = NULL;\n@@ -370,7 +370,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t * fetch.\n \t\t */\n \t\tif (item &&\n-\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t\t    !oidset_contains(&fetch_oids, &item->oid))\n \t\t\tclear_item(item);\n \n@@ -391,7 +391,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t * checked to see if it needs fetching.\n \t */\n \tif (item &&\n-\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n+\t    !has_object_file_with_flags(&item->oid, quick_flags) &&\n \t    !oidset_contains(&fetch_oids, &item->oid))\n \t\tclear_item(item);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 022f1018ace..da3ad4e32a1 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -390,7 +390,7 @@ test_expect_failure 'verify fetch succeeds when asking for new tags' '\n \tgit -C tag-test -c protocol.version=2 fetch --tags origin\n '\n \n-test_expect_failure 'verify fetch downloads only one pack when updating refs' '\n+test_expect_success 'verify fetch downloads only one pack when updating refs' '\n \tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" pack-test &&\n \tls pack-test/.git/objects/pack/*pack >pack-list &&\n \ttest_line_count = 2 pack-list &&\n-- \ngitgitgadget\n"},{"id":"392302","messageId":"xmqqy2su5yal.fsf@gitster-ct.c.googlers.com","threadId":"52841","inReplyTo":"pull.556.v2.git.1582321648.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Document two partial clone bugs, fix one","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-22T17:25:54Z","receivedAt":"2020-02-22T17:26:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> While playing with partial clone, I discovered a few bugs and document them\n> with tests in patch 1. One seems to be a server-side bug that happens in a\n> somewhat rare situation, but not terribly unlikely. The other is a\n> client-side bug that leads to quadratic amounts of data transfer; I fix this\n> bug in patch 2.\n>\n> UPDATES in V2:\n>\n>  * Added \"|| return 1\" inside the for loops.\n>    \n>    \n>  * Added an in-test comment about the test ordering.\n>    \n>    \n>  * Required protocol.version=2 in the tags test due to the bisect Junio\n>    performed.\n>    \n>    \n>  * Updated the commit message via Jonathan Tan's suggestion.\n>    \n\nNow this can safely be queued directly on v2.25.0, I'll\nrebase it (earlyer I queued it after the merge to make protocol v2\nthe default).\n\nThanks.\n"}]}