Re: [PATCH v2] upload-pack: fix filter options scope

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Fri, May 08, 2020 at 10:01:15AM +0200, Christian Couder wrote:
>
>> The changes since the previous RFC version are the following:
>> 
>>   - now filter_options is part of struct upload_pack_data as
>>     suggested by Peff and Taylor
>>   - improved commit message
>>   - updated comment before the test that used to fail
>
> Thanks, this version looks good to me.
>
>>  static void create_pack_file(const struct object_array *have_obj,
>> -			     const struct object_array *want_obj)
>> +			     const struct object_array *want_obj,
>> +			     struct list_objects_filter_options *filter_options)
>
> I had hoped that stuffing it into upload_pack_data would require fewer
> changes passing it around, but I guess many of these functions don't
> know about upload_pack_data in the first place. Oh well. I think this is
> the best we can do for now. In the long run we'd perhaps want to take
> upload_pack_data there, but it wouldn't help until the v0 path also uses
> that struct.

Yup, I agree that this v2-only fix is a good first step.

Even though the log message itself got a lot better explaining the
nature of the issue, I do not think the title of the patch does a
good job explaining what it is about to the readers of shortlog.

"fix" is a meaningless word in a bugfix patch, and it does not make
it clear what bad effect of the original code had by not giving a
clean-slate "options" variable to the second invocation of the
callchain.

Is it that the server side was incapable of serving a follow-up
fetch request in the same process when protocol v2 was in use?
Perhaps

    upload-pack: allow follow-up fetch in protocol v2

or something?

I care about singling out protocol v2 because then we would
immediately know that backporting this to older maintenance track is
of lower priority as the plan is to flip the default back to v0.

Thanks.