{"thread":{"id":"55046","subject":"[PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","startedAt":"2021-01-25T23:11:47Z","lastAt":"2021-02-02T20:24:45Z","messageCount":24,"participants":["Jacob Vosmaer","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason","Eric Sunshine"],"isPatch":true,"patchVersion":3,"patchTotal":1},"messages":[{"id":"415245","messageId":"20210125230952.15468-2-jacob@gitlab.com","threadId":"55046","inReplyTo":"20210125230952.15468-1-jacob@gitlab.com","subject":"[PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-25T23:09:52Z","receivedAt":"2021-01-25T23:11:47Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"This fixes a bug that occurs when you combine partial clone and\nuploadpack.packobjectshook. You can reproduce it as follows:\n\ngit clone -u 'git -c uploadpack.allowfilter '\\\n'-c uploadpack.packobjectshook=env '\\\n'upload-pack' --filter=blob:none --no-local \\\nsrc.git dst.git\n\nBe careful with the line endings because this has a long quoted string\nas the -u argument.\n\nThe error I get when I run this is:\n\nCloning into '/tmp/broken'...\nremote: fatal: invalid filter-spec ''blob:none''\nerror: git upload-pack: git-pack-objects died with error.\nfatal: git upload-pack: aborting due to possible repository corruption on the remote side.\nremote: aborting due to possible repository corruption on the remote side.\nfatal: early EOF\nfatal: index-pack failed\n\nThe problem is an unnecessary and harmful layer of quoting. I tried\ndigging through the history of this function and I think this quoting\nwas there from the start. My best guess is that it stems from a\nmisunderstanding what use_shell=1 means. The code seems to assume it\nmeans \"arguments get joined into one big string, then fed to /bin/sh\".\nBut that is not what it means: use_shell=1 means that the first\nargument in the arguments array may be a shell script and if so should\nbe passed to /bin/sh. All other arguments are passed as normal\narguments.\n\nThe solution is simple: never quote the filter spec.\n\nThis commit removes the conditional quoting and adds a test for\npartial clone in t5544.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n t/t5544-pack-objects-hook.sh | 9 +++++++++\n upload-pack.c                | 9 +--------\n 2 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex 4357af1525..f5ba663d64 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '\n \ttest_path_is_missing .git/hook.stdout\n '\n \n+test_expect_success 'hook works with partial clone' '\n+\tclear_hook_results &&\n+\ttest_config_global uploadpack.packObjectsHook ./hook &&\n+\ttest_config_global uploadpack.allowFilter true &&\n+\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n+\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n+\tgrep \"^?\" objects\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..eae1fdbc55 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n-\t\tif (pack_objects.use_shell) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tsq_quote_buf(&buf, spec);\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n-\t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n-\t\t}\n+\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n \t}\n \tif (uri_protocols) {\n \t\tfor (i = 0; i < uri_protocols->nr; i++)\n-- \n2.30.0\n\n"},{"id":"415246","messageId":"20210125230952.15468-1-jacob@gitlab.com","threadId":"55046","inReplyTo":"YA81LEon1RPzT0T9@coredump.intra.peff.net","subject":"[PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-25T23:09:51Z","receivedAt":"2021-01-25T23:13:03Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Now with sign-off. Thanks for the reviews and comments!\n\nJacob\n\nJacob Vosmaer (1):\n  upload-pack.c: fix filter spec quoting bug\n\n t/t5544-pack-objects-hook.sh | 9 +++++++++\n upload-pack.c                | 9 +--------\n 2 files changed, 10 insertions(+), 8 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"415259","messageId":"YA81LEon1RPzT0T9@coredump.intra.peff.net","threadId":"55046","inReplyTo":"xmqqlfcgyf0o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-25T21:16:28Z","receivedAt":"2021-01-25T23:47:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 25, 2021 at 11:48:07AM -0800, Junio C Hamano wrote:\n\n> I suspect that 0b6069fe (fetch-pack: test support excluding large\n> blobs, 2017-12-08) sort-of fixed half of the problem (i.e. the half\n> when there is no hook used) while leaving the other half still\n> broken as before.\n> \n> But because .use_shell does not affect if we should or should not\n> quote, we can unconditionally drop the use of sq_quote_buf().\n\nYep. That was the same conclusion I came to in my earlier reply:\n\n  https://lore.kernel.org/git/YAs2RMT1rEH%2F2LSp@coredump.intra.peff.net/\n\n> > This commit removes the conditional quoting and adds a test for\n> > partial clone in t5544.\n> > ---\n> \n> Thanks.  Missing sign-off.\n\nAside from the sign-off issue, this version looks good to me.\n\n-Peff\n"},{"id":"415261","messageId":"xmqqy2ggwoqm.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"20210125230952.15468-1-jacob@gitlab.com","subject":"Re: [PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T00:01:05Z","receivedAt":"2021-01-26T00:08:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> Now with sign-off. Thanks for the reviews and comments!\n>\n> Jacob\n>\n> Jacob Vosmaer (1):\n>   upload-pack.c: fix filter spec quoting bug\n>\n>  t/t5544-pack-objects-hook.sh | 9 +++++++++\n>  upload-pack.c                | 9 +--------\n>  2 files changed, 10 insertions(+), 8 deletions(-)\n\nThanks, will queue.\n"},{"id":"415281","messageId":"CADMWQoP+hgPiSHJ5D-3G0+-bZOYm1hr=ojBeSmUJhiS7bp0zPg@mail.gmail.com","threadId":"55046","inReplyTo":"874kj46mwf.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-26T10:29:55Z","receivedAt":"2021-01-26T12:35:33Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Thanks for the feedback Ævar. I am not sure if I am still supposed to\nmake changes to the patch now that it is in \"seen\" as\n7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I?\n\nBest regards,\n\nJacob Vosmaer\nGitLab, Inc.\n\nOn Tue, Jan 26, 2021 at 10:57 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Tue, Jan 26 2021, Jacob Vosmaer wrote:\n>\n> > This fixes a bug that occurs when you combine partial clone and\n> > uploadpack.packobjectshook. You can reproduce it as follows:\n>\n> Let's:\n>\n>  * Refer to the commit we're fixing a bug in, i.e. Junio's mention of\n>    10ac85c7 (upload-pack: add object filtering for partial clone,\n>    2017-12-08) upthread.\n>\n>  * See also \"imperative-mood\" in SubmittingPatches. I.e. say \"Fix a bug\n>    in ...\" not \"This fixes ... can be reproduced as\"\n>\n>  * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C\n>    code.\n>\n> > git clone -u 'git -c uploadpack.allowfilter '\\\n> > '-c uploadpack.packobjectshook=env '\\\n> > 'upload-pack' --filter=blob:none --no-local \\\n> > src.git dst.git\n>\n> This and the output below would be more readable indented.\n>\n> > Be careful with the line endings because this has a long quoted string\n> > as the -u argument.\n> >\n> > The error I get when I run this is:\n> >\n> > Cloning into '/tmp/broken'...\n> > remote: fatal: invalid filter-spec ''blob:none''\n> > error: git upload-pack: git-pack-objects died with error.\n> > fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n> > remote: aborting due to possible repository corruption on the remote side.\n> > fatal: early EOF\n> > fatal: index-pack failed\n>\n> [...]\n>\n> > The problem is an unnecessary and harmful layer of quoting. I tried\n> > digging through the history of this function and I think this quoting\n> > was there from the start.\n>\n>\n> ...So looked at \"git log\" but didn't try to check out 10ac85c7 and see\n> if it had the same issue? If we're going to leave a note about this at\n> all probably better to help future source spelunkers by being able to\n> say the issue was there from the start.\n>\n>\n> > My best guess is that it stems from a\n> > misunderstanding what use_shell=1 means. The code seems to assume it\n> > means \"arguments get joined into one big string, then fed to /bin/sh\".\n> > But that is not what it means: use_shell=1 means that the first\n> > argument in the arguments array may be a shell script and if so should\n> > be passed to /bin/sh. All other arguments are passed as normal\n> > arguments.\n> >\n> > The solution is simple: never quote the filter spec.\n> >\n> > This commit removes the conditional quoting and adds a test for\n> > partial clone in t5544.\n> >\n>\n> Thanks for hacking this up! Hopefully the above is helpful and not too\n> nitpicky. Mainly wanted to help you get future patches through more\n> easily...\n>\n> > Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> > ---\n> >  t/t5544-pack-objects-hook.sh | 9 +++++++++\n> >  upload-pack.c                | 9 +--------\n> >  2 files changed, 10 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\n> > index 4357af1525..f5ba663d64 100755\n> > --- a/t/t5544-pack-objects-hook.sh\n> > +++ b/t/t5544-pack-objects-hook.sh\n> > @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '\n> >       test_path_is_missing .git/hook.stdout\n> >  '\n> >\n> > +test_expect_success 'hook works with partial clone' '\n> > +     clear_hook_results &&\n> > +     test_config_global uploadpack.packObjectsHook ./hook &&\n> > +     test_config_global uploadpack.allowFilter true &&\n> > +     git clone --bare --no-local --filter=blob:none . dst.git &&\n> > +     git -C dst.git rev-list --objects --missing=print HEAD >objects &&\n> > +     grep \"^?\" objects\n> > +'\n> > +\n> >  test_done\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 3b66bf92ba..eae1fdbc55 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> >       if (pack_data->filter_options.choice) {\n> >               const char *spec =\n> >                       expand_list_objects_filter_spec(&pack_data->filter_options);\n> > -             if (pack_objects.use_shell) {\n> > -                     struct strbuf buf = STRBUF_INIT;\n> > -                     sq_quote_buf(&buf, spec);\n> > -                     strvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n> > -                     strbuf_release(&buf);\n> > -             } else {\n> > -                     strvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> > -             }\n> > +             strvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> >       }\n> >       if (uri_protocols) {\n> >               for (i = 0; i < uri_protocols->nr; i++)\n>\n"},{"id":"415286","messageId":"874kj46mwf.fsf@evledraar.gmail.com","threadId":"55046","inReplyTo":"20210125230952.15468-2-jacob@gitlab.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-26T09:57:36Z","receivedAt":"2021-01-26T14:36:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 26 2021, Jacob Vosmaer wrote:\n\n> This fixes a bug that occurs when you combine partial clone and\n> uploadpack.packobjectshook. You can reproduce it as follows:\n\nLet's: \n\n * Refer to the commit we're fixing a bug in, i.e. Junio's mention of\n   10ac85c7 (upload-pack: add object filtering for partial clone,\n   2017-12-08) upthread.\n\n * See also \"imperative-mood\" in SubmittingPatches. I.e. say \"Fix a bug\n   in ...\" not \"This fixes ... can be reproduced as\"\n\n * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C\n   code.\n\n> git clone -u 'git -c uploadpack.allowfilter '\\\n> '-c uploadpack.packobjectshook=env '\\\n> 'upload-pack' --filter=blob:none --no-local \\\n> src.git dst.git\n\nThis and the output below would be more readable indented.\n\n> Be careful with the line endings because this has a long quoted string\n> as the -u argument.\n>\n> The error I get when I run this is:\n>\n> Cloning into '/tmp/broken'...\n> remote: fatal: invalid filter-spec ''blob:none''\n> error: git upload-pack: git-pack-objects died with error.\n> fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n> remote: aborting due to possible repository corruption on the remote side.\n> fatal: early EOF\n> fatal: index-pack failed\n\n[...]\n\n> The problem is an unnecessary and harmful layer of quoting. I tried\n> digging through the history of this function and I think this quoting\n> was there from the start.\n\n\n...So looked at \"git log\" but didn't try to check out 10ac85c7 and see\nif it had the same issue? If we're going to leave a note about this at\nall probably better to help future source spelunkers by being able to\nsay the issue was there from the start.\n\n\n> My best guess is that it stems from a\n> misunderstanding what use_shell=1 means. The code seems to assume it\n> means \"arguments get joined into one big string, then fed to /bin/sh\".\n> But that is not what it means: use_shell=1 means that the first\n> argument in the arguments array may be a shell script and if so should\n> be passed to /bin/sh. All other arguments are passed as normal\n> arguments.\n>\n> The solution is simple: never quote the filter spec.\n>\n> This commit removes the conditional quoting and adds a test for\n> partial clone in t5544.\n>\n\nThanks for hacking this up! Hopefully the above is helpful and not too\nnitpicky. Mainly wanted to help you get future patches through more\neasily...\n\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  t/t5544-pack-objects-hook.sh | 9 +++++++++\n>  upload-pack.c                | 9 +--------\n>  2 files changed, 10 insertions(+), 8 deletions(-)\n>\n> diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\n> index 4357af1525..f5ba663d64 100755\n> --- a/t/t5544-pack-objects-hook.sh\n> +++ b/t/t5544-pack-objects-hook.sh\n> @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '\n>  \ttest_path_is_missing .git/hook.stdout\n>  '\n>  \n> +test_expect_success 'hook works with partial clone' '\n> +\tclear_hook_results &&\n> +\ttest_config_global uploadpack.packObjectsHook ./hook &&\n> +\ttest_config_global uploadpack.allowFilter true &&\n> +\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n> +\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n> +\tgrep \"^?\" objects\n> +'\n> +\n>  test_done\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 3b66bf92ba..eae1fdbc55 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \tif (pack_data->filter_options.choice) {\n>  \t\tconst char *spec =\n>  \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n> -\t\tif (pack_objects.use_shell) {\n> -\t\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\t\tsq_quote_buf(&buf, spec);\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n> -\t\t\tstrbuf_release(&buf);\n> -\t\t} else {\n> -\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n> -\t\t}\n> +\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n>  \t}\n>  \tif (uri_protocols) {\n>  \t\tfor (i = 0; i < uri_protocols->nr; i++)\n\n"},{"id":"415315","messageId":"xmqqlfcg4ep2.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"YA81LEon1RPzT0T9@coredump.intra.peff.net","subject":"Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T02:25:29Z","receivedAt":"2021-01-26T20:46:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jan 25, 2021 at 11:48:07AM -0800, Junio C Hamano wrote:\n>\n>> I suspect that 0b6069fe (fetch-pack: test support excluding large\n>> blobs, 2017-12-08) sort-of fixed half of the problem (i.e. the half\n>> when there is no hook used) while leaving the other half still\n>> broken as before.\n>> \n>> But because .use_shell does not affect if we should or should not\n>> quote, we can unconditionally drop the use of sq_quote_buf().\n>\n> Yep. That was the same conclusion I came to in my earlier reply:\n>\n>   https://lore.kernel.org/git/YAs2RMT1rEH%2F2LSp@coredump.intra.peff.net/\n\nThanks.  As I am behind, I needed to think it aloud to make sure I\nam on the same page as others ;-)  A sanity check like this is\ngreatly appreciated.\n"},{"id":"415329","messageId":"xmqqpn1r3827.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"CADMWQoP+hgPiSHJ5D-3G0+-bZOYm1hr=ojBeSmUJhiS7bp0zPg@mail.gmail.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T17:46:24Z","receivedAt":"2021-01-26T22:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> Thanks for the feedback Ævar. I am not sure if I am still supposed to\n> make changes to the patch now that it is in \"seen\" as\n> 7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I?\n\nBeing in 'seen' does not mean all that much.  You are free to\nimprove your patch by replacing until you and everybody else\nplease.\n\nIt is when it hits 'next' that replacement no longer becomes\nfeasible and it gets cast in stone and goes on track to be hopefully\npart of the next release.\n\nThanks.\n"},{"id":"415330","messageId":"xmqqlfcf37tk.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"874kj46mwf.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T17:51:35Z","receivedAt":"2021-01-26T22:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Tue, Jan 26 2021, Jacob Vosmaer wrote:\n>\n>> This fixes a bug that occurs when you combine partial clone and\n>> uploadpack.packobjectshook. You can reproduce it as follows:\n>\n> Let's: \n>\n>  * Refer to the commit we're fixing a bug in, i.e. Junio's mention of\n>    10ac85c7 (upload-pack: add object filtering for partial clone,\n>    2017-12-08) upthread.\n>\n>  * See also \"imperative-mood\" in SubmittingPatches. I.e. say \"Fix a bug\n>    in ...\" not \"This fixes ... can be reproduced as\"\n>\n>  * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C\n>    code.\n>\n> ... [jc: all the helpful hints snipped] ...\n>\n> Thanks for hacking this up! Hopefully the above is helpful and not too\n> nitpicky. Mainly wanted to help you get future patches through more\n> easily...\n\nYeah, thanks, both, for aiming higher ;-)  \n\nI have to admit that I did find the log message a bit lacking, and\nthat was why I had to dig bit to find out how the historical issue\nhappened myself in the first place, and I tend to agree that it\nfeels a bit of waste for that work to end up buried in the list\narchive without getting reflected in the proposed log message.\n\n"},{"id":"415352","messageId":"YBCEmZHapifbVqRy@coredump.intra.peff.net","threadId":"55046","inReplyTo":"874kj46mwf.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-26T21:07:37Z","receivedAt":"2021-01-26T22:19:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 26, 2021 at 10:57:36AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > This fixes a bug that occurs when you combine partial clone and\n> > uploadpack.packobjectshook. You can reproduce it as follows:\n> \n> Let's: \n> \n>  * Refer to the commit we're fixing a bug in, i.e. Junio's mention of\n>    10ac85c7 (upload-pack: add object filtering for partial clone,\n>    2017-12-08) upthread.\n> \n>  * See also \"imperative-mood\" in SubmittingPatches. I.e. say \"Fix a bug\n>    in ...\" not \"This fixes ... can be reproduced as\"\n> \n>  * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C\n>    code.\n\nGenerally good advice (the imperative stuff IMHO is less important\noutside of the subject line, but a reasonable default way of writing).\n\n> > The problem is an unnecessary and harmful layer of quoting. I tried\n> > digging through the history of this function and I think this quoting\n> > was there from the start.\n> \n> \n> ...So looked at \"git log\" but didn't try to check out 10ac85c7 and see\n> if it had the same issue? If we're going to leave a note about this at\n> all probably better to help future source spelunkers by being able to\n> say the issue was there from the start.\n\nI don't think it could be tested easily at that point. It implemented\nthe server side, but not the client side. And later when the client side\nwas added, the non-hook code path was fixed. (More discussion earlier in\nthe thread).\n\n-Peff\n"},{"id":"415365","messageId":"YBCFBivBLgqEAUr1@coredump.intra.peff.net","threadId":"55046","inReplyTo":"CADMWQoP+hgPiSHJ5D-3G0+-bZOYm1hr=ojBeSmUJhiS7bp0zPg@mail.gmail.com","subject":"Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-26T21:09:26Z","receivedAt":"2021-01-26T22:19:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 26, 2021 at 11:29:55AM +0100, Jacob Vosmaer wrote:\n\n> Thanks for the feedback Ævar. I am not sure if I am still supposed to\n> make changes to the patch now that it is in \"seen\" as\n> 7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I?\n\nIt's OK to send re-rolls of patches that are in \"seen\". That branch is\nrewound and rewritten regularly as part of Junio's integration cycles.\nOnce a commit is in \"next\", then it is generally considered set in\nstone, and fixes should come on top rather than as re-rolls.\n\n(There is one exception; \"next\" is rewound after a release, so that is\nan opportunity for topics that were so muddled in their earlier\nincarnation to get rewritten. That is pretty rare, though).\n\n-Peff\n"},{"id":"415507","messageId":"20210128160453.79169-1-jacob@gitlab.com","threadId":"55046","inReplyTo":"YBCFBivBLgqEAUr1@coredump.intra.peff.net","subject":"[PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-28T16:04:53Z","receivedAt":"2021-01-28T16:07:03Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Fix a bug in upload-pack.c that occurs when you combine partial clone and\nuploadpack.packObjectsHook. You can reproduce it as follows:\n\n\tgit clone -u 'git -c uploadpack.allowfilter '\\\n\t'-c uploadpack.packobjectshook=env '\\\n\t'upload-pack' --filter=blob:none --no-local \\\n\tsrc.git dst.git\n\nBe careful with the line endings because this has a long quoted string\nas the -u argument.\n\nThe error I get when I run this is:\n\n\tCloning into '/tmp/broken'...\n\tremote: fatal: invalid filter-spec ''blob:none''\n\terror: git upload-pack: git-pack-objects died with error.\n\tfatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n\tremote: aborting due to possible repository corruption on the remote side.\n\tfatal: early EOF\n\tfatal: index-pack failed\n\nThe problem is caused by unneeded quoting. This bug was already\npresent in 10ac85c785 (upload-pack: add object filtering for partial\nclone, 2017-12-08) when the server side filter support was introduced.\nIn fact, in 10ac85c785 this was broken regardless of\nuploadpack.packObjectsHook. Then in 0b6069fe0a (fetch-pack: test\nsupport excluding large blobs, 2017-12-08) the quoting was removed but\nonly behind a conditional that depends on whether\nuploadpack.packObjectsHook is set. Because uploadpack.packObjectsHook\nis apparently rarely used, nobody noticed the problematic quoting\ncould still happen.\n\nThis commit removes the conditional quoting and adds a test for\npartial clone in t5544-pack-objects-hook.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n t/t5544-pack-objects-hook.sh | 9 +++++++++\n upload-pack.c                | 9 +--------\n 2 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex 4357af1525..f5ba663d64 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' '\n \ttest_path_is_missing .git/hook.stdout\n '\n \n+test_expect_success 'hook works with partial clone' '\n+\tclear_hook_results &&\n+\ttest_config_global uploadpack.packObjectsHook ./hook &&\n+\ttest_config_global uploadpack.allowFilter true &&\n+\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n+\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n+\tgrep \"^?\" objects\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..eae1fdbc55 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n-\t\tif (pack_objects.use_shell) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tsq_quote_buf(&buf, spec);\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n-\t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n-\t\t}\n+\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n \t}\n \tif (uri_protocols) {\n \t\tfor (i = 0; i < uri_protocols->nr; i++)\n-- \n2.30.0\n\n"},{"id":"415526","messageId":"CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com","threadId":"55046","inReplyTo":"xmqqmtwsx4d9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-28T21:12:42Z","receivedAt":"2021-01-28T21:13:52Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Thu, Jan 28, 2021 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>  * As readers know \"clone\" would get not just the current branch,\n>    the way this rev-list traverses only from HEAD makes them wonder\n>    if you are trying to exclude refs other than the current branch\n>    for a reason.  Better write it as\n>\n>         git -C dst.git rev-list --objects --missing=print --all >objects &&\n\nGood point!\n\n>  * The above says that we are happy if we can clone without erroring\n>    out, as long as some objects are missing, but we could go a bit\n>    stronger than that: among the objects we have, none should be a\n>    blob object.  Is that something we can easily check?\n>\n>    Something along the lines of...\n>\n>         grep -v \"^?\" objects |\n>         git -C dst.git cat-file --batch-check=\"%(objecttype)\" >types &&\n>         sed -e '/^commit/d' -e '/^tag/d' -e '/^tree/d' types >actual &&\n>         test_must_be_empty actual\n>\n>    ... to ensure everything is either commit, tag or tree, perhaps?\n\nMakes sense, I started down that route at first but I bumped my head\nagainst needing --missing to prevent the lazy fetching, and stopped\nonce I had the question mark grep.\n\nNow that we're talking about the test, I was wondering about something else.\n\nIn my original patch, I purposely did not add a test. Why? Because\n--filter is just one option of several that upload-pack passes to\npack-objects (think of --shallow-file and --include-tag, for\ninstance). Why is --filter special? If the original quoting bug had\nnot happened, would we be testing various permutations of clone\noptions in combination with packObjectsHook?\n\nAs a reader looking at t5544, unless I know the backstory of the bug,\nI do not understand why --filter gets a test but those other things do\nnot.\n\nI am not saying this to push back on going back and improving the\ntest, I'm quite happy to. It's more that deleting the test may be the\nultimate improvement. Thanks in advance for humoring this contrarian\nsuggestion. :)\n\nCheers, Jacob\n"},{"id":"415527","messageId":"CADMWQoOVe1cfeO6ncL1_E8oHxp_ovK-RViT-DZLiCDhkWWkPZA@mail.gmail.com","threadId":"55046","inReplyTo":"CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com","subject":"Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-01-28T21:40:06Z","receivedAt":"2021-01-28T21:41:28Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Thu, Jan 28, 2021 at 10:12 PM Jacob Vosmaer <jacob@gitlab.com> wrote:\n> Thanks in advance for humoring this contrarian\n> suggestion. :)\n\nEnglish is not my first language. I should have said \"considering\",\nnot \"humoring\".\n"},{"id":"415528","messageId":"YBMx7yuBit8TecYp@coredump.intra.peff.net","threadId":"55046","inReplyTo":"CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com","subject":"Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T21:51:43Z","receivedAt":"2021-01-28T21:52:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2021 at 10:12:42PM +0100, Jacob Vosmaer wrote:\n\n> Now that we're talking about the test, I was wondering about something else.\n> \n> In my original patch, I purposely did not add a test. Why? Because\n> --filter is just one option of several that upload-pack passes to\n> pack-objects (think of --shallow-file and --include-tag, for\n> instance). Why is --filter special? If the original quoting bug had\n> not happened, would we be testing various permutations of clone\n> options in combination with packObjectsHook?\n> \n> As a reader looking at t5544, unless I know the backstory of the bug,\n> I do not understand why --filter gets a test but those other things do\n> not.\n\nYou're definitely not wrong that this is somewhat closing the barn door\nafter horse has left, and there may be other barns still to be fixed.\n\nBut usually we'll add a test that demonstrates the breakage, if only\nbecause the fact that something so mundane and easy to trigger _wasn't_\ncaught by the existing test is pretty bad. So we should at least make\nsure the combination is now covered, which your test does. And then over\ntime we build up a set of coverage.\n\nOf course it is sometimes nice to be exhaustive, too. The organically\nbuilt-up set of tests is going to have holes if it's not done\nsystematically. But Git also isn't a black box; we know that the\nimplementation of this option was weirdly different.\n\nI guess the argument you are making is that now that it's fixed, it\n_isn't_ different, and we're unlikely to reintroduce the bug. I can buy\nthat, but I think we just do the \"test the fixed bug\" thing on\nprinciple.\n\n-Peff\n"},{"id":"415529","messageId":"xmqqpn1ovi4j.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com","subject":"Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-28T21:58:20Z","receivedAt":"2021-01-28T21:59:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> As a reader looking at t5544, unless I know the backstory of the bug,\n> I do not understand why --filter gets a test but those other things do\n> not.\n\nGood point.  Perhaps a retitle of the test or a bit of comment would\nbenefit future readers.\n\n\n# git clone internally has to invoke upload-pack on the\n# other end with multiple arguments, and it used to quote\n# them incorrectly only when hooks are enabled.\ntest_expect_success 'hook works with partial clone' '\n\tclear_hook_results &&\n\ttest_config_global uploadpack.packObjectsHook ./hook &&\n\ttest_config_global uploadpack.allowFilter true &&\n\n\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n\tgrep \"^?\" objects\n'\n\nBut that may be overkill.  Those curious can run \"git blame\" to go\nback to what you wrote in the log message, and that should be clear\nenough for them why we care about this case.\n\nAnd from that point of view, it may be sufficient that the resulting\nrepository lacks \"some\" objects and not necessarily check what are\nmissing.\n\nThanks.\n"},{"id":"415810","messageId":"20210201202938.39576-1-jacob@gitlab.com","threadId":"55046","inReplyTo":"xmqqpn1ovi4j.fsf@gitster.c.googlers.com","subject":"[PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-02-01T20:29:37Z","receivedAt":"2021-02-01T20:30:42Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"I updated the test based on the latest round of feedback.\n\nJacob Vosmaer (1):\n  upload-pack.c: fix filter spec quoting bug\n\n t/t5544-pack-objects-hook.sh | 10 ++++++++++\n upload-pack.c                |  9 +--------\n 2 files changed, 11 insertions(+), 8 deletions(-)\n\n-- \n2.30.0\n\n"},{"id":"415811","messageId":"20210201202938.39576-2-jacob@gitlab.com","threadId":"55046","inReplyTo":"20210201202938.39576-1-jacob@gitlab.com","subject":"[PATCH v5 1/1] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-02-01T20:29:38Z","receivedAt":"2021-02-01T20:30:46Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Fix a bug in upload-pack.c that occurs when you combine partial clone and\nuploadpack.packObjectsHook. You can reproduce it as follows:\n\n\tgit clone -u 'git -c uploadpack.allowfilter '\\\n\t'-c uploadpack.packobjectshook=env '\\\n\t'upload-pack' --filter=blob:none --no-local \\\n\tsrc.git dst.git\n\nBe careful with the line endings because this has a long quoted string\nas the -u argument.\n\nThe error I get when I run this is:\n\n\tCloning into '/tmp/broken'...\n\tremote: fatal: invalid filter-spec ''blob:none''\n\terror: git upload-pack: git-pack-objects died with error.\n\tfatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n\tremote: aborting due to possible repository corruption on the remote side.\n\tfatal: early EOF\n\tfatal: index-pack failed\n\nThe problem is caused by unneeded quoting. This bug was already\npresent in 10ac85c785 (upload-pack: add object filtering for partial\nclone, 2017-12-08) when the server side filter support was introduced.\nIn fact, in 10ac85c785 this was broken regardless of\nuploadpack.packObjectsHook. Then in 0b6069fe0a (fetch-pack: test\nsupport excluding large blobs, 2017-12-08) the quoting was removed but\nonly behind a conditional that depends on whether\nuploadpack.packObjectsHook is set. Because uploadpack.packObjectsHook\nis apparently rarely used, nobody noticed the problematic quoting\ncould still happen.\n\nThis commit removes the conditional quoting and adds a test for\npartial clone in t5544-pack-objects-hook.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n t/t5544-pack-objects-hook.sh | 10 ++++++++++\n upload-pack.c                |  9 +--------\n 2 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex 4357af1525..dd5f44d986 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -59,4 +59,14 @@ test_expect_success 'hook does not run from repo config' '\n \ttest_path_is_missing .git/hook.stdout\n '\n \n+test_expect_success 'hook works with partial clone' '\n+\tclear_hook_results &&\n+\ttest_config_global uploadpack.packObjectsHook ./hook &&\n+\ttest_config_global uploadpack.allowFilter true &&\n+\tgit clone --bare --no-local --filter=blob:none . dst.git &&\n+\tgit -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects &&\n+\tgit -C dst.git cat-file --batch-check=\"%(objecttype)\" <objects >types &&\n+\t! grep blob types\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..eae1fdbc55 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n-\t\tif (pack_objects.use_shell) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tsq_quote_buf(&buf, spec);\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", buf.buf);\n-\t\t\tstrbuf_release(&buf);\n-\t\t} else {\n-\t\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n-\t\t}\n+\t\tstrvec_pushf(&pack_objects.args, \"--filter=%s\", spec);\n \t}\n \tif (uri_protocols) {\n \t\tfor (i = 0; i < uri_protocols->nr; i++)\n-- \n2.30.0\n\n"},{"id":"415812","messageId":"CADMWQoMap_wRTtcTbR8NeDWR6pKq876FEy3WdpFQwEkOR6E+mg@mail.gmail.com","threadId":"55046","inReplyTo":"YBMx7yuBit8TecYp@coredump.intra.peff.net","subject":"Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-02-01T20:31:06Z","receivedAt":"2021-02-01T20:32:06Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"On Thu, Jan 28, 2021 at 10:51 PM Jeff King <peff@peff.net> wrote:\n> I guess the argument you are making is that now that it's fixed, it\n> _isn't_ different, and we're unlikely to reintroduce the bug. I can buy\n> that, but I think we just do the \"test the fixed bug\" thing on\n> principle.\n\nMakes sense, thanks for explaining!\n"},{"id":"415885","messageId":"xmqq8s87hvd7.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"20210201202938.39576-1-jacob@gitlab.com","subject":"Re: [PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T05:49:40Z","receivedAt":"2021-02-02T05:50:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> I updated the test based on the latest round of feedback.\n\nThanks, but this came a bit too late to wholesale replace the patch;\nthe previous round was good enough and is in 'next' already.\n\nLooking at the change between two versions, I guess that the change\nin tests may be worth salvaging, so perhaps you can make it into an\nincremental \"the test we added earlier was good, but let's make it\neven better by doing X and Y\" patch to be applied on top?\n\nThanks.\n\n\n\n\n> Jacob Vosmaer (1):\n>   upload-pack.c: fix filter spec quoting bug\n>\n>  t/t5544-pack-objects-hook.sh | 10 ++++++++++\n>  upload-pack.c                |  9 +--------\n>  2 files changed, 11 insertions(+), 8 deletions(-)\n"},{"id":"415915","messageId":"20210202103733.42513-1-jacob@gitlab.com","threadId":"55046","inReplyTo":"xmqq8s87hvd7.fsf@gitster.c.googlers.com","subject":"[PATCH 1/1] t5544: clarify 'hook works with partial clone' test","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-02-02T10:37:33Z","receivedAt":"2021-02-02T10:38:55Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Apply a few leftover improvements from the review of ad5df6b782\n(upload-pack.c: fix filter spec quoting bug).\n\n1. Instead of enumerating objects reachable from HEAD, enumerate all\nreachable objects, because HEAD has not special significance in this\ntest.\n\n2. Instead of relying on the knowledge that \"? in rev-list output\nmeans partial clone\", explicitly verify that there are no blobs with\ncat-file.\n---\n t/t5544-pack-objects-hook.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex f5ba663d64..dd5f44d986 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' '\n \ttest_config_global uploadpack.packObjectsHook ./hook &&\n \ttest_config_global uploadpack.allowFilter true &&\n \tgit clone --bare --no-local --filter=blob:none . dst.git &&\n-\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n-\tgrep \"^?\" objects\n+\tgit -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects &&\n+\tgit -C dst.git cat-file --batch-check=\"%(objecttype)\" <objects >types &&\n+\t! grep blob types\n '\n \n test_done\n-- \n2.30.0\n\n"},{"id":"415940","messageId":"CAPig+cQoTpsYS+2pccHmzx=Xoc34u166cYoC=GAcEndofN4gcA@mail.gmail.com","threadId":"55046","inReplyTo":"20210202103733.42513-1-jacob@gitlab.com","subject":"Re: [PATCH 1/1] t5544: clarify 'hook works with partial clone' test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-02T17:22:59Z","receivedAt":"2021-02-02T17:26:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 2, 2021 at 5:40 AM Jacob Vosmaer <jacob@gitlab.com> wrote:\n> Apply a few leftover improvements from the review of ad5df6b782\n> (upload-pack.c: fix filter spec quoting bug).\n>\n> 1. Instead of enumerating objects reachable from HEAD, enumerate all\n> reachable objects, because HEAD has not special significance in this\n> test.\n>\n> 2. Instead of relying on the knowledge that \"? in rev-list output\n> means partial clone\", explicitly verify that there are no blobs with\n> cat-file.\n> ---\n\nMissing Signed-off-by:.\n"},{"id":"415948","messageId":"20210202192417.68663-1-jacob@gitlab.com","threadId":"55046","inReplyTo":"CAPig+cQoTpsYS+2pccHmzx=Xoc34u166cYoC=GAcEndofN4gcA@mail.gmail.com","subject":"[PATCH v2] t5544: clarify 'hook works with partial clone' test","fromName":"Jacob Vosmaer","fromEmail":"jacob@gitlab.com","sentAt":"2021-02-02T19:24:17Z","receivedAt":"2021-02-02T19:27:58Z","isPatch":true,"sender":{"key":"jacob@gitlab.com","avatar":"https://avatars.githubusercontent.com/u/1336311?v=4"},"body":"Apply a few leftover improvements from the review of ad5df6b782\n(upload-pack.c: fix filter spec quoting bug).\n\n1. Instead of enumerating objects reachable from HEAD, enumerate all\nreachable objects, because HEAD has not special significance in this\ntest.\n\n2. Instead of relying on the knowledge that \"? in rev-list output\nmeans partial clone\", explicitly verify that there are no blobs with\ncat-file.\n\nSigned-off-by: Jacob Vosmaer <jacob@gitlab.com>\n---\n t/t5544-pack-objects-hook.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nindex f5ba663d64..dd5f44d986 100755\n--- a/t/t5544-pack-objects-hook.sh\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' '\n \ttest_config_global uploadpack.packObjectsHook ./hook &&\n \ttest_config_global uploadpack.allowFilter true &&\n \tgit clone --bare --no-local --filter=blob:none . dst.git &&\n-\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n-\tgrep \"^?\" objects\n+\tgit -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects &&\n+\tgit -C dst.git cat-file --batch-check=\"%(objecttype)\" <objects >types &&\n+\t! grep blob types\n '\n \n test_done\n-- \n2.30.0\n\n"},{"id":"415956","messageId":"xmqqtuqufcfa.fsf@gitster.c.googlers.com","threadId":"55046","inReplyTo":"20210202192417.68663-1-jacob@gitlab.com","subject":"Re: [PATCH v2] t5544: clarify 'hook works with partial clone' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-02T20:21:45Z","receivedAt":"2021-02-02T20:24:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Vosmaer <jacob@gitlab.com> writes:\n\n> Apply a few leftover improvements from the review of ad5df6b782\n> (upload-pack.c: fix filter spec quoting bug).\n>\n> 1. Instead of enumerating objects reachable from HEAD, enumerate all\n> reachable objects, because HEAD has not special significance in this\n> test.\n>\n> 2. Instead of relying on the knowledge that \"? in rev-list output\n> means partial clone\", explicitly verify that there are no blobs with\n> cat-file.\n>\n> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>\n> ---\n>  t/t5544-pack-objects-hook.sh | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n\nThanks.\n\n>\n> diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\n> index f5ba663d64..dd5f44d986 100755\n> --- a/t/t5544-pack-objects-hook.sh\n> +++ b/t/t5544-pack-objects-hook.sh\n> @@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' '\n>  \ttest_config_global uploadpack.packObjectsHook ./hook &&\n>  \ttest_config_global uploadpack.allowFilter true &&\n>  \tgit clone --bare --no-local --filter=blob:none . dst.git &&\n> -\tgit -C dst.git rev-list --objects --missing=print HEAD >objects &&\n> -\tgrep \"^?\" objects\n> +\tgit -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects &&\n> +\tgit -C dst.git cat-file --batch-check=\"%(objecttype)\" <objects >types &&\n> +\t! grep blob types\n>  '\n>  \n>  test_done\n"}]}