[PATCH 0/1] upload-pack.c: fix filter spec quoting bug

[PATCH 0/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

This fixes a bug in upload-pack.c that only happens when you combine
partial clone with a pack-objects hook.


Jacob Vosmaer (1):
  upload-pack.c: fix filter spec quoting bug

 upload-pack.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

-- 
2.30.0

[PATCH 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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=" exec" '\
'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. Note that there is an intentional space before
'exec'. Without that space, run-command.c tries to be smart and the
command fails for the wrong reason.

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 of 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.
---
 upload-pack.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

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

Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug

[Jeff King]

On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:

> 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=" exec" '\
> '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. Note that there is an intentional space before
> 'exec'. Without that space, run-command.c tries to be smart and the
> command fails for the wrong reason.

The "-u" command is run with a shell, so:

  git clone \
    -u 'git -c uploadpack.allowfilter \
            -c uploadpack.packobjectshook=env \
	    upload-pack' \
  --filter=blob:none --no-local src.git dst.git

may be a more readable version. I also found the use of " exec" clever,
but rather subtle; you need the extra space so that our "don't bother
using a shell" run-command optimization does not kick in. I replaced it
with "env" here, which is a slightly more canonical way of running a
sub-program that does not rely on shell builtins.

But all of this should be added as a new test, probably in t5544 with
the other pack-objects hook tests.

> 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 of 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.

Yeah, that is exactly right. "use_shell" just means that the command is
(possibly) run with a shell. Quoting for any extra arguments is handled
automatically.

I think you're correct that this was broken from the start in 10ac85c785
(upload-pack: add object filtering for partial clone, 2017-12-08).
That's even before the use_shell was added, and then later it was pushed
into that conditional by 0b6069fe0a (fetch-pack: test support excluding
large blobs, 2017-12-08). Presumably because the non-hook path would not
have worked at all, and that was the first time any of it was actually
tested. ;)

(I've cc'd authors of those commits as an FYI; I think both were
relatively new to the project at the time so misunderstanding this
subtlety of run-command is not too surprising).

I'm somewhat embarrassed to say that despite being the one who added the
pack-objects hook 4 years ago, we still have not switched over to it at
GitHub from our custom patch (the reason is just mundane; there's some
other adjustments that would have to happen and nobody has ever quite
gotten around to it). Presumably you are looking to use it at GitLab.
Just beware that you are probably treading new-ish ground, so there may
be other bugs like this lurking.

> 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);

Yep, this looks like the right fix. I think with an addition to the test
suite, this will be good to go.

-Peff

[PATCH] run-command: document use_shell option

[Jeff King]

On Fri, Jan 22, 2021 at 03:32:04PM -0500, Jeff King wrote:

> Yeah, that is exactly right. "use_shell" just means that the command is
> (possibly) run with a shell. Quoting for any extra arguments is handled
> automatically.
> 
> I think you're correct that this was broken from the start in 10ac85c785
> (upload-pack: add object filtering for partial clone, 2017-12-08).
> That's even before the use_shell was added, and then later it was pushed
> into that conditional by 0b6069fe0a (fetch-pack: test support excluding
> large blobs, 2017-12-08). Presumably because the non-hook path would not
> have worked at all, and that was the first time any of it was actually
> tested. ;)
> 
> (I've cc'd authors of those commits as an FYI; I think both were
> relatively new to the project at the time so misunderstanding this
> subtlety of run-command is not too surprising).

While we're thinking about it, let's beef up the documentation a bit.

-- >8 --
Subject: [PATCH] run-command: document use_shell option

It's unclear how run-command's use_shell option should impact the
arguments fed to a command. Plausibly it could mean that we glue all of
the arguments together into a string to pass to the shell, in which case
that opens the question of whether the caller needs to quote them.

But in fact we don't implement it that way (and even if we did, we'd
probably auto-quote the arguments as part of the glue step). And we must
not receive quoted arguments, because we might actually optimize out the
shell entirely (i.e., the caller does not even know if a shell will be
involved in the end or not).

