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

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Jacob Vosmaer <jacob@gitlab.com>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 1/1] upload-pack.c: fix filter spec quoting bug
Date: Fri, 22 Jan 2021 14:10:25 -0800	[thread overview]
Message-ID: <xmqq4kj87hda.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <YAs2RMT1rEH/2LSp@coredump.intra.peff.net> (Jeff King's message of "Fri, 22 Jan 2021 15:32:04 -0500")

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.