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
