Re: [PATCH v2] clone: add clone.<url>.defaultObjectFilter config

> This is unlike how http.<url>.<var> configuration variables work,
> and while I can see that server operators may not want to see users
> set clone.defaultObjectFilter and affect traffic with _all_ sites, I
> am afraid that this design choice may appear a bit counter-intuitive
> to end users.

> "Alan Braithwaite via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Alan Braithwaite <alan@braithwaite.dev>
>>
>> Add a new configuration option that lets users specify a default
>> partial clone filter per URL pattern.  When cloning a repository
>> whose URL matches a configured pattern, git-clone automatically
>> applies the filter, equivalent to passing --filter on the command
>> line.
>>
>>     [clone "https://github.com/"]
>>         defaultObjectFilter = blob:limit=5m
>>
>>     [clone "https://internal.corp.com/large-project/"]
>>         defaultObjectFilter = blob:none
>>
>> URL matching uses the existing urlmatch_config_entry() infrastructure,
>> following the same rules as http.<url>.* — you can match a domain,
>> a namespace path, or a specific project, and the most specific match
>> wins.
>>
>> The config only affects the initial clone.  Once the clone completes,
>> the filter is recorded in remote.<name>.partialCloneFilter, so
>> subsequent fetches inherit it automatically.  An explicit --filter
>> flag on the command line takes precedence.
>
> The motivation behind the change is clearly described.  Reusing the
> existing urlmatch_config_entry() infrastructure is very appropriate
> as it makes the feature intuitive for those familiar with
> http.<url>.* settings.
>
>> Only the URL-qualified form (clone.<url>.defaultObjectFilter) is
>> honored; a bare clone.defaultObjectFilter without a URL subsection
>> is ignored.
>
> This is unlike how http.<url>.<var> configuration variables work,
> and while I can see that server operators may not want to see users
> set clone.defaultObjectFilter and affect traffic with _all_ sites, I
> am afraid that this design choice may appear a bit counter-intuitive
> to end users.
>
>
>> Signed-off-by: Alan Braithwaite <alan@braithwaite.dev>
>
>>  Documentation/config/clone.adoc | 26 ++++++++++++
>>  builtin/clone.c                 | 68 ++++++++++++++++++++++++++++++
>>  t/t5616-partial-clone.sh        | 73 +++++++++++++++++++++++++++++++++
>>  3 files changed, 167 insertions(+)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 45d8fa0eed..5e20b5343d 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -44,6 +44,7 @@
>>  #include "path.h"
>>  #include "pkt-line.h"
>>  #include "list-objects-filter-options.h"
>> +#include "urlmatch.h"
>>  #include "hook.h"
>>  #include "bundle.h"
>>  #include "bundle-uri.h"
>> @@ -757,6 +758,65 @@ static int git_clone_config(const char *k, const char *v,
>>  	return git_default_config(k, v, ctx, cb);
>>  }
>>  
>> +struct clone_filter_data {
>> +	char *default_object_filter;
>> +};
>> +
>> +static int clone_filter_collect(const char *var, const char *value,
>> +				const struct config_context *ctx UNUSED,
>> +				void *cb)
>> +{
>> +	struct clone_filter_data *data = cb;
>> +
>> +	if (!strcmp(var, "clone.defaultobjectfilter")) {
>> +		free(data->default_object_filter);
>> +		data->default_object_filter = xstrdup(value);
>> +	}
>> +	return 0;
>> +}
>
> This will segfault with a "value-less truth", i.e.,
>
> 	[clone "<URL>"]
> 		defaultObjectFilter
>
> so there should be 
>
> 		if (!value)
> 			return config_error_nonbool(var);
>
> in it.
>
> I cannot convince myself that a new structure only to hold a single
> "char *" member is not over-engineering.  Wouldn't it work equally
> well (unless you have an immediate plan to add more members to the
> struct, that is):
>
> 	char **filter_spec_p = cb;
>
> 	if (!strcmp(var, "clone.defaultobjectfilter")) {
> 		if (!value)
> 			retgurn config_error_nonbool(var);
> 		free(*filter_spec_p);
> 		*filter_spec_p = xstrdup(value);
> 	}
> 	return 0;
>
>> +/*
>> + * Look up clone.<url>.defaultObjectFilter using the urlmatch
>> + * infrastructure.  Only URL-qualified forms are supported; a bare
>> + * clone.defaultObjectFilter (without a URL) is ignored.
>> + */
>> +static char *get_default_object_filter(const char *url)
>> +{
>> +	struct urlmatch_config config = URLMATCH_CONFIG_INIT;
>> +	struct clone_filter_data data = { 0 };
>> +	struct string_list_item *item;
>> +	char *normalized_url;
>> +
>> +	config.section = "clone";
>> +	config.key = "defaultobjectfilter";
>> +	config.collect_fn = clone_filter_collect;
>> +	config.cascade_fn = git_clone_config;
>> +	config.cb = &data;
>> +
>> +	normalized_url = url_normalize(url, &config.url);
>> +
>> +	repo_config(the_repository, urlmatch_config_entry, &config);
>> +	free(normalized_url);
>
> This forces a second full scan of the configuration space.  But it
> cannot be avoided, because the existing repo_config() call has to
> happen early before we call parse_options() to give us the
> configured default to overwrite with the command line, and we would
> not know what our URL is before we called parse_options().
>
> However, I thihk you want to leave the .cascade_fn NULL; you do not
> want urlmatch_config_entry() to call git_clone_config() AGAIN on the
> configuration variables, as the first call to repo_config() before
> we call parse_options() should have already handled them, no?
>
> Thanks.