Subject: Re: [PATCHv3 00/13] the refs-transactions-reflog series

From: Jonathan Nieder <[email-masked]>
Date: Thu, 4 Dec 2014 10:14:04 -0800

Michael Haggerty wrote:

> I am still unhappy with the approach of this series, for the reasons
> that I explained earlier [1]. In short, I think that the abstraction
> level is wrong. In my opinion, consumers of the refs API should barely
> even have to *know* about reflogs, let alone implement reflog expiration
> themselves.

Okay, now returning to the substance of this comment.  This is
revisiting themes from [3], so my opinions are probably not a
surprise.

I think that the API changes that you and Stefan are proposing are
consistent and could both go in.

You suggested refactoring expire_reflogs() to use a callback that
decides what to expire.  Then it doesn't have to care that the
expiration happens by creating a new reflog and copying over the
reflog entries that are not being expired.  The result is a clean
reflog expiration API.

The ref-transaction-reflog series allows those low-level steps to be
part of a ref transaction.  Any ref backend (the current files-based
backend or a future other one) would get a chance to reimplement those
low-level steps, which are part of what happens during ref updates and
reflog deletion.  The goal is for all reflog updates to use the
transaction API, so that new ref/reflog backends only need to
implement the transaction functions.

So *both* are making good changes, with different goals.

The implementation of the reflog expiration API can use the ref
transaction API.

> Of course, reflog expiration *should* be done atomically. But that is
> the business of the refs module; callers shouldn't have to do all the
> complicated work of building the transaction themselves.

I don't understand this comment.  After the ref-transaction-reflog
series, a transaction_update_ref() call still takes care of the
corresponding reflog update without the caller having to worry about
it.

Thanks for looking it over,
Jonathan

> [1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[3] http://thread.gmane.org/gmane.comp.version-control.git/259939/focus=259967