From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 06 Mar 2025 23:17:37 GMT
Subject: Re: [PATCH 3/6] builtin/reflog: stop storing per-reflog expiry dates globally
Message-ID: <35jnncjitwoy7bb7w5zyk6nhjvab4tmdjhtuqltljypfigk2g7@swpulgkzooxl>
In-Reply-To: <Z8l69bRy8PY3Ep9J@pks.im>

On 25/03/06 11:37AM, Patrick Steinhardt wrote:
> > 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.

That's fair. This interface certainly feels awkward and exposing it
further doesn't seem ideal. At the same time, maybe its not too big of a
deal that it should be dealt with now.

-Justin