On Fri, Dec 12, 2014 at 11:17 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 12/06/2014 03:46 AM, Stefan Beller wrote: >> This goes on top of Michaels series. The idea of this series is make the >> reflogs being part of the transaction API, so it will be part of the contract >> of transaction_commit to either commit all the changes or none at all. >> >> Currently when using the transaction API to change refs, also reflogs are changed. >> But the changes to the reflogs just happen as a side effect and not as part of >> the atomic part of changes we want to commit altogether. > > Would you please explain why this patch series is still needed if my > "reflog_expire()" series is accepted? > > reflog_expire() already has its own little transaction. Reflog > expiration never needs to be combined with other reference changes, so > there is no need for reflog expiration to be done via a ref_transaction. > > ref_transaction_commit() already updates the reflog if necessary when a > reference is updated. That update is part of the containing > ref_transaction, because it is locked by the lock on the reference > itself at the beginning of the transaction and unlocked at the end of > the transaction [1]. It can't fail in normal circumstances because the > preconditions for the transaction have already been checked. > > As far as I can tell, the only thing left to do is provide a substitute > for log_ref_setup() a.k.a. create_reflog() that can be triggered within > a transaction. In my opinion the easiest way to do that is to give > ref_transaction_update()'s flag parameter an additional option, > REF_CREATE_REFLOG, which forces the reference's reflog to be created if > it does not already exist. > > 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. 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 ? 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. > Michael > > [1] The reflog updates are not atomic in the face of disk errors and > power outages and stuff, but neither are reference updates. In other > words, I think reflog updates in ref_transaction_commit() are done as > safely as reference updates, which is surely good enough for this > reference backend. Other reference backends can do a better job with > both while staying within the existing API. > > -- > Michael Haggerty > mhagger@xxxxxxxxxxxx > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html