Re: [PATCH 2/2] builtin/maintenance: improve heuristic for "rerere gc"
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Sep 3, 2026, 14:19 UTC
- Message-ID
- <2ca2b4db-1fd9-46e8-9385-260a12af43bb@gmail.com>
- In-Reply-To
- <20260903-b4-pks-maintenance-rerere-gc-heuristic-v1-2-9929c45a9788@pks.im>
On 9/3/2026 5:04 AM, Patrick Steinhardt wrote:
Show 18 quoted lines
> The "rerere-gc" maintenance task is responsible for pruning rerere > entries older than a certain configurable cutoff point. Whether or not > the task gets run during auto-maintenance can be configured via > "maintenance.rerere-gc.auto": > > - A negative value indicates that maintenance should always run. > > - A zero value indicates that maintenance should never run. > > - Otherwise, a positive value indicates that maintenance should always > run in case we have at least a single rerere entry. > > While the first two conditions are sensible, the last one is less so as > it does not account for whether we would even prune old entries in the > first place. Instead, it effectively implies that we unconditionally > spawn "git rerere gc" when rerere is enabled. Chances are high though > that there is nothing to prune, as the default cutoff dates are 60 days > for resolved rerere entries and 15 days for unresolved ones.
I agree on these points.
Show 12 quoted lines
> @@ -121,10 +121,10 @@ maintenance.rerere-gc.auto:: > This integer config option controls how often the `rerere-gc` task > should be run as part of `git maintenance run --auto`. If zero, then > the `rerere-gc` task will not run with the `--auto` option. A negative > - value will force the task to run every time. Otherwise, any positive > - value implies the command will run when the "rr-cache" directory exists > - and has at least one entry, regardless of whether it is stale or not. > - This heuristic may be refined in the future. The default value is 1. > + value will force the task to run every time. Otherwise, a positive > + value implies the command should run when the estimated number of stale > + entries that would be pruned is greater than or equal to the configured > + value. The default value is 512.
Thanks for updating the docs so clearly.
Show 18 quoted lines
> maintenance.worktree-prune.auto::
> This integer config option controls how often the `worktree-prune` task
> 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.
> + 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?"
Show 35 quoted lines
> +size_t rerere_gc_estimate(struct repository *r, size_t limit)
> +{
> + timestamp_t cutoff_resolve, cutoff_noresolve;
> + struct strbuf buf = STRBUF_INIT;
> + struct dirent *e;
> + size_t count = 0;
> + DIR *dir;
> +
> + dir = opendir(repo_git_path_replace(r, &buf, "rr-cache"));
> + if (!dir)
> + goto out;
> +
> + rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve);
> +
> + while ((e = readdir_skip_dot_and_dotdot(dir))) {
> + struct rerere_id id;
> +
> + /*
> + * We estimate the number of stale entries by only considering
> + * those starting with "17". This is the same strategy that we
> + * use for estimating the number of loose objects.
> + */
> + if (!starts_with(e->d_name, "17") ||
> + !is_rr_cache_dirname(e->d_name))
> + continue;
> +
> + id.collection = find_rerere_dir(e->d_name);
> + for (id.variant = 0;
> + id.variant < id.collection->status_nr;
> + id.variant++) {
> + if (rerere_id_is_stale(&id, cutoff_resolve,
> + cutoff_noresolve)) {
> + count += 256;
> + if (count >= limit)
> + goto out;This short-circuit is valuable and helps me understand the method prototype including a limit. If the method is changed to be a boolean result, then this would be 'result 1; goto out;'
Show 12 quoted lines
> + } > + } > + } > + > +out: > + if (dir) > + closedir(dir); > + free_rerere_dirs(); > + strbuf_release(&buf); > + return count; > +} > +
Again, all I can find are taste preferences. This is a good implementation and leaves some flexibility for future callers to care about the number of stale entries.
Thank you also, for covering your change with tests.
Both patches LGTM.
Thanks, -Stolee