On Fri, Dec 12, 2014 at 8: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? If we can live with the fact that reflog and refs have different APIs then, we can probably drop that series. That series may also have performance issues, Junio pointed out. Most likely I'll look at the remaining 4 patch series from Ronnie, if I can put them on top of your series now. > > 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. I already ported the patch which replaces log_ref_setup() by create_reflog() on top of your series-v1, but I refrained to send it out as Junio seems to dislike invasive patches[1] or possible merge conflicts so I was waiting for discussion on your patch series calm down and be merged to next at least. Apart from that rename, I also looked into flagging REF_CREATE_REFLOG additionally. Though I run into problems there (test suite doesn't pass and I did not find the mistake for 5 hours :/) > > What am I missing? I think the original plan of Ronnie was to have one and only one transaction API, which would be used for anything eventually (refs, reflogs, config, what have you) If we go for a relaxed version of that, we'd be fine. Thanks, Stefan [1] http://www.spinics.net/lists/git/msg243451.html The patch in question is not an invasive patch, but still touching places which may be touched currently by your series. So I just wanted to wait a bit. -- 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