Since this ambiguity may have been the cause of a recent bug, let's
document the option a bit.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/run-command.h b/run-command.h
index 6472b38bde..d08414a92e 100644
--- a/run-command.h
+++ b/run-command.h
@@ -126,8 +126,15 @@ struct child_process {
 	 */
 	unsigned silent_exec_failure:1;
 
-	unsigned stdout_to_stderr:1;
+	/**
+	 * Run the command from argv[0] using a shell (but note that we may
+	 * still optimize out the shell call if the command contains no
+	 * metacharacters). Note that further arguments to the command in
+	 * argv[1], etc, do not need to be shell-quoted.
+	 */
 	unsigned use_shell:1;
+
+	unsigned stdout_to_stderr:1;
 	unsigned clean_on_exit:1;
 	unsigned wait_after_clean:1;
 	void (*clean_on_exit_handler)(struct child_process *process);
-- 
2.30.0.664.g35e6628185

Re: [PATCH] run-command: document use_shell option

[Taylor Blau]

On Fri, Jan 22, 2021 at 04:03:33PM -0500, Jeff King wrote:
> Subject: [PATCH] run-command: document use_shell option

Nice. This does indeed make things a little clearer (and as we've seen
it was certainly not as clear before), so I think this is worth doing.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH] run-command: document use_shell option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/run-command.h b/run-command.h
> index 6472b38bde..d08414a92e 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -126,8 +126,15 @@ struct child_process {
>  	 */
>  	unsigned silent_exec_failure:1;
>  
> -	unsigned stdout_to_stderr:1;
> +	/**
> +	 * Run the command from argv[0] using a shell (but note that we may
> +	 * still optimize out the shell call if the command contains no
> +	 * metacharacters). Note that further arguments to the command in
> +	 * argv[1], etc, do not need to be shell-quoted.
> +	 */
>  	unsigned use_shell:1;
> +
> +	unsigned stdout_to_stderr:1;

Reads well.  Thanks.

It is curious why "diff" chose to move stdout_to_stderr line around,
though.

>  	unsigned clean_on_exit:1;
>  	unsigned wait_after_clean:1;
>  	void (*clean_on_exit_handler)(struct child_process *process);

Re: [PATCH] run-command: document use_shell option

[Jeff King]

On Fri, Jan 22, 2021 at 02:21:15PM -0800, Junio C Hamano wrote:

> > -	unsigned stdout_to_stderr:1;
> > +	/**
> > +	 * Run the command from argv[0] using a shell (but note that we may
> > +	 * still optimize out the shell call if the command contains no
> > +	 * metacharacters). Note that further arguments to the command in
> > +	 * argv[1], etc, do not need to be shell-quoted.
> > +	 */
> >  	unsigned use_shell:1;
> > +
> > +	unsigned stdout_to_stderr:1;
> 
> Reads well.  Thanks.
> 
> It is curious why "diff" chose to move stdout_to_stderr line around,
> though.

Heh, I also noticed that and tried a few options to no avail (both
patience/histogram, but also diff.indentHeuristic). It is one deletion
and 8 insertions even if it is done the other way:

  + /*
  + ... 6 more lines ...
  + */
  + unsigned use_shell:1;
  +
    unsigned stdout_to_stderr:1;
  - unsigned use_shell:1;

so it may just come down to the order of xdiff parsing the lines.

-Peff

Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:
>
>> This fixes a bug that occurs when you combine partial clone and
>> uploadpack.packobjectshook. You can reproduce it as follows:
> ...
> I'm somewhat embarrassed to say that despite being the one who added the
> pack-objects hook 4 years ago, we still have not switched over to it at
> GitHub from our custom patch (the reason is just mundane; there's some
> other adjustments that would have to happen and nobody has ever quite
> gotten around to it). Presumably you are looking to use it at GitLab.
> Just beware that you are probably treading new-ish ground, so there may
> be other bugs like this lurking.
>
>> 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);
>
> Yep, this looks like the right fix. I think with an addition to the test
> suite, this will be good to go.

Yeah, that looks simpler and better.  It does deserve new tests.

Thanks, both.

[PATCH v2] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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.
---
 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

Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jacob Vosmaer <jacob@gitlab.com> 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++)

Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug

[Jeff King]

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

[PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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

[PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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(-)

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

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Ævar Arnfjörð Bjarmason]

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++)

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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++)
>

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

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.

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Jeff King]

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

[PATCH v4] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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 <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++)
-- 
2.30.0

Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

On Thu, Jan 28, 2021 at 8:12 PM Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

On Thu, Jan 28, 2021 at 10:12 PM Jacob Vosmaer <jacob@gitlab.com> wrote:
> Thanks in advance for humoring this contrarian
> suggestion. :)

English is not my first language. I should have said "considering",
not "humoring".

Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

[Jeff King]

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

Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

On Thu, Jan 28, 2021 at 10:51 PM Jeff King <peff@peff.net> 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!

Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jacob Vosmaer <jacob@gitlab.com> 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.

[PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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

[PATCH v5 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

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 <jacob@gitlab.com>
---
 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)" <objects >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

Re: [PATCH v5 0/1] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jacob Vosmaer <jacob@gitlab.com> 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(-)

[PATCH 1/1] t5544: clarify 'hook works with partial clone' test

[Jacob Vosmaer]

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)" <objects >types &&
+	! grep blob types
 '
 
 test_done
-- 
2.30.0

Re: [PATCH 1/1] t5544: clarify 'hook works with partial clone' test

[Eric Sunshine]

On Tue, Feb 2, 2021 at 5:40 AM Jacob Vosmaer <jacob@gitlab.com> 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:.

[PATCH v2] t5544: clarify 'hook works with partial clone' test

[Jacob Vosmaer]

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 <jacob@gitlab.com>
---
 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)" <objects >types &&
+	! grep blob types
 '
 
 test_done
-- 
2.30.0

Re: [PATCH v2] t5544: clarify 'hook works with partial clone' test

[Junio C Hamano]

