From: Alan Braithwaite Date: Thu, 05 Mar 2026 23:11:41 GMT Subject: Re: [PATCH v2] clone: add clone..defaultObjectFilter config Message-ID: In-Reply-To: Junio C Hamano wrote: > This is unlike how http.. 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. Funny enough, I actually prefer that but gathered from the previous commentary that it wasn't desired. I'd be more than content to add it. Junio C Hamano wrote: > 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): You're right, it's been a while I've written C. Thanks for catching that. I think my mind was going somewhere else with it, but YAGNI. Junio C Hamano wrote: > However, I think 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? Good catch. I'll fix it. Will set cascade_fn to NULL so the second pass only looks at clone..defaultObjectFilter entries. Thanks for the review and for your patience as I shake the gopher out of me and figure out how to do real programming again. Thanks, - Alan On Thu, Mar 5, 2026, at 11:01, Junio C Hamano wrote: > "Alan Braithwaite via GitGitGadget" writes: > >> From: Alan Braithwaite >> >> 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..* — 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..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..* settings. > >> Only the URL-qualified form (clone..defaultObjectFilter) is >> honored; a bare clone.defaultObjectFilter without a URL subsection >> is ignored. > > This is unlike how http.. 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 > >> 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 ""] > 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..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.