From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 04 Sep 2026 05:21:25 GMT
Subject: Re: [PATCH 2/2] builtin/maintenance: improve heuristic for "rerere gc"
Message-ID: <appVVYr6oW0fyMRD@pks.im>
In-Reply-To: <2ca2b4db-1fd9-46e8-9385-260a12af43bb@gmail.com>

On Thu, Sep 03, 2026 at 10:19:33AM -0400, Derrick Stolee wrote:
> On 9/3/2026 5:04 AM, Patrick Steinhardt wrote:
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index de2f9e7fed..9147418a61 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -396,31 +396,13 @@ static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED,
> >  
> >  static int rerere_gc_condition(struct gc_config *cfg UNUSED)
> >  {
> > -	struct strbuf path = STRBUF_INIT;
> > -	int should_gc = 0, limit = 1;
> > -	DIR *dir = NULL;
> > +	int limit = 512;
> >  
> >  	repo_config_get_int(the_repository, "maintenance.rerere-gc.auto", &limit);
> > +	if (limit <= 0)
> > +		return limit < 0;
> 
> This is cute, but works. It's logically equivalent to
> 
> 	if (!limit)
> 		return 0;
> 	if (limit < 0)
> 		return 1;
> 
> which would map more directly to the two documented cases. It takes
> the slightest amount of mental processing to connect the docs to
> the format you have.

This also existed in the preimage, but I agree it's harder to read than
really necessary. Will improve while at it.

> > +	return rerere_gc_estimate(the_repository, limit) >= (size_t)limit;
> >  }
> 
> I do like that this method is simpler in the builtin code in favor
> of a method that has access to rerere internals.
> 
> I do wonder if rerere_gc_estimate() should be
> rerere_stale_above_limit() instead, as we are not using any callers
> that care about the resulting number other than "is it at least limit?"

Agreed, the current name isn't great. I'm somehow hestitant to use
`rerere_stale_above_limit()` too, though. I'll adapt it to
`rerere_gc_needed()` instead.

Thanks for your review!

Patrick