| Message ID | 20210125230952.15468-2-jacob@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | upload-pack.c: fix filter spec quoting bug | expand |
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 <jacob@gitlab.com> > --- > 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++)
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 <avarab@gmail.com> 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 <jacob@gitlab.com> > > --- > > 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++) >
Jacob Vosmaer <jacob@gitlab.com> 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
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
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
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++)
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 <jacob@gitlab.com> --- t/t5544-pack-objects-hook.sh | 9 +++++++++ upload-pack.c | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-)