Re: [PATCH v2 3/7] builtin/receive-pack: read unpack limit config lazily

On 26/08/10 07:15AM, Patrick Steinhardt wrote:
> 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.

Will update.

> > +{
> > +	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.

Good point and yes in practice we really will only be reading this once
so there isn't really point to caching it. Will update.

> > +	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;
> 	}

Good suggestion. Because `repo_config_get_uint()` leaves the value
untouched if no config is set, we could probably simplify even further
to something like this:

  static unsigned int get_unpack_limit(struct repository *repo)
  {
  	unsigned int limit = 100;
  
  	repo_config_get_uint(repo, "transfer.unpacklimit", &limit);
  	repo_config_get_uint(repo, "receive.unpacklimit", &limit);
  
  	return limit;
  }

Thanks,
-Justin