From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 16 Mar 2026 07:47:35 GMT
Subject: Re: [PATCH v6] clone: add clone.<url>.defaultObjectFilter config
Message-ID: <abe1l8ONmFIhzaxi@pks.im>
In-Reply-To: <pull.2058.v6.git.1773553022381.gitgitgadget@gmail.com>

On Sun, Mar 15, 2026 at 05:37:02AM +0000, Alan Braithwaite via GitGitGadget wrote:
>  1:  fa1ea69bdb ! 1:  480453b2e7 clone: add clone.<url>.defaultObjectFilter config
>      @@ t/t5616-partial-clone.sh: test_expect_success 'after fetching descendants of non
>       +test_expect_success 'setup for clone.defaultObjectFilter tests' '
>       +	git init default-filter-src &&
>       +	echo "small" >default-filter-src/small.txt &&
>      -+	dd if=/dev/zero of=default-filter-src/large.bin bs=1024 count=100 2>/dev/null &&
>       +	git -C default-filter-src add . &&
>       +	git -C default-filter-src commit -m "initial" &&
>       +

As Junio already pointed out, this change here is a bit puzzling. Not
that I think it's a problem, but one wonders why this existed in the
first place if it seemed to not be necessary.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 45d8fa0eed..18316a7da9 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -757,6 +758,51 @@ static int git_clone_config(const char *k, const char *v,
>  	return git_default_config(k, v, ctx, cb);
>  }
>  
> +static int clone_filter_collect(const char *var, const char *value,
> +				const struct config_context *ctx UNUSED,
> +				void *cb)
> +{
> +	char **filter_spec_p = cb;
> +
> +	if (!strcmp(var, "clone.defaultobjectfilter")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		free(*filter_spec_p);
> +		*filter_spec_p = xstrdup(value);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Look up clone.defaultObjectFilter or clone.<url>.defaultObjectFilter
> + * using the urlmatch infrastructure.  A URL-qualified entry that matches
> + * the clone URL takes precedence over the bare form, following the same
> + * rules as http.<url>.* configuration variables.
> + */
> +static char *get_default_object_filter(const char *url)
> +{
> +	struct urlmatch_config config = URLMATCH_CONFIG_INIT;
> +	char *filter_spec = NULL;
> +	char *normalized_url;
> +
> +	config.section = "clone";
> +	config.key = "defaultobjectfilter";
> +	config.collect_fn = clone_filter_collect;
> +	config.cb = &filter_spec;
> +
> +	normalized_url = url_normalize(url, &config.url);
> +	if (!normalized_url) {
> +		urlmatch_config_release(&config);
> +		return NULL;
> +	}

We haven't allocated anything, right? So in theory, we should be able to
return early without calling `urlmatch_config_release()`. This could be
stressed further by moving the error path earlier, so that it's the
first thing we do in the function.

Patrick