Re: [PATCH 2/2] builtin/maintenance: improve heuristic for "rerere gc"
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 4, 2026, 05:21 UTC
- 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:
Show 28 quoted lines
> 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.
Show 9 quoted lines
> > + 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