From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 10 Aug 2026 05:15:46 GMT
Subject: Re: [PATCH v2 3/7] builtin/receive-pack: read unpack limit config lazily
Message-ID: <anlegs6zfUysbx0C@pks.im>
In-Reply-To: <20260809190106.1565882-4-jltobler@gmail.com>

On Sun, Aug 09, 2026 at 02:01:02PM -0500, Justin Tobler wrote:
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 78d2911c00..5264d70467 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2333,6 +2320,30 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
>  		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
>  }
>  
> +static int get_unpack_limit(struct repository *repo)

Shouldn't the function return `unsigned int`? We always expect it to be
a positiv value, and in the final commit we have to add a cast because
of that.

> +{
> +	static int limit = -1;

Is it really necessary to have this be a static variable? As far as I
can see we'd only call `unpack()` once. Also, the cache would become
stale if we ever tried to read the limit for multiple different repos.

> +	if (limit < 0) {
> +		int receive_limit = -1;
> +		int transfer_limit = -1;
> +
> +		repo_config_get_int(repo, "receive.unpacklimit",
> +				    &receive_limit);
> +		repo_config_get_int(repo, "transfer.unpacklimit",
> +				    &transfer_limit);
> +
> +		if (receive_limit >= 0)
> +			limit = receive_limit;
> +		else if (transfer_limit >= 0)
> +			limit = transfer_limit;
> +		else
> +			limit = 100;
> +	}
> +
> +	return limit;
> +}

So how about something like this instead?

	static unsigned int get_unpack_limit(struct repository *repo)
	{
		unsigned int limit = 100;
		if (!repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit) ||
		    !repo_config_get_uint(repo, "receive.unpacklimit", &receive_limit))
			/* do nothing */;
		return limit;
	}

Patrick