On 3/20/2020 4:26 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
