Re: [PATCH 1/2] partial-clone: set default filter with --partial

[Derrick Stolee <stolee@gmail.com>]

On 3/20/2020 4:26 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
>> index 256bcfbdfe6..a71716ef75e 100644
>> --- a/list-objects-filter-options.c
>> +++ b/list-objects-filter-options.c
>> @@ -270,6 +270,24 @@ int opt_parse_list_objects_filter(const struct option *opt,
>>  	return 0;
>>  }
>>  
>> +int opt_set_blob_none_filter(const struct option *opt,
>> +			     const char *arg, int unset)
> 
> Isn't the convention to use "opt_parse_" for canned parse-options
> callbacks?

I can do that.

>> +{
>> +	struct strbuf filter_arg = STRBUF_INIT;
>> +	struct list_objects_filter_options *filter_options = opt->value;
>> +	
>> +	if (unset || !arg || !strcmp(arg, "0")) {
>> +		parse_list_objects_filter(filter_options, "blob:none");
>> +		return 0;
> 
> If "--no-partial" were allowed, it should be usable to countermand
> "--partial" earlier on the command line or perhaps configured
> default.  But the above (especially the "unset ||" part) makes
> "--no-partial" a synonym to "--filter=blob:none", no?

I should have been more careful about the use of "unset" (which
would never be true with the current option parsing).

>> +	}
>> +	
>> +	strbuf_addf(&filter_arg, "blob:limit=%s", arg);
>> +	parse_list_objects_filter(filter_options, filter_arg.buf);
>> +	strbuf_release(&filter_arg);
> 
> I would have expected the body of the function to read more like
> this:
> 
> 	if (unset) {
>         	... clear filter_options stored in opt->value ...
> 	} else {
> 		struct strbuf filter_arg = STRBUF_INIT;
> 		if (!arg)
> 			strbuf_addstr(&filter_arg, "blob:none");
> 		else
> 			strbuf_addf(&filter_arg, "blob:limit=%s", arg);
> 		parse_list_objects_filter(filter_options, filter_arg.buf);
> 		strbuf_release(&filter_arg);
> 	}

This is a better organization and I will use it.

> Specifically, I find it unsatisifying to see the "0" optimization at
> this level.  Shouldn't it be done in parse_list_objects_filter() to
> parse "blob:limit=<num>" and after realizing <num> is zero, pretend
> as if it got "blob:none" to optimize?  That way, people can even say
> "--partial=0k" and get it interpreted as "--filter=blob:none", right?

I suppose it would be worth checking the recent server-side improvements
to see if they translate a limit=0k to a "size 0" and then ignore the
size check and simply remove all blobs.

>>  #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
>>  	{ OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \
>>  	  N_("object filtering"), 0, \
>> -	  opt_parse_list_objects_filter }
>> +	  opt_parse_list_objects_filter }, \
>> +	{ OPTION_CALLBACK, 0, CL_ARG__PARTIAL, fo, N_("size"), \
>> +	  N_("partial clone with blob filter"), \
>> +	  PARSE_OPT_OPTARG | PARSE_OPT_NONEG , opt_set_blob_none_filter }
> 
> PARSE_OPT_NONEG means "--no-partial" is forbidden and the callback
> won't see unset==1 at all, right?

You're right. I'm being inconsistent. Your reasons above point to a
good reason to have --no-partial available.

Thanks,
-Stolee