From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 03 Sep 2026 14:19:33 GMT
Subject: Re: [PATCH 2/2] builtin/maintenance: improve heuristic for "rerere gc"
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:
> 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. 
> @@ -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.
>  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?"

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

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