Re: [PATCH 3/6] builtin/reflog: stop storing per-reflog expiry dates globally

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 6, 2025, 10:37 UTC
Message-ID: <Z8l69bRy8PY3Ep9J@pks.im>
In-Reply-To: <usf4pzrp322xvfrvsasgy22rnw7d2a6krzuctwpp23ppdgh6ji@ram73wv5dn54>

On Tue, Mar 04, 2025 at 05:41:08PM -0600, Justin Tobler wrote:

Show 35 quoted lines

> On 25/02/26 04:24PM, Patrick Steinhardt wrote:
> > diff --git a/reflog.h b/reflog.h
> > index a9d464bbf8c..b08780a30a7 100644
> > --- a/reflog.h
> > +++ b/reflog.h
> > @@ -2,7 +2,15 @@
> >  #define REFLOG_H
> >  #include "refs.h"
> >  
> > +struct reflog_expire_entry_option {
> > +	struct reflog_expire_entry_option *next;
> > +	timestamp_t expire_total;
> > +	timestamp_t expire_unreachable;
> > +	char pattern[FLEX_ARRAY];
> > +};
> > +
> >  struct reflog_expire_options {
> > +	struct reflog_expire_entry_option *entries, **entries_tail;
> 
> Now we can also store the configured per-reflog-pattern expiry entries
> in the options type instead of relying on global state.
> 
> >  	int stalefix;
> >  	int explicit_expiry;
> >  	timestamp_t default_expire_total;
> 
> Now that all the reflog expiry configuration is contained within
> reflog_expire_options, I wonder if it really makes sense to also keep
> the expire_total and expire_unreachable fields.
> 
> From my understanding these fields are not really for configuration, but
> hold the reflog expiry configuration for the current active reference
> while iterating. This gets set by set_reflog_expiry_param() prior to
> calling refs_reflog_expire(). It seems like this could be figured out
> during refs_reflog_expire() now.

Yes, these fields hold state indeed, namely the value for a given refname. These fields thus need to be updated for every refname for which you want to check expiration, which is done by calling `reflog_expire_options_set_refname()`. This interfaces is extremely awkward from my perspective, and it would be very much preferable to instead have an interface that, given the options and a refname as parameters, tells you whether the reflog contains entries that should be pruned.

In fact, I did have a look at fixing this awkward interface, but it always ended up with way more changes than I felt comfortable with. So I decided to only go a couple steps into the direction of better encapsulation, but to not fix all of the design issues with the current interface.

Patrick