From: Derrick Stolee <stolee@gmail.com>
Date: Thu, 03 Sep 2026 14:11:20 GMT
Subject: Re: [PATCH 1/2] rerere: extract logic to determine whether entries are stale
Message-ID: <a63c3bbe-28b8-4026-9c07-11c2d445c504@gmail.com>
In-Reply-To: <20260903-b4-pks-maintenance-rerere-gc-heuristic-v1-1-9929c45a9788@pks.im>

On 9/3/2026 5:04 AM, Patrick Steinhardt wrote:
> When garbage collecting rerere entries we need to figure out whether any
> given entry is stale before pruning it. In a subsequent commit we're
> about to introduce a second caller that wants to determine staleness,
> but the logic is not currently reusable.
> 
> Extract the logic to compute staleness by introducing two new helper
> functions `rerere_gc_cutoffs()` and `rerere_id_is_stale()`.

Thanks for doing these extractions. It reduces complexity in the top-
level logic.

> -static void prune_one(struct rerere_id *id,
> -		      timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
...> +static bool rerere_id_is_stale(struct rerere_id *id,
> +			       timestamp_t cutoff_resolve,
> +			       timestamp_t cutoff_noresolve)

This modification of prune_one() to a staleness check is good to
have split, but...

>  		for (id.variant = 0, id.collection = rr_dir;
>  		     id.variant < id.collection->status_nr;
>  		     id.variant++) {
> -			prune_one(&id, cutoff_resolve, cutoff_noresolve);
> +			if (rerere_id_is_stale(&id, cutoff_resolve, cutoff_noresolve))
> +				unlink_rr_item(&id);
>  			if (id.collection->status[id.variant])
>  				now_empty = 0;
>  		}

...this loop gets slightly more complicated. This is not worth
a change, but I'm thinking out loud that I would have updated
prune_one to be this simple:

static void prune_one(struct rerere_id *id,
		      timestamp_t cutoff_resolve, timestamp_t cutoff_noresolve)
{
	if (rerere_id_is_stale(&id, cutoff_resolve, cutoff_noresolve))
		unlink_rr_item(&id);
} 
and left the loop alone. This is only a preference, as your
implementation is also quite clean.

I did look to patch 2 to see if this choice of splitting the
prune_one() method had an impact there, and it doesn't appear
to matter.

The rerere_gc_cutoffs() and rerere_id_is_stale() methods are
needed in patch 2, so this adjustment to prune_one() is
important.

Thanks,
-Stolee