From: Junio C Hamano <hidden> Date: 2021-01-25 20:43:42
Jacob Vosmaer [off-list ref] writes:
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.
Meaning that 10ac85c7 (upload-pack: add object filtering for partial
clone, 2017-12-08) that added:
if (filter_options.filter_spec) {
struct strbuf buf = STRBUF_INIT;
sq_quote_buf(&buf, filter_options.filter_spec);
argv_array_pushf(&pack_objects.args, "--filter=%s", buf.buf);
strbuf_release(&buf);
}
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.
I noticed another thing that hasn't changed since that commit, which
is that the setting of .use_shell is conditional. In today's code,
at the beginning of create_pack_file(), we have
if (!pack_data->pack_objects_hook)
pack_objects.git_cmd = 1;
else {
strvec_push(&pack_objects.args, pack_data->pack_objects_hook);
strvec_push(&pack_objects.args, "git");
pack_objects.use_shell = 1;
}
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().
The solution is simple: never quote the filter spec.
Which makes sense.
This commit removes the conditional quoting and adds a test for
partial clone in t5544.
---
Thanks. Missing sign-off.
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++)
From: Jeff King <hidden> Date: 2021-01-25 23:47:12
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().
From: Jacob Vosmaer <hidden> Date: 2021-01-25 23:11:47
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 <redacted>
---
t/t5544-pack-objects-hook.sh | 9 +++++++++
upload-pack.c | 9 +--------
2 files changed, 10 insertions(+), 8 deletions(-)
@@ -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_globaluploadpack.packObjectsHook./hook&&+test_config_globaluploadpack.allowFiltertrue&&+gitclone--bare--no-local--filter=blob:none.dst.git&&+git-Cdst.gitrev-list--objects--missing=printHEAD>objects&&+grep"^?"objects+'+ test_done
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.
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...
@@ -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_globaluploadpack.packObjectsHook./hook&&+test_config_globaluploadpack.allowFiltertrue&&+gitclone--bare--no-local--filter=blob:none.dst.git&&+git-Cdst.gitrev-list--objects--missing=printHEAD>objects&&+grep"^?"objects+'+ test_done
From: Jacob Vosmaer <hidden> Date: 2021-01-26 12:35:33
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
[off-list ref] wrote:
On Tue, Jan 26 2021, Jacob Vosmaer wrote:
quoted
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.
This and the output below would be more readable indented.
quoted
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
[...]
quoted
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.
quoted
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...
@@ -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_globaluploadpack.packObjectsHook./hook&&+test_config_globaluploadpack.allowFiltertrue&&+gitclone--bare--no-local--filter=blob:none.dst.git&&+git-Cdst.gitrev-list--objects--missing=printHEAD>objects&&+grep"^?"objects+'+ test_done
From: Jeff King <hidden> Date: 2021-01-26 22:19:57
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
From: Jacob Vosmaer <hidden> Date: 2021-01-28 16:07:03
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 <redacted>
---
t/t5544-pack-objects-hook.sh | 9 +++++++++
upload-pack.c | 9 +--------
2 files changed, 10 insertions(+), 8 deletions(-)
@@ -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_globaluploadpack.packObjectsHook./hook&&+test_config_globaluploadpack.allowFiltertrue&&+gitclone--bare--no-local--filter=blob:none.dst.git&&+git-Cdst.gitrev-list--objects--missing=printHEAD>objects&&+grep"^?"objects+'+ test_done
From: Jeff King <hidden> Date: 2021-01-26 22:19:57
On Tue, Jan 26, 2021 at 10:57:36AM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
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).
quoted
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