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@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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++)
>