Jacob Vosmaer <jacob@gitlab.com> 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 <jacob@gitlab.com>
> ---
>  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)" <objects >types &&
> +	! grep blob types
>  '
>  
>  test_done

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Æ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.

Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

[Jeff King]

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

Re: [PATCH v3 0/1] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jacob Vosmaer <jacob@gitlab.com> 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.

Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH v2] upload-pack.c: fix filter spec quoting bug

[Jeff King]

On Mon, Jan 25, 2021 at 06:09:21PM +0100, Jacob Vosmaer wrote:

> +test_expect_success 'hook works with partial clone' '
> +	clear_hook_results &&
> +	test_config_global uploadpack.packObjectsHook ./hook &&
> +	test_config_global uploadpack.allowFilter true &&

I was going to complain that:

 test_config -C dst.git uploadpack.packObjectsHook ./hook &&
 test_config -C dst.git uploadpack.allowFilter true &&

would be more clear, since it tells us which repo we care about
impacting. But the rest of the script doesn't do that, and indeed it
can't, because we don't allow packObjectsHook to be set in per-repo
config for security reasons. :)

(We could do it per-repo for allowFilter, but I think it is just as well
to keep the two calls consistent).

> +	git clone --bare --no-local --filter=blob:none . dst.git &&
> +	git -C dst.git rev-list --objects --missing=print HEAD >objects &&
> +	grep "^?" objects

Previously that clone would fail, so the bug-fix is demonstrated by it
succeeding. But the other two commands are added to make sure that we
actually did apply the filter correctly. Even better. Thanks for being
thorough.

-Peff

Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

Thanks, I changed the reproducing command to use 'env' and I added a
test case in t5544.


Best regards,

Jacob Vosmaer
GitLab, Inc.

On Fri, Jan 22, 2021 at 9:32 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Jan 22, 2021 at 03:21:37PM +0100, Jacob Vosmaer wrote:
>
> > 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=" exec" '\
> > '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. Note that there is an intentional space before
> > 'exec'. Without that space, run-command.c tries to be smart and the
> > command fails for the wrong reason.
>
> The "-u" command is run with a shell, so:
>
>   git clone \
>     -u 'git -c uploadpack.allowfilter \
>             -c uploadpack.packobjectshook=env \
>             upload-pack' \
>   --filter=blob:none --no-local src.git dst.git
>
> may be a more readable version. I also found the use of " exec" clever,
> but rather subtle; you need the extra space so that our "don't bother
> using a shell" run-command optimization does not kick in. I replaced it
> with "env" here, which is a slightly more canonical way of running a
> sub-program that does not rely on shell builtins.
>
> But all of this should be added as a new test, probably in t5544 with
> the other pack-objects hook tests.
>
> > 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 of 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.
>
> Yeah, that is exactly right. "use_shell" just means that the command is
> (possibly) run with a shell. Quoting for any extra arguments is handled
> automatically.
>
> I think you're correct that this was broken from the start in 10ac85c785
> (upload-pack: add object filtering for partial clone, 2017-12-08).
> That's even before the use_shell was added, and then later it was pushed
> into that conditional by 0b6069fe0a (fetch-pack: test support excluding
> large blobs, 2017-12-08). Presumably because the non-hook path would not
> have worked at all, and that was the first time any of it was actually
> tested. ;)
>
> (I've cc'd authors of those commits as an FYI; I think both were
> relatively new to the project at the time so misunderstanding this
> subtlety of run-command is not too surprising).
>
> I'm somewhat embarrassed to say that despite being the one who added the
> pack-objects hook 4 years ago, we still have not switched over to it at
> GitHub from our custom patch (the reason is just mundane; there's some
> other adjustments that would have to happen and nobody has ever quite
> gotten around to it). Presumably you are looking to use it at GitLab.
> Just beware that you are probably treading new-ish ground, so there may
> be other bugs like this lurking.
>
> > 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);
>
> Yep, this looks like the right fix. I think with an addition to the test
> suite, this will be good to go.
>
> -Peff

Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug

[Jacob Vosmaer]

On Fri, Jan 22, 2021 at 9:32 PM Jeff King <peff@peff.net> wrote:
> I also found the use of " exec" clever,
> but rather subtle; you need the extra space so that our "don't bother
> using a shell" run-command optimization does not kick in. I replaced it
> with "env" here, which is a slightly more canonical way of running a
> sub-program that does not rely on shell builtins.
Yes good idea, the exec thing is too clever for its own good.


> But all of this should be added as a new test, probably in t5544 with
> the other pack-objects hook tests.

Did that, hope it is what you had in mind.


> I'm somewhat embarrassed to say that despite being the one who added the
> pack-objects hook 4 years ago, we still have not switched over to it at
> GitHub from our custom patch (the reason is just mundane; there's some
> other adjustments that would have to happen and nobody has ever quite
> gotten around to it). Presumably you are looking to use it at GitLab.
> Just beware that you are probably treading new-ish ground, so there may
> be other bugs like this lurking.

Thanks for the heads up!

Jacob