From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 04 Sep 2026 05:21:15 GMT
Subject: Re: [PATCH 1/2] rerere: extract logic to determine whether entries are stale
Message-ID: <appVS2P6ThaRU9fc@pks.im>
In-Reply-To: <a63c3bbe-28b8-4026-9c07-11c2d445c504@gmail.com>

On Thu, Sep 03, 2026 at 10:11:20AM -0400, Derrick Stolee wrote:
> 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.

That's fair. I originally retained `prune_one()`, but then I wasn't sure
whether it's really worth it anymore given that it's essentially a
two-line function now. Anyway, will restore it.

Patrick