# [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug 24 messages from 2021-01-25 to 2021-02-02. Participants: Jacob Vosmaer, Jeff King, Junio C Hamano, Ævar Arnfjörð Bjarmason, Eric Sunshine. Thread: https://gitlist.dev/t/55046 ## Jacob Vosmaer, 2021-01-25 23:09 Subject: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: <20210125230952.15468-2-jacob@gitlab.com> URL: https://gitlist.dev/e/20210125230952.15468-2-jacob%40gitlab.com In-Reply-To: <20210125230952.15468-1-jacob@gitlab.com> ``` This fixes a bug that occurs when you combine partial clone and uploadpack.packobjectshook. You can reproduce it as follows: git clone -u 'git -c uploadpack.allowfilter '\ '-c uploadpack.packobjectshook=env '\ 'upload-pack' --filter=blob:none --no-local \ src.git dst.git Be careful with the line endings because this has a long quoted string as the -u argument. The error I get when I run this is: Cloning into '/tmp/broken'... remote: fatal: invalid filter-spec ''blob:none'' error: git upload-pack: git-pack-objects died with error. fatal: git upload-pack: aborting due to possible repository corruption on the remote side. remote: aborting due to possible repository corruption on the remote side. fatal: early EOF fatal: index-pack failed The problem is an unnecessary and harmful layer of quoting. I tried digging through the history of this function and I think this quoting was there from the start. My best guess is that it stems from a misunderstanding what use_shell=1 means. The code seems to assume it means "arguments get joined into one big string, then fed to /bin/sh". But that is not what it means: use_shell=1 means that the first argument in the arguments array may be a shell script and if so should be passed to /bin/sh. All other arguments are passed as normal arguments. The solution is simple: never quote the filter spec. This commit removes the conditional quoting and adds a test for partial clone in t5544. Signed-off-by: Jacob Vosmaer --- t/t5544-pack-objects-hook.sh | 9 +++++++++ upload-pack.c | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index 4357af1525..f5ba663d64 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' ' test_path_is_missing .git/hook.stdout ' +test_expect_success 'hook works with partial clone' ' + clear_hook_results && + test_config_global uploadpack.packObjectsHook ./hook && + test_config_global uploadpack.allowFilter true && + git clone --bare --no-local --filter=blob:none . dst.git && + git -C dst.git rev-list --objects --missing=print HEAD >objects && + grep "^?" objects +' + test_done diff --git a/upload-pack.c b/upload-pack.c index 3b66bf92ba..eae1fdbc55 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, if (pack_data->filter_options.choice) { const char *spec = expand_list_objects_filter_spec(&pack_data->filter_options); - if (pack_objects.use_shell) { - struct strbuf buf = STRBUF_INIT; - sq_quote_buf(&buf, spec); - strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf); - strbuf_release(&buf); - } else { - strvec_pushf(&pack_objects.args, "--filter=%s", spec); - } + strvec_pushf(&pack_objects.args, "--filter=%s", spec); } if (uri_protocols) { for (i = 0; i < uri_protocols->nr; i++) -- 2.30.0 ``` ## Jacob Vosmaer, 2021-01-25 23:09 Subject: [PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug Message-ID: <20210125230952.15468-1-jacob@gitlab.com> URL: https://gitlist.dev/e/20210125230952.15468-1-jacob%40gitlab.com In-Reply-To: ``` Now with sign-off. Thanks for the reviews and comments! Jacob Jacob Vosmaer (1): upload-pack.c: fix filter spec quoting bug t/t5544-pack-objects-hook.sh | 9 +++++++++ upload-pack.c | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-) -- 2.30.0 ``` ## Jeff King, 2021-01-25 21:16 Subject: Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/YA81LEon1RPzT0T9%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Jan 25, 2021 at 11:48:07AM -0800, Junio C Hamano wrote: > I suspect that 0b6069fe (fetch-pack: test support excluding large > blobs, 2017-12-08) sort-of fixed half of the problem (i.e. the half > when there is no hook used) while leaving the other half still > broken as before. > > But because .use_shell does not affect if we should or should not > quote, we can unconditionally drop the use of sq_quote_buf(). Yep. That was the same conclusion I came to in my earlier reply: https://lore.kernel.org/git/YAs2RMT1rEH%2F2LSp@coredump.intra.peff.net/ > > This commit removes the conditional quoting and adds a test for > > partial clone in t5544. > > --- > > Thanks. Missing sign-off. Aside from the sign-off issue, this version looks good to me. -Peff ``` ## Junio C Hamano, 2021-01-26 00:01 Subject: Re: [PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqqy2ggwoqm.fsf%40gitster.c.googlers.com In-Reply-To: <20210125230952.15468-1-jacob@gitlab.com> ``` Jacob Vosmaer writes: > Now with sign-off. Thanks for the reviews and comments! > > Jacob > > Jacob Vosmaer (1): > upload-pack.c: fix filter spec quoting bug > > t/t5544-pack-objects-hook.sh | 9 +++++++++ > upload-pack.c | 9 +-------- > 2 files changed, 10 insertions(+), 8 deletions(-) Thanks, will queue. ``` ## Jacob Vosmaer, 2021-01-26 10:29 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/CADMWQoP%2BhgPiSHJ5D-3G0%2B-bZOYm1hr%3DojBeSmUJhiS7bp0zPg%40mail.gmail.com In-Reply-To: <874kj46mwf.fsf@evledraar.gmail.com> ``` Thanks for the feedback Ævar. I am not sure if I am still supposed to make changes to the patch now that it is in "seen" as 7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I? Best regards, Jacob Vosmaer GitLab, Inc. On Tue, Jan 26, 2021 at 10:57 AM Ævar Arnfjörð Bjarmason wrote: > > > On Tue, Jan 26 2021, Jacob Vosmaer wrote: > > > This fixes a bug that occurs when you combine partial clone and > > uploadpack.packobjectshook. You can reproduce it as follows: > > Let's: > > * Refer to the commit we're fixing a bug in, i.e. Junio's mention of > 10ac85c7 (upload-pack: add object filtering for partial clone, > 2017-12-08) upthread. > > * See also "imperative-mood" in SubmittingPatches. I.e. say "Fix a bug > in ..." not "This fixes ... can be reproduced as" > > * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C > code. > > > git clone -u 'git -c uploadpack.allowfilter '\ > > '-c uploadpack.packobjectshook=env '\ > > 'upload-pack' --filter=blob:none --no-local \ > > src.git dst.git > > This and the output below would be more readable indented. > > > Be careful with the line endings because this has a long quoted string > > as the -u argument. > > > > The error I get when I run this is: > > > > Cloning into '/tmp/broken'... > > remote: fatal: invalid filter-spec ''blob:none'' > > error: git upload-pack: git-pack-objects died with error. > > fatal: git upload-pack: aborting due to possible repository corruption on the remote side. > > remote: aborting due to possible repository corruption on the remote side. > > fatal: early EOF > > fatal: index-pack failed > > [...] > > > The problem is an unnecessary and harmful layer of quoting. I tried > > digging through the history of this function and I think this quoting > > was there from the start. > > > ...So looked at "git log" but didn't try to check out 10ac85c7 and see > if it had the same issue? If we're going to leave a note about this at > all probably better to help future source spelunkers by being able to > say the issue was there from the start. > > > > My best guess is that it stems from a > > misunderstanding what use_shell=1 means. The code seems to assume it > > means "arguments get joined into one big string, then fed to /bin/sh". > > But that is not what it means: use_shell=1 means that the first > > argument in the arguments array may be a shell script and if so should > > be passed to /bin/sh. All other arguments are passed as normal > > arguments. > > > > The solution is simple: never quote the filter spec. > > > > This commit removes the conditional quoting and adds a test for > > partial clone in t5544. > > > > Thanks for hacking this up! Hopefully the above is helpful and not too > nitpicky. Mainly wanted to help you get future patches through more > easily... > > > Signed-off-by: Jacob Vosmaer > > --- > > t/t5544-pack-objects-hook.sh | 9 +++++++++ > > upload-pack.c | 9 +-------- > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh > > index 4357af1525..f5ba663d64 100755 > > --- a/t/t5544-pack-objects-hook.sh > > +++ b/t/t5544-pack-objects-hook.sh > > @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' ' > > test_path_is_missing .git/hook.stdout > > ' > > > > +test_expect_success 'hook works with partial clone' ' > > + clear_hook_results && > > + test_config_global uploadpack.packObjectsHook ./hook && > > + test_config_global uploadpack.allowFilter true && > > + git clone --bare --no-local --filter=blob:none . dst.git && > > + git -C dst.git rev-list --objects --missing=print HEAD >objects && > > + grep "^?" objects > > +' > > + > > test_done > > diff --git a/upload-pack.c b/upload-pack.c > > index 3b66bf92ba..eae1fdbc55 100644 > > --- a/upload-pack.c > > +++ b/upload-pack.c > > @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, > > if (pack_data->filter_options.choice) { > > const char *spec = > > expand_list_objects_filter_spec(&pack_data->filter_options); > > - if (pack_objects.use_shell) { > > - struct strbuf buf = STRBUF_INIT; > > - sq_quote_buf(&buf, spec); > > - strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf); > > - strbuf_release(&buf); > > - } else { > > - strvec_pushf(&pack_objects.args, "--filter=%s", spec); > > - } > > + strvec_pushf(&pack_objects.args, "--filter=%s", spec); > > } > > if (uri_protocols) { > > for (i = 0; i < uri_protocols->nr; i++) > ``` ## Ævar Arnfjörð Bjarmason, 2021-01-26 09:57 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: <874kj46mwf.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/874kj46mwf.fsf%40evledraar.gmail.com In-Reply-To: <20210125230952.15468-2-jacob@gitlab.com> ``` On Tue, Jan 26 2021, Jacob Vosmaer wrote: > This fixes a bug that occurs when you combine partial clone and > uploadpack.packobjectshook. You can reproduce it as follows: Let's: * Refer to the commit we're fixing a bug in, i.e. Junio's mention of 10ac85c7 (upload-pack: add object filtering for partial clone, 2017-12-08) upthread. * See also "imperative-mood" in SubmittingPatches. I.e. say "Fix a bug in ..." not "This fixes ... can be reproduced as" * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C code. > git clone -u 'git -c uploadpack.allowfilter '\ > '-c uploadpack.packobjectshook=env '\ > 'upload-pack' --filter=blob:none --no-local \ > src.git dst.git This and the output below would be more readable indented. > Be careful with the line endings because this has a long quoted string > as the -u argument. > > The error I get when I run this is: > > Cloning into '/tmp/broken'... > remote: fatal: invalid filter-spec ''blob:none'' > error: git upload-pack: git-pack-objects died with error. > fatal: git upload-pack: aborting due to possible repository corruption on the remote side. > remote: aborting due to possible repository corruption on the remote side. > fatal: early EOF > fatal: index-pack failed [...] > The problem is an unnecessary and harmful layer of quoting. I tried > digging through the history of this function and I think this quoting > was there from the start. ...So looked at "git log" but didn't try to check out 10ac85c7 and see if it had the same issue? If we're going to leave a note about this at all probably better to help future source spelunkers by being able to say the issue was there from the start. > My best guess is that it stems from a > misunderstanding what use_shell=1 means. The code seems to assume it > means "arguments get joined into one big string, then fed to /bin/sh". > But that is not what it means: use_shell=1 means that the first > argument in the arguments array may be a shell script and if so should > be passed to /bin/sh. All other arguments are passed as normal > arguments. > > The solution is simple: never quote the filter spec. > > This commit removes the conditional quoting and adds a test for > partial clone in t5544. > Thanks for hacking this up! Hopefully the above is helpful and not too nitpicky. Mainly wanted to help you get future patches through more easily... > Signed-off-by: Jacob Vosmaer > --- > t/t5544-pack-objects-hook.sh | 9 +++++++++ > upload-pack.c | 9 +-------- > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh > index 4357af1525..f5ba663d64 100755 > --- a/t/t5544-pack-objects-hook.sh > +++ b/t/t5544-pack-objects-hook.sh > @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' ' > test_path_is_missing .git/hook.stdout > ' > > +test_expect_success 'hook works with partial clone' ' > + clear_hook_results && > + test_config_global uploadpack.packObjectsHook ./hook && > + test_config_global uploadpack.allowFilter true && > + git clone --bare --no-local --filter=blob:none . dst.git && > + git -C dst.git rev-list --objects --missing=print HEAD >objects && > + grep "^?" objects > +' > + > test_done > diff --git a/upload-pack.c b/upload-pack.c > index 3b66bf92ba..eae1fdbc55 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, > if (pack_data->filter_options.choice) { > const char *spec = > expand_list_objects_filter_spec(&pack_data->filter_options); > - if (pack_objects.use_shell) { > - struct strbuf buf = STRBUF_INIT; > - sq_quote_buf(&buf, spec); > - strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf); > - strbuf_release(&buf); > - } else { > - strvec_pushf(&pack_objects.args, "--filter=%s", spec); > - } > + strvec_pushf(&pack_objects.args, "--filter=%s", spec); > } > if (uri_protocols) { > for (i = 0; i < uri_protocols->nr; i++) ``` ## Junio C Hamano, 2021-01-26 02:25 Subject: Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqqlfcg4ep2.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jeff King writes: > On Mon, Jan 25, 2021 at 11:48:07AM -0800, Junio C Hamano wrote: > >> I suspect that 0b6069fe (fetch-pack: test support excluding large >> blobs, 2017-12-08) sort-of fixed half of the problem (i.e. the half >> when there is no hook used) while leaving the other half still >> broken as before. >> >> But because .use_shell does not affect if we should or should not >> quote, we can unconditionally drop the use of sq_quote_buf(). > > Yep. That was the same conclusion I came to in my earlier reply: > > https://lore.kernel.org/git/YAs2RMT1rEH%2F2LSp@coredump.intra.peff.net/ Thanks. As I am behind, I needed to think it aloud to make sure I am on the same page as others ;-) A sanity check like this is greatly appreciated. ``` ## Junio C Hamano, 2021-01-26 17:46 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqqpn1r3827.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jacob Vosmaer writes: > Thanks for the feedback Ævar. I am not sure if I am still supposed to > make changes to the patch now that it is in "seen" as > 7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I? Being in 'seen' does not mean all that much. You are free to improve your patch by replacing until you and everybody else please. It is when it hits 'next' that replacement no longer becomes feasible and it gets cast in stone and goes on track to be hopefully part of the next release. Thanks. ``` ## Junio C Hamano, 2021-01-26 17:51 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqqlfcf37tk.fsf%40gitster.c.googlers.com In-Reply-To: <874kj46mwf.fsf@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: > On Tue, Jan 26 2021, Jacob Vosmaer wrote: > >> This fixes a bug that occurs when you combine partial clone and >> uploadpack.packobjectshook. You can reproduce it as follows: > > Let's: > > * Refer to the commit we're fixing a bug in, i.e. Junio's mention of > 10ac85c7 (upload-pack: add object filtering for partial clone, > 2017-12-08) upthread. > > * See also "imperative-mood" in SubmittingPatches. I.e. say "Fix a bug > in ..." not "This fixes ... can be reproduced as" > > * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C > code. > > ... [jc: all the helpful hints snipped] ... > > Thanks for hacking this up! Hopefully the above is helpful and not too > nitpicky. Mainly wanted to help you get future patches through more > easily... Yeah, thanks, both, for aiming higher ;-) I have to admit that I did find the log message a bit lacking, and that was why I had to dig bit to find out how the historical issue happened myself in the first place, and I tend to agree that it feels a bit of waste for that work to end up buried in the list archive without getting reflected in the proposed log message. ``` ## Jeff King, 2021-01-26 21:07 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/YBCEmZHapifbVqRy%40coredump.intra.peff.net In-Reply-To: <874kj46mwf.fsf@evledraar.gmail.com> ``` On Tue, Jan 26, 2021 at 10:57:36AM +0100, Ævar Arnfjörð Bjarmason wrote: > > This fixes a bug that occurs when you combine partial clone and > > uploadpack.packobjectshook. You can reproduce it as follows: > > Let's: > > * Refer to the commit we're fixing a bug in, i.e. Junio's mention of > 10ac85c7 (upload-pack: add object filtering for partial clone, > 2017-12-08) upthread. > > * See also "imperative-mood" in SubmittingPatches. I.e. say "Fix a bug > in ..." not "This fixes ... can be reproduced as" > > * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C > code. Generally good advice (the imperative stuff IMHO is less important outside of the subject line, but a reasonable default way of writing). > > The problem is an unnecessary and harmful layer of quoting. I tried > > digging through the history of this function and I think this quoting > > was there from the start. > > > ...So looked at "git log" but didn't try to check out 10ac85c7 and see > if it had the same issue? If we're going to leave a note about this at > all probably better to help future source spelunkers by being able to > say the issue was there from the start. I don't think it could be tested easily at that point. It implemented the server side, but not the client side. And later when the client side was added, the non-hook code path was fixed. (More discussion earlier in the thread). -Peff ``` ## Jeff King, 2021-01-26 21:09 Subject: Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/YBCFBivBLgqEAUr1%40coredump.intra.peff.net In-Reply-To: ``` On Tue, Jan 26, 2021 at 11:29:55AM +0100, Jacob Vosmaer wrote: > Thanks for the feedback Ævar. I am not sure if I am still supposed to > make changes to the patch now that it is in "seen" as > 7c6e2ea381d9aafe0a1eff0616013f81d957c0fd. Am I? It's OK to send re-rolls of patches that are in "seen". That branch is rewound and rewritten regularly as part of Junio's integration cycles. Once a commit is in "next", then it is generally considered set in stone, and fixes should come on top rather than as re-rolls. (There is one exception; "next" is rewound after a release, so that is an opportunity for topics that were so muddled in their earlier incarnation to get rewritten. That is pretty rare, though). -Peff ``` ## Jacob Vosmaer, 2021-01-28 16:04 Subject: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: <20210128160453.79169-1-jacob@gitlab.com> URL: https://gitlist.dev/e/20210128160453.79169-1-jacob%40gitlab.com In-Reply-To: ``` Fix a bug in upload-pack.c that occurs when you combine partial clone and uploadpack.packObjectsHook. You can reproduce it as follows: git clone -u 'git -c uploadpack.allowfilter '\ '-c uploadpack.packobjectshook=env '\ 'upload-pack' --filter=blob:none --no-local \ src.git dst.git Be careful with the line endings because this has a long quoted string as the -u argument. The error I get when I run this is: Cloning into '/tmp/broken'... remote: fatal: invalid filter-spec ''blob:none'' error: git upload-pack: git-pack-objects died with error. fatal: git upload-pack: aborting due to possible repository corruption on the remote side. remote: aborting due to possible repository corruption on the remote side. fatal: early EOF fatal: index-pack failed The problem is caused by unneeded quoting. This bug was already present in 10ac85c785 (upload-pack: add object filtering for partial clone, 2017-12-08) when the server side filter support was introduced. In fact, in 10ac85c785 this was broken regardless of uploadpack.packObjectsHook. Then in 0b6069fe0a (fetch-pack: test support excluding large blobs, 2017-12-08) the quoting was removed but only behind a conditional that depends on whether uploadpack.packObjectsHook is set. Because uploadpack.packObjectsHook is apparently rarely used, nobody noticed the problematic quoting could still happen. This commit removes the conditional quoting and adds a test for partial clone in t5544-pack-objects-hook. Signed-off-by: Jacob Vosmaer --- t/t5544-pack-objects-hook.sh | 9 +++++++++ upload-pack.c | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index 4357af1525..f5ba663d64 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -59,4 +59,13 @@ test_expect_success 'hook does not run from repo config' ' test_path_is_missing .git/hook.stdout ' +test_expect_success 'hook works with partial clone' ' + clear_hook_results && + test_config_global uploadpack.packObjectsHook ./hook && + test_config_global uploadpack.allowFilter true && + git clone --bare --no-local --filter=blob:none . dst.git && + git -C dst.git rev-list --objects --missing=print HEAD >objects && + grep "^?" objects +' + test_done diff --git a/upload-pack.c b/upload-pack.c index 3b66bf92ba..eae1fdbc55 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, if (pack_data->filter_options.choice) { const char *spec = expand_list_objects_filter_spec(&pack_data->filter_options); - if (pack_objects.use_shell) { - struct strbuf buf = STRBUF_INIT; - sq_quote_buf(&buf, spec); - strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf); - strbuf_release(&buf); - } else { - strvec_pushf(&pack_objects.args, "--filter=%s", spec); - } + strvec_pushf(&pack_objects.args, "--filter=%s", spec); } if (uri_protocols) { for (i = 0; i < uri_protocols->nr; i++) -- 2.30.0 ``` ## Jacob Vosmaer, 2021-01-28 21:12 Subject: Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG%3DpdDmc1bJr%3Dg%40mail.gmail.com In-Reply-To: ``` On Thu, Jan 28, 2021 at 8:12 PM Junio C Hamano wrote: > * As readers know "clone" would get not just the current branch, > the way this rev-list traverses only from HEAD makes them wonder > if you are trying to exclude refs other than the current branch > for a reason. Better write it as > > git -C dst.git rev-list --objects --missing=print --all >objects && Good point! > * The above says that we are happy if we can clone without erroring > out, as long as some objects are missing, but we could go a bit > stronger than that: among the objects we have, none should be a > blob object. Is that something we can easily check? > > Something along the lines of... > > grep -v "^?" objects | > git -C dst.git cat-file --batch-check="%(objecttype)" >types && > sed -e '/^commit/d' -e '/^tag/d' -e '/^tree/d' types >actual && > test_must_be_empty actual > > ... to ensure everything is either commit, tag or tree, perhaps? Makes sense, I started down that route at first but I bumped my head against needing --missing to prevent the lazy fetching, and stopped once I had the question mark grep. Now that we're talking about the test, I was wondering about something else. In my original patch, I purposely did not add a test. Why? Because --filter is just one option of several that upload-pack passes to pack-objects (think of --shallow-file and --include-tag, for instance). Why is --filter special? If the original quoting bug had not happened, would we be testing various permutations of clone options in combination with packObjectsHook? As a reader looking at t5544, unless I know the backstory of the bug, I do not understand why --filter gets a test but those other things do not. I am not saying this to push back on going back and improving the test, I'm quite happy to. It's more that deleting the test may be the ultimate improvement. Thanks in advance for humoring this contrarian suggestion. :) Cheers, Jacob ``` ## Jacob Vosmaer, 2021-01-28 21:40 Subject: Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/CADMWQoOVe1cfeO6ncL1_E8oHxp_ovK-RViT-DZLiCDhkWWkPZA%40mail.gmail.com In-Reply-To: ``` On Thu, Jan 28, 2021 at 10:12 PM Jacob Vosmaer wrote: > Thanks in advance for humoring this contrarian > suggestion. :) English is not my first language. I should have said "considering", not "humoring". ``` ## Jeff King, 2021-01-28 21:51 Subject: Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/YBMx7yuBit8TecYp%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Jan 28, 2021 at 10:12:42PM +0100, Jacob Vosmaer wrote: > Now that we're talking about the test, I was wondering about something else. > > In my original patch, I purposely did not add a test. Why? Because > --filter is just one option of several that upload-pack passes to > pack-objects (think of --shallow-file and --include-tag, for > instance). Why is --filter special? If the original quoting bug had > not happened, would we be testing various permutations of clone > options in combination with packObjectsHook? > > As a reader looking at t5544, unless I know the backstory of the bug, > I do not understand why --filter gets a test but those other things do > not. You're definitely not wrong that this is somewhat closing the barn door after horse has left, and there may be other barns still to be fixed. But usually we'll add a test that demonstrates the breakage, if only because the fact that something so mundane and easy to trigger _wasn't_ caught by the existing test is pretty bad. So we should at least make sure the combination is now covered, which your test does. And then over time we build up a set of coverage. Of course it is sometimes nice to be exhaustive, too. The organically built-up set of tests is going to have holes if it's not done systematically. But Git also isn't a black box; we know that the implementation of this option was weirdly different. I guess the argument you are making is that now that it's fixed, it _isn't_ different, and we're unlikely to reintroduce the bug. I can buy that, but I think we just do the "test the fixed bug" thing on principle. -Peff ``` ## Junio C Hamano, 2021-01-28 21:58 Subject: Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqqpn1ovi4j.fsf%40gitster.c.googlers.com In-Reply-To: ``` Jacob Vosmaer writes: > As a reader looking at t5544, unless I know the backstory of the bug, > I do not understand why --filter gets a test but those other things do > not. Good point. Perhaps a retitle of the test or a bit of comment would benefit future readers. # git clone internally has to invoke upload-pack on the # other end with multiple arguments, and it used to quote # them incorrectly only when hooks are enabled. test_expect_success 'hook works with partial clone' ' clear_hook_results && test_config_global uploadpack.packObjectsHook ./hook && test_config_global uploadpack.allowFilter true && git clone --bare --no-local --filter=blob:none . dst.git && git -C dst.git rev-list --objects --missing=print HEAD >objects && grep "^?" objects ' But that may be overkill. Those curious can run "git blame" to go back to what you wrote in the log message, and that should be clear enough for them why we care about this case. And from that point of view, it may be sufficient that the resulting repository lacks "some" objects and not necessarily check what are missing. Thanks. ``` ## Jacob Vosmaer, 2021-02-01 20:29 Subject: [PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug Message-ID: <20210201202938.39576-1-jacob@gitlab.com> URL: https://gitlist.dev/e/20210201202938.39576-1-jacob%40gitlab.com In-Reply-To: ``` I updated the test based on the latest round of feedback. Jacob Vosmaer (1): upload-pack.c: fix filter spec quoting bug t/t5544-pack-objects-hook.sh | 10 ++++++++++ upload-pack.c | 9 +-------- 2 files changed, 11 insertions(+), 8 deletions(-) -- 2.30.0 ``` ## Jacob Vosmaer, 2021-02-01 20:29 Subject: [PATCH v5 1/1] upload-pack.c: fix filter spec quoting bug Message-ID: <20210201202938.39576-2-jacob@gitlab.com> URL: https://gitlist.dev/e/20210201202938.39576-2-jacob%40gitlab.com In-Reply-To: <20210201202938.39576-1-jacob@gitlab.com> ``` Fix a bug in upload-pack.c that occurs when you combine partial clone and uploadpack.packObjectsHook. You can reproduce it as follows: git clone -u 'git -c uploadpack.allowfilter '\ '-c uploadpack.packobjectshook=env '\ 'upload-pack' --filter=blob:none --no-local \ src.git dst.git Be careful with the line endings because this has a long quoted string as the -u argument. The error I get when I run this is: Cloning into '/tmp/broken'... remote: fatal: invalid filter-spec ''blob:none'' error: git upload-pack: git-pack-objects died with error. fatal: git upload-pack: aborting due to possible repository corruption on the remote side. remote: aborting due to possible repository corruption on the remote side. fatal: early EOF fatal: index-pack failed The problem is caused by unneeded quoting. This bug was already present in 10ac85c785 (upload-pack: add object filtering for partial clone, 2017-12-08) when the server side filter support was introduced. In fact, in 10ac85c785 this was broken regardless of uploadpack.packObjectsHook. Then in 0b6069fe0a (fetch-pack: test support excluding large blobs, 2017-12-08) the quoting was removed but only behind a conditional that depends on whether uploadpack.packObjectsHook is set. Because uploadpack.packObjectsHook is apparently rarely used, nobody noticed the problematic quoting could still happen. This commit removes the conditional quoting and adds a test for partial clone in t5544-pack-objects-hook. Signed-off-by: Jacob Vosmaer --- t/t5544-pack-objects-hook.sh | 10 ++++++++++ upload-pack.c | 9 +-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index 4357af1525..dd5f44d986 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -59,4 +59,14 @@ test_expect_success 'hook does not run from repo config' ' test_path_is_missing .git/hook.stdout ' +test_expect_success 'hook works with partial clone' ' + clear_hook_results && + test_config_global uploadpack.packObjectsHook ./hook && + test_config_global uploadpack.allowFilter true && + git clone --bare --no-local --filter=blob:none . dst.git && + git -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects && + git -C dst.git cat-file --batch-check="%(objecttype)" types && + ! grep blob types +' + test_done diff --git a/upload-pack.c b/upload-pack.c index 3b66bf92ba..eae1fdbc55 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -305,14 +305,7 @@ static void create_pack_file(struct upload_pack_data *pack_data, if (pack_data->filter_options.choice) { const char *spec = expand_list_objects_filter_spec(&pack_data->filter_options); - if (pack_objects.use_shell) { - struct strbuf buf = STRBUF_INIT; - sq_quote_buf(&buf, spec); - strvec_pushf(&pack_objects.args, "--filter=%s", buf.buf); - strbuf_release(&buf); - } else { - strvec_pushf(&pack_objects.args, "--filter=%s", spec); - } + strvec_pushf(&pack_objects.args, "--filter=%s", spec); } if (uri_protocols) { for (i = 0; i < uri_protocols->nr; i++) -- 2.30.0 ``` ## Jacob Vosmaer, 2021-02-01 20:31 Subject: Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/CADMWQoMap_wRTtcTbR8NeDWR6pKq876FEy3WdpFQwEkOR6E%2Bmg%40mail.gmail.com In-Reply-To: ``` On Thu, Jan 28, 2021 at 10:51 PM Jeff King wrote: > I guess the argument you are making is that now that it's fixed, it > _isn't_ different, and we're unlikely to reintroduce the bug. I can buy > that, but I think we just do the "test the fixed bug" thing on > principle. Makes sense, thanks for explaining! ``` ## Junio C Hamano, 2021-02-02 05:49 Subject: Re: [PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug Message-ID: URL: https://gitlist.dev/e/xmqq8s87hvd7.fsf%40gitster.c.googlers.com In-Reply-To: <20210201202938.39576-1-jacob@gitlab.com> ``` Jacob Vosmaer writes: > I updated the test based on the latest round of feedback. Thanks, but this came a bit too late to wholesale replace the patch; the previous round was good enough and is in 'next' already. Looking at the change between two versions, I guess that the change in tests may be worth salvaging, so perhaps you can make it into an incremental "the test we added earlier was good, but let's make it even better by doing X and Y" patch to be applied on top? Thanks. > Jacob Vosmaer (1): > upload-pack.c: fix filter spec quoting bug > > t/t5544-pack-objects-hook.sh | 10 ++++++++++ > upload-pack.c | 9 +-------- > 2 files changed, 11 insertions(+), 8 deletions(-) ``` ## Jacob Vosmaer, 2021-02-02 10:37 Subject: [PATCH 1/1] t5544: clarify 'hook works with partial clone' test Message-ID: <20210202103733.42513-1-jacob@gitlab.com> URL: https://gitlist.dev/e/20210202103733.42513-1-jacob%40gitlab.com In-Reply-To: ``` Apply a few leftover improvements from the review of ad5df6b782 (upload-pack.c: fix filter spec quoting bug). 1. Instead of enumerating objects reachable from HEAD, enumerate all reachable objects, because HEAD has not special significance in this test. 2. Instead of relying on the knowledge that "? in rev-list output means partial clone", explicitly verify that there are no blobs with cat-file. --- t/t5544-pack-objects-hook.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index f5ba663d64..dd5f44d986 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' ' test_config_global uploadpack.packObjectsHook ./hook && test_config_global uploadpack.allowFilter true && git clone --bare --no-local --filter=blob:none . dst.git && - git -C dst.git rev-list --objects --missing=print HEAD >objects && - grep "^?" objects + git -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects && + git -C dst.git cat-file --batch-check="%(objecttype)" types && + ! grep blob types ' test_done -- 2.30.0 ``` ## Eric Sunshine, 2021-02-02 17:22 Subject: Re: [PATCH 1/1] t5544: clarify 'hook works with partial clone' test Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQoTpsYS%2B2pccHmzx%3DXoc34u166cYoC%3DGAcEndofN4gcA%40mail.gmail.com In-Reply-To: <20210202103733.42513-1-jacob@gitlab.com> ``` On Tue, Feb 2, 2021 at 5:40 AM Jacob Vosmaer wrote: > Apply a few leftover improvements from the review of ad5df6b782 > (upload-pack.c: fix filter spec quoting bug). > > 1. Instead of enumerating objects reachable from HEAD, enumerate all > reachable objects, because HEAD has not special significance in this > test. > > 2. Instead of relying on the knowledge that "? in rev-list output > means partial clone", explicitly verify that there are no blobs with > cat-file. > --- Missing Signed-off-by:. ``` ## Jacob Vosmaer, 2021-02-02 19:24 Subject: [PATCH v2] t5544: clarify 'hook works with partial clone' test Message-ID: <20210202192417.68663-1-jacob@gitlab.com> URL: https://gitlist.dev/e/20210202192417.68663-1-jacob%40gitlab.com In-Reply-To: ``` Apply a few leftover improvements from the review of ad5df6b782 (upload-pack.c: fix filter spec quoting bug). 1. Instead of enumerating objects reachable from HEAD, enumerate all reachable objects, because HEAD has not special significance in this test. 2. Instead of relying on the knowledge that "? in rev-list output means partial clone", explicitly verify that there are no blobs with cat-file. Signed-off-by: Jacob Vosmaer --- t/t5544-pack-objects-hook.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh index f5ba663d64..dd5f44d986 100755 --- a/t/t5544-pack-objects-hook.sh +++ b/t/t5544-pack-objects-hook.sh @@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' ' test_config_global uploadpack.packObjectsHook ./hook && test_config_global uploadpack.allowFilter true && git clone --bare --no-local --filter=blob:none . dst.git && - git -C dst.git rev-list --objects --missing=print HEAD >objects && - grep "^?" objects + git -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects && + git -C dst.git cat-file --batch-check="%(objecttype)" types && + ! grep blob types ' test_done -- 2.30.0 ``` ## Junio C Hamano, 2021-02-02 20:21 Subject: Re: [PATCH v2] t5544: clarify 'hook works with partial clone' test Message-ID: URL: https://gitlist.dev/e/xmqqtuqufcfa.fsf%40gitster.c.googlers.com In-Reply-To: <20210202192417.68663-1-jacob@gitlab.com> ``` Jacob Vosmaer writes: > Apply a few leftover improvements from the review of ad5df6b782 > (upload-pack.c: fix filter spec quoting bug). > > 1. Instead of enumerating objects reachable from HEAD, enumerate all > reachable objects, because HEAD has not special significance in this > test. > > 2. Instead of relying on the knowledge that "? in rev-list output > means partial clone", explicitly verify that there are no blobs with > cat-file. > > Signed-off-by: Jacob Vosmaer > --- > t/t5544-pack-objects-hook.sh | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Thanks. > > diff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh > index f5ba663d64..dd5f44d986 100755 > --- a/t/t5544-pack-objects-hook.sh > +++ b/t/t5544-pack-objects-hook.sh > @@ -64,8 +64,9 @@ test_expect_success 'hook works with partial clone' ' > test_config_global uploadpack.packObjectsHook ./hook && > test_config_global uploadpack.allowFilter true && > git clone --bare --no-local --filter=blob:none . dst.git && > - git -C dst.git rev-list --objects --missing=print HEAD >objects && > - grep "^?" objects > + git -C dst.git rev-list --objects --missing=allow-any --no-object-names --all >objects && > + git -C dst.git cat-file --batch-check="%(objecttype)" types && > + ! grep blob types > ' > > test_done ```