{"thread":{"id":"60742","subject":"[PATCH] clone: support cloning of filtered bundles","startedAt":"2024-01-14T11:16:53Z","lastAt":"2024-01-16T20:06:52Z","messageCount":9,"participants":["Nikolay Edigaryev via GitGitGadget","Phillip Wood","Nikolay Edigaryev","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486775","messageId":"pull.1644.git.git.1705231010118.gitgitgadget@gmail.com","threadId":"60742","inReplyTo":null,"subject":"[PATCH] clone: support cloning of filtered bundles","fromName":"Nikolay Edigaryev via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-14T11:16:49Z","receivedAt":"2024-01-14T11:16:53Z","isPatch":true,"sender":{"key":"edigaryev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/85709?v=4"},"body":"From: Nikolay Edigaryev \n\nf18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\nincredibly useful ability to create filtered bundles, which advances\nthe partial clone/promisor support in Git and allows for archiving\nlarge repositories to object storages like S3 in bundles that are:\n\n* easy to manage\n * bundle is just a single file, it's easier to guarantee atomic\n replacements in object storages like S3 and they are faster to\n fetch than a bare repository since there's only a single GET\n request involved\n* incredibly tiny\n * no indexes (which may be more than 10 MB for some repositories)\n and other fluff, compared to cloning a bare repository\n * bundle can be filtered to only contain the tips of refs neccessary\n for e.g. code-analysis purposes\n\nHowever, in 86fdd94d72 (clone: fail gracefully when cloning filtered\nbundle, 2022-03-09) the cloning of such bundles was disabled, with a\nnote that this behavior is not desired, and it the long-term this\nshould be possible.\n\nThe commit above states that it's not possible to have this at the\nmoment due to lack of remote and a repository-global config that\nspecifies an object filter, yet it's unclear why a remote-specific\nconfig can't be used instead, which is what this change does.\n\nSigned-off-by: Nikolay Edigaryev \n---\n clone: support cloning of filtered bundles\n \n f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n incredibly useful ability to create filtered bundles, which advances the\n partial clone/promisor support in Git and allows for archiving large\n repositories to object storages like S3 in bundles that are:\n \n * easy to manage\n * bundle is just a single file, it's easier to guarantee atomic\n replacements in object storages like S3 and they are faster to\n fetch than a bare repository since there's only a single GET\n request involved\n * incredibly tiny\n * no indexes (which may be more than 10 MB for some repositories) and\n other fluff, compared to cloning a bare repository\n * bundle can be filtered to only contain the tips of refs neccessary\n for e.g. code-analysis purposes\n \n However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n note that this behavior is not desired, and it the long-term this should\n be possible.\n \n The commit above states that it's not possible to have this at the\n moment due to lack of remote and a repository-global config that\n specifies an object filter, yet it's unclear why a remote-specific\n config can't be used instead, which is what this change does.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1644%2Fedigaryev%2Fclone-filtered-bundles-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1644/edigaryev/clone-filtered-bundles-v1\nPull-Request: https://github.com/git/git/pull/1644\n\n builtin/clone.c | 13 +++++++++++--\n t/t6020-bundle-misc.sh | 13 +++----------\n 2 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af9498..4b3fedf78ed 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tif (fd > 0)\n \t\t\tclose(fd);\n+\n+\t\tif (has_filter) {\n+\t\t\tstrbuf_addf(&key, \"remote.%s.promisor\", remote_name);\n+\t\t\tgit_config_set(key.buf, \"true\");\n+\t\t\tstrbuf_reset(&key);\n+\n+\t\t\tstrbuf_addf(&key, \"remote.%s.partialclonefilter\", remote_name);\n+\t\t\tgit_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));\n+\t\t\tstrbuf_reset(&key);\n+\t\t}\n+\n \t\tbundle_header_release(&header);\n-\t\tif (has_filter)\n-\t\t\tdie(_(\"cannot clone from filtered bundle\"));\n \t}\n \n \ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 3e6bcbf30cd..f449df00642 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -555,16 +555,9 @@ do\n \t'\n done\n \n-# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered\n-# bundle, but that requires a change to promisor/filter config options.\n-# For now, we fail gracefully with a helpful error. This behavior can be\n-# changed in the future to succeed as much as possible.\n-test_expect_success 'cloning from filtered bundle has useful error' '\n-\tgit bundle create partial.bdl \\\n-\t\t--all \\\n-\t\t--filter=blob:none &&\n-\ttest_must_fail git clone --bare partial.bdl partial 2>err &&\n-\tgrep \"cannot clone from filtered bundle\" err\n+test_expect_success 'cloning from filtered bundle works' '\n+\tgit bundle create partial.bdl --all --filter=blob:none &&\n+\tgit clone --bare partial.bdl partial 2>err\n '\n \n test_expect_success 'verify catches unreachable, broken prerequisites' '\n\nbase-commit: 564d0252ca632e0264ed670534a51d18a689ef5d\n-- \ngitgitgadget\n"},{"id":"486776","messageId":"24b82309-34e9-49a0-983b-7e94dad3d06b@gmail.com","threadId":"60742","inReplyTo":"pull.1644.git.git.1705231010118.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-14T18:00:52Z","receivedAt":"2024-01-14T18:00:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Nikolay\n\nOn 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:\n> From: Nikolay Edigaryev \n> \n> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n> incredibly useful ability to create filtered bundles, which advances\n> the partial clone/promisor support in Git and allows for archiving\n> large repositories to object storages like S3 in bundles that are:\n> \n> * easy to manage\n> * bundle is just a single file, it's easier to guarantee atomic\n> replacements in object storages like S3 and they are faster to\n> fetch than a bare repository since there's only a single GET\n> request involved\n> * incredibly tiny\n> * no indexes (which may be more than 10 MB for some repositories)\n> and other fluff, compared to cloning a bare repository\n> * bundle can be filtered to only contain the tips of refs neccessary\n> for e.g. code-analysis purposes\n> \n> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n> bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n> note that this behavior is not desired, and it the long-term this\n> should be possible.\n> \n> The commit above states that it's not possible to have this at the\n> moment due to lack of remote and a repository-global config that\n> specifies an object filter, yet it's unclear why a remote-specific\n> config can't be used instead, which is what this change does.\n\nAs I understand it if you're cloning from a bundle file then then there \nis no remote so how can we set a remote-specific config?\n\nI'm surprised that the proposed change does not require the user to pass \n\"--filter\" to \"git clone\" as I expected that we'd want to check that the \nfilter on the command line was compatible with the filter used to create \nthe bundle. Allowing \"git clone\" to create a partial clone without the \nuser asking for it by passing the \"--filter\" option feels like is going \nto end up confusing users.\n\n> +test_expect_success 'cloning from filtered bundle works' '\n> +\tgit bundle create partial.bdl --all --filter=blob:none &&\n> +\tgit clone --bare partial.bdl partial 2>err\n\nThe redirection hides any error message which will make debugging test \nfailures harder. It would be nice to see this test check any config set \nwhen cloning and that git commands can run successfully in the repository.\n\nBest Wishes\n\nPhillip\n"},{"id":"486778","messageId":"CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com","threadId":"60742","inReplyTo":"24b82309-34e9-49a0-983b-7e94dad3d06b@gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Nikolay Edigaryev","fromEmail":"edigaryev@gmail.com","sentAt":"2024-01-14T19:39:40Z","receivedAt":"2024-01-14T19:39:52Z","isPatch":true,"sender":{"key":"edigaryev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/85709?v=4"},"body":"Hello Phillip,\n\n> As I understand it if you're cloning from a bundle file then then there\n> is no remote so how can we set a remote-specific config?\n\nThere is a remote, for more details see df61c88979 (clone: also\nconfigure url for bare clones, 2010-03-29), which has the following\ncode:\n\nstrbuf_addf(&key, \"remote.%s.url\", remote_name);\ngit_config_set(key.buf, repo);\nstrbuf_reset(&key);\n\nYou can verify this by creating a bundle on Git 2.43.0 with \"git create\nbundle bundle.bundle --all\" and then cloning it with \"git clone\n--bare /path/to/bundle.bundle\", in my case the following repo-wide\nconfiguration file was created:\n\n[core]\nrepositoryformatversion = 0\nfilemode = true\nbare = true\nignorecase = true\nprecomposeunicode = true\n[remote \"origin\"]\nurl = /Users/edi/src/cirrus-cli/cli.bundle\n\n> I'm surprised that the proposed change does not require the user to pass\n> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n> filter on the command line was compatible with the filter used to create\n> the bundle. Allowing \"git clone\" to create a partial clone without the\n> user asking for it by passing the \"--filter\" option feels like is going\n> to end up confusing users.\n\nNote that currently, when you clone a normal non-filtered bundle with a\n'--filter' argument specified, no filtering will take place and no error\nwill be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\nwill be set in the repo config, but no .promisor file will be created.\nThis is even more confusing IMO, but that's how it currently on\nGit 2.43.0.\n\nYou have a good point, but I feel like completely preventing cloning of\nfiltered bundles and requiring a '--filter' argument is very taxing. If\nyou've already specified a '--filter' when creating a bundle (and thus\nyour intent to use partially cloned data), why do it multiple times?\n\nWhat I propose as an alternative here is to act based on the user's\nintent when cloning:\n\n* when the user specifies no '--filter' argument, do nothing special,\n allow cloning both types of bundles: normal and filtered (with the\n logic from this patch)\n\n* when the user does specify a '--filter' argument, either:\n * throw an error explaining that filtering of filtered bundles is not\n supported\n * or compare the user's filter specification and the one that is\n in the bundle and only throw an error if they mismatch\n\nLet me know what you think about this (and perhaps you have a more\nconcrete example in mind where this will have negative consequences)\nand I'll be happy to do a next iteration.\n\n\nOn Sun, Jan 14, 2024 at 10:00 PM Phillip Wood wrote:\n>\n> Hi Nikolay\n>\n> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:\n> > From: Nikolay Edigaryev \n> >\n> > f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n> > incredibly useful ability to create filtered bundles, which advances\n> > the partial clone/promisor support in Git and allows for archiving\n> > large repositories to object storages like S3 in bundles that are:\n> >\n> > * easy to manage\n> > * bundle is just a single file, it's easier to guarantee atomic\n> > replacements in object storages like S3 and they are faster to\n> > fetch than a bare repository since there's only a single GET\n> > request involved\n> > * incredibly tiny\n> > * no indexes (which may be more than 10 MB for some repositories)\n> > and other fluff, compared to cloning a bare repository\n> > * bundle can be filtered to only contain the tips of refs neccessary\n> > for e.g. code-analysis purposes\n> >\n> > However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n> > bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n> > note that this behavior is not desired, and it the long-term this\n> > should be possible.\n> >\n> > The commit above states that it's not possible to have this at the\n> > moment due to lack of remote and a repository-global config that\n> > specifies an object filter, yet it's unclear why a remote-specific\n> > config can't be used instead, which is what this change does.\n>\n> As I understand it if you're cloning from a bundle file then then there\n> is no remote so how can we set a remote-specific config?\n>\n> I'm surprised that the proposed change does not require the user to pass\n> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n> filter on the command line was compatible with the filter used to create\n> the bundle. Allowing \"git clone\" to create a partial clone without the\n> user asking for it by passing the \"--filter\" option feels like is going\n> to end up confusing users.\n>\n> > +test_expect_success 'cloning from filtered bundle works' '\n> > + git bundle create partial.bdl --all --filter=blob:none &&\n> > + git clone --bare partial.bdl partial 2>err\n>\n> The redirection hides any error message which will make debugging test\n> failures harder. It would be nice to see this test check any config set\n> when cloning and that git commands can run successfully in the repository.\n>\n> Best Wishes\n>\n> Phillip\n"},{"id":"486781","messageId":"CAFX5hXQ291gR4831dtRTdvtffWefCNCFp13ADvOkU7s3SVPczQ@mail.gmail.com","threadId":"60742","inReplyTo":"CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Nikolay Edigaryev","fromEmail":"edigaryev@gmail.com","sentAt":"2024-01-14T21:26:53Z","receivedAt":"2024-01-14T21:27:05Z","isPatch":true,"sender":{"key":"edigaryev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/85709?v=4"},"body":"> Note that currently, when you clone a normal non-filtered bundle with a\n> '--filter' argument specified, no filtering will take place and no error\n> will be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\n> will be set in the repo config, but no .promisor file will be created.\n> This is even more confusing IMO, but that's how it currently on\n> Git 2.43.0.\n\nI was wrong about this one: the .promisor pack file actually gets created.\n\nAnd the filtering is not being done because the \"uploadpack.allowFilter\"\nglobal option is not enabled by default.\n\nUnfortunately the only way to figure this out is to run Git with\nGIT_TRACE=2, which shows:\n\nwarning: filtering not recognized by server, ignoring\n\nSo please feel free to skip this part from the consideration.\n\n\nOn Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev wrote:\n>\n> Hello Phillip,\n>\n> > As I understand it if you're cloning from a bundle file then then there\n> > is no remote so how can we set a remote-specific config?\n>\n> There is a remote, for more details see df61c88979 (clone: also\n> configure url for bare clones, 2010-03-29), which has the following\n> code:\n>\n> strbuf_addf(&key, \"remote.%s.url\", remote_name);\n> git_config_set(key.buf, repo);\n> strbuf_reset(&key);\n>\n> You can verify this by creating a bundle on Git 2.43.0 with \"git create\n> bundle bundle.bundle --all\" and then cloning it with \"git clone\n> --bare /path/to/bundle.bundle\", in my case the following repo-wide\n> configuration file was created:\n>\n> [core]\n> repositoryformatversion = 0\n> filemode = true\n> bare = true\n> ignorecase = true\n> precomposeunicode = true\n> [remote \"origin\"]\n> url = /Users/edi/src/cirrus-cli/cli.bundle\n>\n> > I'm surprised that the proposed change does not require the user to pass\n> > \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n> > filter on the command line was compatible with the filter used to create\n> > the bundle. Allowing \"git clone\" to create a partial clone without the\n> > user asking for it by passing the \"--filter\" option feels like is going\n> > to end up confusing users.\n>\n> Note that currently, when you clone a normal non-filtered bundle with a\n> '--filter' argument specified, no filtering will take place and no error\n> will be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\n> will be set in the repo config, but no .promisor file will be created.\n> This is even more confusing IMO, but that's how it currently on\n> Git 2.43.0.\n>\n> You have a good point, but I feel like completely preventing cloning of\n> filtered bundles and requiring a '--filter' argument is very taxing. If\n> you've already specified a '--filter' when creating a bundle (and thus\n> your intent to use partially cloned data), why do it multiple times?\n>\n> What I propose as an alternative here is to act based on the user's\n> intent when cloning:\n>\n> * when the user specifies no '--filter' argument, do nothing special,\n> allow cloning both types of bundles: normal and filtered (with the\n> logic from this patch)\n>\n> * when the user does specify a '--filter' argument, either:\n> * throw an error explaining that filtering of filtered bundles is not\n> supported\n> * or compare the user's filter specification and the one that is\n> in the bundle and only throw an error if they mismatch\n>\n> Let me know what you think about this (and perhaps you have a more\n> concrete example in mind where this will have negative consequences)\n> and I'll be happy to do a next iteration.\n>\n>\n> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood wrote:\n> >\n> > Hi Nikolay\n> >\n> > On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:\n> > > From: Nikolay Edigaryev \n> > >\n> > > f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n> > > incredibly useful ability to create filtered bundles, which advances\n> > > the partial clone/promisor support in Git and allows for archiving\n> > > large repositories to object storages like S3 in bundles that are:\n> > >\n> > > * easy to manage\n> > > * bundle is just a single file, it's easier to guarantee atomic\n> > > replacements in object storages like S3 and they are faster to\n> > > fetch than a bare repository since there's only a single GET\n> > > request involved\n> > > * incredibly tiny\n> > > * no indexes (which may be more than 10 MB for some repositories)\n> > > and other fluff, compared to cloning a bare repository\n> > > * bundle can be filtered to only contain the tips of refs neccessary\n> > > for e.g. code-analysis purposes\n> > >\n> > > However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n> > > bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n> > > note that this behavior is not desired, and it the long-term this\n> > > should be possible.\n> > >\n> > > The commit above states that it's not possible to have this at the\n> > > moment due to lack of remote and a repository-global config that\n> > > specifies an object filter, yet it's unclear why a remote-specific\n> > > config can't be used instead, which is what this change does.\n> >\n> > As I understand it if you're cloning from a bundle file then then there\n> > is no remote so how can we set a remote-specific config?\n> >\n> > I'm surprised that the proposed change does not require the user to pass\n> > \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n> > filter on the command line was compatible with the filter used to create\n> > the bundle. Allowing \"git clone\" to create a partial clone without the\n> > user asking for it by passing the \"--filter\" option feels like is going\n> > to end up confusing users.\n> >\n> > > +test_expect_success 'cloning from filtered bundle works' '\n> > > + git bundle create partial.bdl --all --filter=blob:none &&\n> > > + git clone --bare partial.bdl partial 2>err\n> >\n> > The redirection hides any error message which will make debugging test\n> > failures harder. It would be nice to see this test check any config set\n> > when cloning and that git commands can run successfully in the repository.\n> >\n> > Best Wishes\n> >\n> > Phillip\n"},{"id":"486787","messageId":"xmqqcyu35rg9.fsf@gitster.g","threadId":"60742","inReplyTo":"pull.1644.git.git.1705231010118.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-15T01:13:42Z","receivedAt":"2024-01-15T01:18:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nikolay Edigaryev via GitGitGadget\" \nwrites:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index c6357af9498..4b3fedf78ed 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \n> \t\tif (fd > 0)\n> \t\t\tclose(fd);\n> +\n> +\t\tif (has_filter) {\n> +\t\t\tstrbuf_addf(&key, \"remote.%s.promisor\", remote_name);\n> +\t\t\tgit_config_set(key.buf, \"true\");\n> +\t\t\tstrbuf_reset(&key);\n> +\n> +\t\t\tstrbuf_addf(&key, \"remote.%s.partialclonefilter\", remote_name);\n> +\t\t\tgit_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));\n> +\t\t\tstrbuf_reset(&key);\n> +\t\t}\n> +\n\n> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered\n> -# bundle, but that requires a change to promisor/filter config options.\n\nSorry, but this \"should be able to\" does not make sense to me in the\nfirst place.\n\nI can understand how an operation to create a narrow clone of an\nunfiltered bundle and then prepare the resulting repository for\nfuture \"fattening\" by naming the unfiltered bundle file a\n\"promisor\", and allow the user to lazily fetch objects that have\ninitially been filtered out of the bundle lazily.\n\nBut a bundle that were created with objects _omitted_ already? It\nobviously cannot \"promise\" to deliber any objects that ought to be\nreachable from the objects contained in it, so setting the bundle\nfile as promisor in the configuration does not help the resulting\nrepository. Those missing objects must be obtained from somewhere\nother than that original \"filtered\" bundle, and if that source of\nobjects that can promise all the objects that are ought to be\nreachable were specified as the promisor, it would make sense. But\nthe source of this clone operation, i.e. the bundle file that is\npointed at by \"remote.$remote_name.url\", cannot be that promisor.\n"},{"id":"486788","messageId":"xmqq8r4r5ovf.fsf@gitster.g","threadId":"60742","inReplyTo":"xmqqcyu35rg9.fsf@gitster.g","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-15T02:09:24Z","receivedAt":"2024-01-15T02:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Nikolay Edigaryev via GitGitGadget\" \n> writes:\n>\n>> diff --git a/builtin/clone.c b/builtin/clone.c\n>> index c6357af9498..4b3fedf78ed 100644\n>> --- a/builtin/clone.c\n>> +++ b/builtin/clone.c\n>> @@ -1227,9 +1227,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>> \n>> \t\tif (fd > 0)\n>> \t\t\tclose(fd);\n>> +\n>> +\t\tif (has_filter) {\n>> +\t\t\tstrbuf_addf(&key, \"remote.%s.promisor\", remote_name);\n>> +\t\t\tgit_config_set(key.buf, \"true\");\n>> +\t\t\tstrbuf_reset(&key);\n>> +\n>> +\t\t\tstrbuf_addf(&key, \"remote.%s.partialclonefilter\", remote_name);\n>> +\t\t\tgit_config_set(key.buf, expand_list_objects_filter_spec(&header.filter));\n>> +\t\t\tstrbuf_reset(&key);\n>> +\t\t}\n>> +\n>\n>> -# NEEDSWORK: 'git clone --bare' should be able to clone from a filtered\n>> -# bundle, but that requires a change to promisor/filter config options.\n> ...\n> But a bundle that were created with objects _omitted_ already?\n> ... the source of this clone operation, i.e. the bundle file that is\n> pointed at by \"remote.$remote_name.url\", cannot be that promisor.\n\nExtending the above a bit, one important way a bundle is used is as\na medium for sneaker-net. Instead of making a full clone over the\nnetwork, if you can create a bundle that records all objects and all\nrefs out of the source repository and then unbundle it in a\ndifferent place to create a repository, you can tweak the resulting\nrepository by either adding a separete remote or changing the\nremote.origin.url so that your subsequent fetch goes over the\nnetwork to the repository you took the initial bundle from.\n\nThe \"tweak the resulting repository\" part however MUST be done\nmanually with the current system. If we can optionally record the\npublically reachable URL of the source repository when we create a\nbundle file, and \"git clone\" on the receiving side can read the URL\nout of the bundle and act on it (e.g., show it to the user and offer\nto record it as remote.origin.url in the resulting repository---I do\nnot think it is wise to do this silently without letting the user\nknow from security's point of view), then the use of bundle files as\na medium for sneaker-netting will become even easier.\n\nAnd once that is done, perhaps allowing a filtered bundle to act as\na sneaker-net medium to simulate an initial filtered clone would\nmake sense. The promisor as well as the origin will be the network\nreachable URL and subsequent fetches (both deliberate ones via \"git\nfetch\" as well as lazy on-demand ones that backfills missing objects\nvia the \"promisor\" access) would become possible.\n\nBut without such a change to the bundle file format, allowing\n\"clone\" to finish and pretend the resulting repository is usable is\nsomewhat irresponsible to the users. The on-demand lazy fetch would\nfail after this code cloned from such a filtered bundle, no?\n"},{"id":"486794","messageId":"0d1f662d-fcdf-457c-8a7d-35e5e8d31dcd@gmail.com","threadId":"60742","inReplyTo":"CAFX5hXTCPt-rDrWZ-RN8S84o_FooY3Ck2H1kMYdHQGzuetPBSw@mail.gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-15T10:18:59Z","receivedAt":"2024-01-15T10:19:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Nikolay\n\nOn 14/01/2024 19:39, Nikolay Edigaryev wrote:\n> Hello Phillip,\n> \n>> As I understand it if you're cloning from a bundle file then then there\n>> is no remote so how can we set a remote-specific config?\n> \n> There is a remote, for more details see df61c88979 (clone: also\n> configure url for bare clones, 2010-03-29), which has the following\n> code:\n> \n> strbuf_addf(&key, \"remote.%s.url\", remote_name);\n> git_config_set(key.buf, repo);\n> strbuf_reset(&key);\n> \n> You can verify this by creating a bundle on Git 2.43.0 with \"git create\n> bundle bundle.bundle --all\" and then cloning it with \"git clone\n> --bare /path/to/bundle.bundle\", in my case the following repo-wide\n> configuration file was created:\n> \n> [core]\n> repositoryformatversion = 0\n> filemode = true\n> bare = true\n> ignorecase = true\n> precomposeunicode = true\n> [remote \"origin\"]\n> url = /Users/edi/src/cirrus-cli/cli.bundle\n\nOh, thanks for clarifying that I didn't realize we set \"origin\" to point \nto the bundle. That means this patch creates a promisor remote config \npointing to a bundle that does not contain the missing objects. As Junio \nsaid that doesn't make much sense to me as the point of the promisor \nconfig is to allow git to lazily fetch the missing objects.\n\nBest Wishes\n\nPhillip\n\n>> I'm surprised that the proposed change does not require the user to pass\n>> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n>> filter on the command line was compatible with the filter used to create\n>> the bundle. Allowing \"git clone\" to create a partial clone without the\n>> user asking for it by passing the \"--filter\" option feels like is going\n>> to end up confusing users.\n> \n> Note that currently, when you clone a normal non-filtered bundle with a\n> '--filter' argument specified, no filtering will take place and no error\n> will be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\n> will be set in the repo config, but no .promisor file will be created.\n> This is even more confusing IMO, but that's how it currently on\n> Git 2.43.0.\n> \n> You have a good point, but I feel like completely preventing cloning of\n> filtered bundles and requiring a '--filter' argument is very taxing. If\n> you've already specified a '--filter' when creating a bundle (and thus\n> your intent to use partially cloned data), why do it multiple times?\n> \n> What I propose as an alternative here is to act based on the user's\n> intent when cloning:\n> \n> * when the user specifies no '--filter' argument, do nothing special,\n> allow cloning both types of bundles: normal and filtered (with the\n> logic from this patch)\n> \n> * when the user does specify a '--filter' argument, either:\n> * throw an error explaining that filtering of filtered bundles is not\n> supported\n> * or compare the user's filter specification and the one that is\n> in the bundle and only throw an error if they mismatch\n> \n> Let me know what you think about this (and perhaps you have a more\n> concrete example in mind where this will have negative consequences)\n> and I'll be happy to do a next iteration.\n> \n> \n> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood wrote:\n>>\n>> Hi Nikolay\n>>\n>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:\n>>> From: Nikolay Edigaryev \n>>>\n>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n>>> incredibly useful ability to create filtered bundles, which advances\n>>> the partial clone/promisor support in Git and allows for archiving\n>>> large repositories to object storages like S3 in bundles that are:\n>>>\n>>> * easy to manage\n>>> * bundle is just a single file, it's easier to guarantee atomic\n>>> replacements in object storages like S3 and they are faster to\n>>> fetch than a bare repository since there's only a single GET\n>>> request involved\n>>> * incredibly tiny\n>>> * no indexes (which may be more than 10 MB for some repositories)\n>>> and other fluff, compared to cloning a bare repository\n>>> * bundle can be filtered to only contain the tips of refs neccessary\n>>> for e.g. code-analysis purposes\n>>>\n>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n>>> note that this behavior is not desired, and it the long-term this\n>>> should be possible.\n>>>\n>>> The commit above states that it's not possible to have this at the\n>>> moment due to lack of remote and a repository-global config that\n>>> specifies an object filter, yet it's unclear why a remote-specific\n>>> config can't be used instead, which is what this change does.\n>>\n>> As I understand it if you're cloning from a bundle file then then there\n>> is no remote so how can we set a remote-specific config?\n>>\n>> I'm surprised that the proposed change does not require the user to pass\n>> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n>> filter on the command line was compatible with the filter used to create\n>> the bundle. Allowing \"git clone\" to create a partial clone without the\n>> user asking for it by passing the \"--filter\" option feels like is going\n>> to end up confusing users.\n>>\n>>> +test_expect_success 'cloning from filtered bundle works' '\n>>> + git bundle create partial.bdl --all --filter=blob:none &&\n>>> + git clone --bare partial.bdl partial 2>err\n>>\n>> The redirection hides any error message which will make debugging test\n>> failures harder. It would be nice to see this test check any config set\n>> when cloning and that git commands can run successfully in the repository.\n>>\n>> Best Wishes\n>>\n>> Phillip\n"},{"id":"486795","messageId":"56522074-47b4-44d7-91da-8cf1149e85c7@gmail.com","threadId":"60742","inReplyTo":"CAFX5hXQ291gR4831dtRTdvtffWefCNCFp13ADvOkU7s3SVPczQ@mail.gmail.com","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-15T10:35:49Z","receivedAt":"2024-01-15T10:35:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Nikolay\n\nOn 14/01/2024 21:26, Nikolay Edigaryev wrote:\n>> Note that currently, when you clone a normal non-filtered bundle with a\n>> '--filter' argument specified, no filtering will take place and no error\n>> will be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\n>> will be set in the repo config, but no .promisor file will be created.\n>> This is even more confusing IMO, but that's how it currently on\n>> Git 2.43.0.\n> \n> I was wrong about this one: the .promisor pack file actually gets created.\n> \n> And the filtering is not being done because the \"uploadpack.allowFilter\"\n> global option is not enabled by default.\n\nThat makes sense - if you try to make a partial clone from a remote that \ndoes not support object filtering we fallback to a full clone and print \nthe warning you mention below.\n\n> Unfortunately the only way to figure this out is to run Git with\n> GIT_TRACE=2, which shows:\n\nThat seems strange, you should see the warning without having to set \nGIT_TRACE. I've certainly seen the warning in the past when trying to \ncreate a partial clone from a remote did not support them without me \nsetting any special environment variables.\n\nBest Wishes\n\nPhillip\n\n> warning: filtering not recognized by server, ignoring\n> \n> So please feel free to skip this part from the consideration.\n> \n> \n> On Sun, Jan 14, 2024 at 11:39 PM Nikolay Edigaryev wrote:\n>>\n>> Hello Phillip,\n>>\n>>> As I understand it if you're cloning from a bundle file then then there\n>>> is no remote so how can we set a remote-specific config?\n>>\n>> There is a remote, for more details see df61c88979 (clone: also\n>> configure url for bare clones, 2010-03-29), which has the following\n>> code:\n>>\n>> strbuf_addf(&key, \"remote.%s.url\", remote_name);\n>> git_config_set(key.buf, repo);\n>> strbuf_reset(&key);\n>>\n>> You can verify this by creating a bundle on Git 2.43.0 with \"git create\n>> bundle bundle.bundle --all\" and then cloning it with \"git clone\n>> --bare /path/to/bundle.bundle\", in my case the following repo-wide\n>> configuration file was created:\n>>\n>> [core]\n>> repositoryformatversion = 0\n>> filemode = true\n>> bare = true\n>> ignorecase = true\n>> precomposeunicode = true\n>> [remote \"origin\"]\n>> url = /Users/edi/src/cirrus-cli/cli.bundle\n>>\n>>> I'm surprised that the proposed change does not require the user to pass\n>>> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n>>> filter on the command line was compatible with the filter used to create\n>>> the bundle. Allowing \"git clone\" to create a partial clone without the\n>>> user asking for it by passing the \"--filter\" option feels like is going\n>>> to end up confusing users.\n>>\n>> Note that currently, when you clone a normal non-filtered bundle with a\n>> '--filter' argument specified, no filtering will take place and no error\n>> will be thrown. \"promisor = true\" and \"partialclonefilter = ...\" options\n>> will be set in the repo config, but no .promisor file will be created.\n>> This is even more confusing IMO, but that's how it currently on\n>> Git 2.43.0.\n>>\n>> You have a good point, but I feel like completely preventing cloning of\n>> filtered bundles and requiring a '--filter' argument is very taxing. If\n>> you've already specified a '--filter' when creating a bundle (and thus\n>> your intent to use partially cloned data), why do it multiple times?\n>>\n>> What I propose as an alternative here is to act based on the user's\n>> intent when cloning:\n>>\n>> * when the user specifies no '--filter' argument, do nothing special,\n>> allow cloning both types of bundles: normal and filtered (with the\n>> logic from this patch)\n>>\n>> * when the user does specify a '--filter' argument, either:\n>> * throw an error explaining that filtering of filtered bundles is not\n>> supported\n>> * or compare the user's filter specification and the one that is\n>> in the bundle and only throw an error if they mismatch\n>>\n>> Let me know what you think about this (and perhaps you have a more\n>> concrete example in mind where this will have negative consequences)\n>> and I'll be happy to do a next iteration.\n>>\n>>\n>> On Sun, Jan 14, 2024 at 10:00 PM Phillip Wood wrote:\n>>>\n>>> Hi Nikolay\n>>>\n>>> On 14/01/2024 11:16, Nikolay Edigaryev via GitGitGadget wrote:\n>>>> From: Nikolay Edigaryev \n>>>>\n>>>> f18b512bbb (bundle: create filtered bundles, 2022-03-09) introduced an\n>>>> incredibly useful ability to create filtered bundles, which advances\n>>>> the partial clone/promisor support in Git and allows for archiving\n>>>> large repositories to object storages like S3 in bundles that are:\n>>>>\n>>>> * easy to manage\n>>>> * bundle is just a single file, it's easier to guarantee atomic\n>>>> replacements in object storages like S3 and they are faster to\n>>>> fetch than a bare repository since there's only a single GET\n>>>> request involved\n>>>> * incredibly tiny\n>>>> * no indexes (which may be more than 10 MB for some repositories)\n>>>> and other fluff, compared to cloning a bare repository\n>>>> * bundle can be filtered to only contain the tips of refs neccessary\n>>>> for e.g. code-analysis purposes\n>>>>\n>>>> However, in 86fdd94d72 (clone: fail gracefully when cloning filtered\n>>>> bundle, 2022-03-09) the cloning of such bundles was disabled, with a\n>>>> note that this behavior is not desired, and it the long-term this\n>>>> should be possible.\n>>>>\n>>>> The commit above states that it's not possible to have this at the\n>>>> moment due to lack of remote and a repository-global config that\n>>>> specifies an object filter, yet it's unclear why a remote-specific\n>>>> config can't be used instead, which is what this change does.\n>>>\n>>> As I understand it if you're cloning from a bundle file then then there\n>>> is no remote so how can we set a remote-specific config?\n>>>\n>>> I'm surprised that the proposed change does not require the user to pass\n>>> \"--filter\" to \"git clone\" as I expected that we'd want to check that the\n>>> filter on the command line was compatible with the filter used to create\n>>> the bundle. Allowing \"git clone\" to create a partial clone without the\n>>> user asking for it by passing the \"--filter\" option feels like is going\n>>> to end up confusing users.\n>>>\n>>>> +test_expect_success 'cloning from filtered bundle works' '\n>>>> + git bundle create partial.bdl --all --filter=blob:none &&\n>>>> + git clone --bare partial.bdl partial 2>err\n>>>\n>>> The redirection hides any error message which will make debugging test\n>>> failures harder. It would be nice to see this test check any config set\n>>> when cloning and that git commands can run successfully in the repository.\n>>>\n>>> Best Wishes\n>>>\n>>> Phillip\n"},{"id":"486863","messageId":"CAFX5hXR1S_pLr+HTyR5ucazniO+4qZC_DyivDELitwS-Emx4GQ@mail.gmail.com","threadId":"60742","inReplyTo":"xmqq8r4r5ovf.fsf@gitster.g","subject":"Re: [PATCH] clone: support cloning of filtered bundles","fromName":"Nikolay Edigaryev","fromEmail":"edigaryev@gmail.com","sentAt":"2024-01-16T20:06:39Z","receivedAt":"2024-01-16T20:06:52Z","isPatch":true,"sender":{"key":"edigaryev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/85709?v=4"},"body":"Hello Junio and Phillip,\n\nThanks a lot for the explanations of how this is supposed to work. It\nseems that to make this work properly, we'd need to:\n\n(1) add an argument (or an option) to 'git bundle create', so that\n the user will be able to explicitly request the inclusion of a\n desired remote's URL\n\nWithout such mechanism in place data leak is possible, e.g. remote with\ncredentials hardcoded in it.\n\n(2) extend the 'gitformat-bundle' to include 'url'\n\nHowever, a remote can have multiple URLs and other remote-specific\noptions might be necessary to properly work with it.\n\n(3) add an argument (or an option) to 'git clone', so that the user\n will be able to explicitly request the write of the URL contained\n in the bundle to the repository's config\n\nOtherwise, it's insecure, e.g. someone might craft a bundle with a URL\nthat collects data from the user.\n\nI don't want waste anyone's time on this anymore because I've toyed with\n'git bundle' a bit more and realized that what I'm trying to accomplish\ncan be done the other way:\n\n1. git init\n\n2. git bundle unbundle |