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

Jacob Vosmaer [off-list ref] 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.

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

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