{"thread":{"id":"54683","subject":"[PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","startedAt":"2020-11-20T20:36:50Z","lastAt":"2020-11-24T08:04:24Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","Jeff King","Philippe Blain","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"410440","messageId":"pull.797.git.1605904586929.gitgitgadget@gmail.com","threadId":"54683","inReplyTo":null,"subject":"[PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-20T20:36:26Z","receivedAt":"2020-11-20T20:36:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe partial clone feature has several modes, but only a few are quick\nfor a server to process using reachability bitmaps:\n\n* Blobless: --filter=blob:none downloads all commits and trees and\n  fetches necessary blobs on-demand.\n\n* Treeless: --filter=tree:0 downloads all commits and fetches necessary\n  trees and blobs on demand.\n\nThis treeles mode is most similar to a shallow clone in the total size\n(it only adds the commit objects for the full history). This makes\ntreeless clones an interesting replacement for shallow clones. A user\ncan run more commands in a treeless clone than in a shallow clone,\nespecially 'git log' (no pathspec).\n\nIn particular, servers can still serve 'git fetch' requests quickly by\ncalculating the difference between commit wants and haves using bitmaps.\n\nI was testing this feature with this in mind, and I knew that some trees\nwould be downloaded multiple times when checking out a new branch, but I\ndid not expect to discover a significant issue with 'git fetch', at\nleast in repostiories with submodules.\n\nI was testing these commands:\n\n\t$ git clone --filter=tree:0 --single-branch --branch=master \\\n\t  https://github.com/git/git\n\t$ git -C git fetch origin \"+refs/heads/*:refs/remotes/origin/*\"\n\nThis fetch command started downloading several pack-files of trees\nbefore completing the command. I never let it finish since I got so\nimpatient with the repeated downloads. During debugging, I found that\nthe stack triggering promisor_remote_get_direct() was going through\nfetch_populated_submodules(). Notice that I did not recurse my\nsubmodules in the original clone, so the sha1collisiondetection\nsubmodule is not initialized. Even so, my 'git fetch' was scanning\ncommits for updates to submodules.\n\nI decided that even if I did populate the submodules, the nature of\ntreeless clones makes me not want to care about the contents of commits\nother than those that I am explicitly navigating to.\n\nThis loop of tree fetches can be avoided by adding\n--no-recurse-submodules to the 'git fetch' command or setting\nfetch.recurseSubmodules=no.\n\nTo make this as painless as possible for future users of treeless\nclones, automatically set fetch.recurseSubmodules=no at clone time.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n    clone: --filter=tree:0 implies fetch.recurseSubmodules=no\n    \n    While testing different partial clone options, I stumbled across this\n    one. My initial thought was that we were parsing commits and loading\n    their root trees unnecessarily, but I see that doesn't happen after this\n    change.\n    \n    Here are some recent discussions about using --filter=tree:0:\n    \n    [1] \n    https://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n    [2] https://lore.kernel.org/git/cover.1588633810.git.me@ttaylorr.com/[3] \n    https://lore.kernel.org/git/58274817-7ac6-b6ae-0d10-22485dfe5e0e@syntevo.com/\n    \n    Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-797%2Fderrickstolee%2Ftree-0-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-797/derrickstolee/tree-0-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/797\n\n list-objects-filter-options.c | 4 ++++\n t/t5616-partial-clone.sh      | 6 ++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex defd3dfd10..249939dfa5 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -376,6 +376,10 @@ void partial_clone_register(\n \t\t       expand_list_objects_filter_spec(filter_options));\n \tfree(filter_name);\n \n+\tif (filter_options->choice == LOFC_TREE_DEPTH &&\n+\t    !filter_options->tree_exclude_depth)\n+\t\tgit_config_set(\"fetch.recursesubmodules\", \"no\");\n+\n \t/* Make sure the config info are reset */\n \tpromisor_remote_reinit();\n }\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex f4d49d8335..b2eaf78069 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -341,6 +341,12 @@ test_expect_success 'partial clone with sparse filter succeeds' '\n \t)\n '\n \n+test_expect_success '--filter=tree:0 sets fetch.recurseSubmodules=no' '\n+\trm -rf dst &&\n+\tgit clone --filter=tree:0 \"file://$(pwd)/src\" dst &&\n+\ttest_config -C dst fetch.recursesubmodules no\n+'\n+\n test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '\n \trm -rf dst.git &&\n \ttest_must_fail git clone --no-local --bare \\\n\nbase-commit: faefdd61ec7c7f6f3c8c9907891465ac9a2a1475\n-- \ngitgitgadget\n"},{"id":"410447","messageId":"20201121000436.GB353076@coredump.intra.peff.net","threadId":"54683","inReplyTo":"pull.797.git.1605904586929.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-21T00:04:36Z","receivedAt":"2020-11-21T00:04:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 20, 2020 at 08:36:26PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> I decided that even if I did populate the submodules, the nature of\n> treeless clones makes me not want to care about the contents of commits\n> other than those that I am explicitly navigating to.\n> \n> This loop of tree fetches can be avoided by adding\n> --no-recurse-submodules to the 'git fetch' command or setting\n> fetch.recurseSubmodules=no.\n> \n> To make this as painless as possible for future users of treeless\n> clones, automatically set fetch.recurseSubmodules=no at clone time.\n\nI think it's definitely worth adjusting the defaults here to make this\nless painful out of the box. But I wonder if this is too big a hammer,\nand the on-demand logic just needs to be a bit less aggressive.\n\nIf I clone with tree:0, I'm still going to get the tree for the thing\nI'm actually checking out (assuming a non-bare repo). It would be\nreasonable to recursively fetch the submodules in that commit to\ncheck them out (assuming you specified --recurse-submodules).\n\nIf I then fetch again, I'll end up with another tree that I'm about to\ncheckout. So likewise, would it make sense to fetch any updates from\nthere?\n\nI.e., I think the problem is that whether or not cared about submodules\nin the first place, the default \"on-demand\" setting of fetch.submodules\nis very eager to poke through history looking at the .gitmodules file to\nsee if there is anything worth also fetching. But:\n\n  - if we know there are no active submodules in the first place\n    (because you did not say --recurse-submodules), should it skip that\n    poking? That seems like it shouldn't be too hard.\n\n  - during its poking, should it set the necessary variables so that it\n    never demand-fetches from a promisor remote? I suspect this part\n    may be hard, because \"fetch\" and \"checkout\" are distinct operations\n    (so during the \"fetch\" we don't yet have the new tree demand-fetched\n    by checkout; in fact the user might not even be interested in\n    checking it out yet).\n\nGiven the difficulties in the latter case, this may be the best we can\ndo. But in that case, what happens when we _do_ care about submodules,\nand do:\n\n  git clone --recurse-submodules --filter=tree:0 ...\n  git fetch\n  git merge origin\n\nWill we correctly fetch-on-demand the submodules we need during the\nmerge operation? If so, then that user experience is probably pretty\nreasonable.\n\n-Peff\n"},{"id":"410460","messageId":"e539892e-743a-96d7-a540-b7f0af22cbe1@gmail.com","threadId":"54683","inReplyTo":"pull.797.git.1605904586929.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-11-21T16:19:00Z","receivedAt":"2020-11-21T16:19:05Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Stolee,\n\nOn 20-11-20 15 h 36, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The partial clone feature has several modes, but only a few are quick\n> for a server to process using reachability bitmaps:\n>\n> * Blobless: --filter=blob:none downloads all commits and trees and\n>   fetches necessary blobs on-demand.\n>\n> * Treeless: --filter=tree:0 downloads all commits and fetches necessary\n>   trees and blobs on demand.\n>\n> This treeles mode is most similar to a shallow clone in the total size\n> (it only adds the commit objects for the full history). This makes\n> treeless clones an interesting replacement for shallow clones. A user\n> can run more commands in a treeless clone than in a shallow clone,\n> especially 'git log' (no pathspec).\n>\n> In particular, servers can still serve 'git fetch' requests quickly by\n> calculating the difference between commit wants and haves using bitmaps.\n>\n> I was testing this feature with this in mind, and I knew that some trees\n> would be downloaded multiple times when checking out a new branch, but I\n> did not expect to discover a significant issue with 'git fetch', at\n> least in repostiories with submodules.\n>\n> I was testing these commands:\n>\n> \t$ git clone --filter=tree:0 --single-branch --branch=master \\\n> \t  https://github.com/git/git\n> \t$ git -C git fetch origin \"+refs/heads/*:refs/remotes/origin/*\"\n>\n> This fetch command started downloading several pack-files of trees\n> before completing the command. I never let it finish since I got so\n> impatient with the repeated downloads. During debugging, I found that\n> the stack triggering promisor_remote_get_direct() was going through\n> fetch_populated_submodules(). Notice that I did not recurse my\n> submodules in the original clone, so the sha1collisiondetection\n> submodule is not initialized. Even so, my 'git fetch' was scanning\n> commits for updates to submodules.\n\nI'm not super familiar with the inner workings\noffetch_populated_submodules(), but is seems weird that this function\ndoes something in that case. It should do nothing, as the submodule is\nnot populated. Maybe it would be worth it to investigate what exactly is\nhappening?\n\n> I decided that even if I did populate the submodules, the nature of\n> treeless clones makes me not want to care about the contents of commits\n> other than those that I am explicitly navigating to.\n>\n> This loop of tree fetches can be avoided by adding\n> --no-recurse-submodules to the 'git fetch' command or setting\n> fetch.recurseSubmodules=no.\n>\n> To make this as painless as possible for future users of treeless\n> clones, automatically set fetch.recurseSubmodules=no at clone time.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>     clone: --filter=tree:0 implies fetch.recurseSubmodules=no\n>     \n>     While testing different partial clone options, I stumbled across this\n>     one. My initial thought was that we were parsing commits and loading\n>     their root trees unnecessarily, but I see that doesn't happen after this\n>     change.\n>     \n>     Here are some recent discussions about using --filter=tree:0:\n>     \n>     [1] \n>     https://lore.kernel.org/git/aa7b89ee-08aa-7943-6a00-28dcf344426e@syntevo.com/\n>     [2] https://lore.kernel.org/git/cover.1588633810.git.me@ttaylorr.com/[3] \n>     https://lore.kernel.org/git/58274817-7ac6-b6ae-0d10-22485dfe5e0e@syntevo.com/\n>     \n>     Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-797%2Fderrickstolee%2Ftree-0-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-797/derrickstolee/tree-0-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/797\n>\n>  list-objects-filter-options.c | 4 ++++\n>  t/t5616-partial-clone.sh      | 6 ++++++\n>  2 files changed, 10 insertions(+)\n\nIn any case I think such a change would also need a doc update, probably\nin Documentation/fetch-options.txt and Documentation/config/fetch.txt.\n\nCheers,\n\nPhilippe.\n\n"},{"id":"410530","messageId":"8a31af0e-4440-d957-11fb-48c4d2abd5c7@gmail.com","threadId":"54683","inReplyTo":"20201121000436.GB353076@coredump.intra.peff.net","subject":"Re: [PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-11-23T15:18:42Z","receivedAt":"2020-11-23T15:18:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/20/2020 7:04 PM, Jeff King wrote:\n> On Fri, Nov 20, 2020 at 08:36:26PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> I decided that even if I did populate the submodules, the nature of\n>> treeless clones makes me not want to care about the contents of commits\n>> other than those that I am explicitly navigating to.\n>>\n>> This loop of tree fetches can be avoided by adding\n>> --no-recurse-submodules to the 'git fetch' command or setting\n>> fetch.recurseSubmodules=no.\n>>\n>> To make this as painless as possible for future users of treeless\n>> clones, automatically set fetch.recurseSubmodules=no at clone time.\n> \n> I think it's definitely worth adjusting the defaults here to make this\n> less painful out of the box. But I wonder if this is too big a hammer,\n> and the on-demand logic just needs to be a bit less aggressive.\n\nYes, you are probably right.\n\nI really should have labeled this patch as \"RFC\" as that was my intent.\n\n> If I clone with tree:0, I'm still going to get the tree for the thing\n> I'm actually checking out (assuming a non-bare repo). It would be\n> reasonable to recursively fetch the submodules in that commit to\n> check them out (assuming you specified --recurse-submodules).\n> \n> If I then fetch again, I'll end up with another tree that I'm about to\n> checkout. So likewise, would it make sense to fetch any updates from\n> there?\n\nOne thing that is different is that we will fetch the trees we need\n_during that checkout_, so why do it preemptively in the fetch?\n\nFurther, the number of trees being fetched is _not_ the number of ref\ntips, but seems to be related to the total number of commits different.\nThere must be a rev walk looking for which commits changed the modules\nfile or the commit link in the root tree.\n\n> I.e., I think the problem is that whether or not cared about submodules\n> in the first place, the default \"on-demand\" setting of fetch.submodules\n> is very eager to poke through history looking at the .gitmodules file to\n> see if there is anything worth also fetching. But:\n> \n>   - if we know there are no active submodules in the first place\n>     (because you did not say --recurse-submodules), should it skip that\n>     poking? That seems like it shouldn't be too hard.\n\nThis is a good direction to pursue.\n\n>   - during its poking, should it set the necessary variables so that it\n>     never demand-fetches from a promisor remote? I suspect this part\n>     may be hard, because \"fetch\" and \"checkout\" are distinct operations\n>     (so during the \"fetch\" we don't yet have the new tree demand-fetched\n>     by checkout; in fact the user might not even be interested in\n>     checking it out yet).\n\nI also think this is a good idea. In particular, should we consider\nmaking the \"submodule fetch\" be part of the \"promisor\"? That is, we\nonly fetch our submodule on checkout? Can we assume that the commit\nwill still exist on the remote, to some extent?\n\nNaturally, this only really applies for --filter=tree:0, since in cases\nlike blobless clones, we would still want the commits and trees from the\nsubmodule.\n\n> Given the difficulties in the latter case, this may be the best we can\n> do. But in that case, what happens when we _do_ care about submodules,\n> and do:\n> \n>   git clone --recurse-submodules --filter=tree:0 ...\n>   git fetch\n>   git merge origin\n> \n> Will we correctly fetch-on-demand the submodules we need during the\n> merge operation? If so, then that user experience is probably pretty\n> reasonable.\n\nHopefully. Notably, the filter option does _not_ apply recursively\nto the submodules, so even if we try to make the superproject a partial\nclone, the submodule is not partial.\n\nMore research is required. Let's drop this patch, as I don't currently\nhave time to do the necessary deep dive. If someone else has time to\nlook into this, I'd be happy to review a better patch.\n\nThanks,\n-Stolee\n\n\n"},{"id":"410644","messageId":"X7y+cZ3MPqqmGXx/@coredump.intra.peff.net","threadId":"54683","inReplyTo":"8a31af0e-4440-d957-11fb-48c4d2abd5c7@gmail.com","subject":"Re: [PATCH] clone: --filter=tree:0 implies fetch.recurseSubmodules=no","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-24T08:04:01Z","receivedAt":"2020-11-24T08:04:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 23, 2020 at 10:18:42AM -0500, Derrick Stolee wrote:\n\n> > If I clone with tree:0, I'm still going to get the tree for the thing\n> > I'm actually checking out (assuming a non-bare repo). It would be\n> > reasonable to recursively fetch the submodules in that commit to\n> > check them out (assuming you specified --recurse-submodules).\n> > \n> > If I then fetch again, I'll end up with another tree that I'm about to\n> > checkout. So likewise, would it make sense to fetch any updates from\n> > there?\n> \n> One thing that is different is that we will fetch the trees we need\n> _during that checkout_, so why do it preemptively in the fetch?\n> \n> Further, the number of trees being fetched is _not_ the number of ref\n> tips, but seems to be related to the total number of commits different.\n> There must be a rev walk looking for which commits changed the modules\n> file or the commit link in the root tree.\n\nYeah, that's exactly what the \"on-demand\" thing is. I said before it was\nlooking for .gitmodules, but I think it has to be looking for updated\ngitlinks. I'm pretty sure the logic only kicks in if we have a\n.gitmodules, though, to avoid the extra traversal when you're not using\nsubmodules (but I guess we're all now paying that traversal cost in\ngit.git, even if we don't populate the modules).\n\nI think the goal is to do all of the fetches at once, so after \"git\nfetch\" you can then run checkout, merge, etc, without worrying that\nyou'll need network access later.\n\nBut it's not actually \"checkout\" that does the fetching. It will update\nthe gitlink, but that will just show a diff against the content of the\nsubmodule. You have to actually \"git submodule update\" to update the\nrepository, which will then auto-fetch (though I suspect with\nappropriate config or command-line options, checkout can be convinced to\ntrigger \"submodule update\").\n\nSo in that sense, maybe your patch is the most sensible thing. If you're\nnot checking out immediately, we probably have no business in a partial\nclone guessing at trees you _might_ check out later, especially if it\ninvolves demand-fetching them from the server. And if you do check out\nand \"submodule update\" immediately, then we'd do the fetch then anyway.\n\nWe can punt on the logic to walk the trees, only looking at ones we\nactually _do_ have locally, until somebody else wants to work on it.\n\n> >   - during its poking, should it set the necessary variables so that it\n> >     never demand-fetches from a promisor remote? I suspect this part\n> >     may be hard, because \"fetch\" and \"checkout\" are distinct operations\n> >     (so during the \"fetch\" we don't yet have the new tree demand-fetched\n> >     by checkout; in fact the user might not even be interested in\n> >     checking it out yet).\n> \n> I also think this is a good idea. In particular, should we consider\n> making the \"submodule fetch\" be part of the \"promisor\"? That is, we\n> only fetch our submodule on checkout? Can we assume that the commit\n> will still exist on the remote, to some extent?\n\nI think issues of whether the commit will exist are outside the scope\nhere. \"submodule update\" has to decide if we have it, or how to get it\n(if it can). But it won't fail a checkout in that case; you'll just be\nleft with a diff between the gitlink and what's in the submodule repo.\n\nI'm not quite sure what you're asking with the rest of it. From this:\n\n> Naturally, this only really applies for --filter=tree:0, since in cases\n> like blobless clones, we would still want the commits and trees from the\n> submodule.\n\nit sounds like you're asking whether the submodule should also be using\nthe same filter. Maybe, but I think it would depend on the workflow and\nthe module (and probably is something that you'd want to be able to\nconfigure independently). At any rate, I think that's orthogonal to this\nissue.\n\n> > Given the difficulties in the latter case, this may be the best we can\n> > do. But in that case, what happens when we _do_ care about submodules,\n> > and do:\n> > \n> >   git clone --recurse-submodules --filter=tree:0 ...\n> >   git fetch\n> >   git merge origin\n> > \n> > Will we correctly fetch-on-demand the submodules we need during the\n> > merge operation? If so, then that user experience is probably pretty\n> > reasonable.\n\nAnswering my own question: no, you'd run \"git submodule update\"\nafterwards.\n\n> Hopefully. Notably, the filter option does _not_ apply recursively\n> to the submodules, so even if we try to make the superproject a partial\n> clone, the submodule is not partial.\n\nYep, but I do think that's orthogonal.\n\n> More research is required. Let's drop this patch, as I don't currently\n> have time to do the necessary deep dive. If someone else has time to\n> look into this, I'd be happy to review a better patch.\n\nI don't mind dropping it, but I've actually come around to the idea that\nyour patch or something like it is probably a strict improvement.\n\n-Peff\n"}]}