Re: [PATCH 0/8] Making reflog modifications part of the transactions API

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Dec 14, 2014, 23:17 UTC
Message-ID: <548E1A71.7080707@alum.mit.edu>
In-Reply-To: <CAN05THRiMiQCqO1VZX7zQeWLBZMPG_sx1aVvdGirAoEh2Hd9Dw@mail.gmail.com>

On 12/12/2014 10:16 PM, ronnie sahlberg wrote:

Show 21 quoted lines

> On Fri, Dec 12, 2014 at 11:17 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> What am I missing?
> 
> My original idea was to clean up a bit of the reflog handling API and
> have one single transaction API for all  ref and reflog operations.
> 
> Think future use cases where you have a database backend for both refs
> and reflogs. It would be very nice to have a single atomic transaction
> that would either commit or fail atomically any update to refs and/or
> reflogs.
> Otherwise you would have all consumers of the API have to invent their
> own transaction and rewind support : 'oh the ref transaction failed
> and I have already done the reflog commit,   have to manually uncommit
> ...".
> And this quickly becomes quite burdensome for consumers.
> 
> I think a transaction API should remove this burden from consumers and
> make it as easy as possible to use the API.
> 
> Conditional of if it is desireable to have transactions for reflogs at all.

Nobody is against ACID. But the API to trigger an ACID update doesn't always have to look like

    ref_transaction_begin()
    ref_transaction_update_XXX()
    /* ... */
    ref_transaction_commit()

The reflog_expire() function that I wrote does everything within an internal transaction that the caller doesn't have to know about. Similarly, the reflog update that happens when a reference is updated also occurs within a transaction, even without the caller having to ask for the reflog update explicitly.

Show 16 quoted lines

> About the cleanup part. The current API, and I think also the current
> direction of both my old patches (which I think did not go far enough
> in the transactifications) or this current patchseries is that they
> all
> have a very confusing and inconsistent API for reflog updates.
> With this I mean,   sometimes reflog updates happen within a
> transaction as a side effect of a ref_transaction_update().
> Other times reflog updates happens ooutside of transactions by calling
> a special reflog API.
> 
> I.e.  reflogs sometimes update as part of a transaction and sometimes not.
> A follow up question then on this API is what should happen if you
> have a transaction open, but not committed, and while the transaction
> is open you call the non-transactional reflog API for a reflog for the
> same ref that is already beeing/or going to be/ updated as the
> ref-update-side-effect ?

The same thing happens as when two independent processes try to update the same reference simultaneously: one fails. But there is currently no need for any command that would do such a thing.

Show 14 quoted lines

> I think an api where "sometimes you operate on foo from within a
> transaction and sometimes you do not, and if you do the latter when a
> transaction is open, it is unclear what should happen" is not great.
> IMHO, refs and reflog updates are related and I think:
> 
> * a transaction should be the ONLY way to mutate either a ref or a
> reflog or both.
> * if you update both a ref and a reflog then that should happen as
> part of one single transaction.
> * (later) it would probably make the API better if the code was
> refactored so write_ref_sha1() will NOT call log_ref_write() anymore
> and instead make the reflog update that happens explicit perhaps by
> calling something like a ref_transaction_update_reflog() as part of
> the ref_transaction_update() call.

I disagree. The reflog should be updated *whenever* a reference is updated (for references that have reflogs enabled). So why should callers have to remember to trigger the reflog update as an extra step? It's better that the reflog update is an intrinsic part of updating the reference; that way nobody can forget it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu