On Sat, Apr 1, 2017 at 11:54 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 03/26/2017 04:42 AM, Nguyễn Thái Ngọc Duy wrote: >> The transaction struct now takes a ref store at creation and will >> operate on that ref store alone. > > Having worked downstream of this patch series for a while, I started to > wonder why a `ref_store` has to be baked into a `ref_transaction` at > creation. I haven't noticed any technical reason why it must be so. > > If we expected to need to virtualize `ref_transaction_begin()` sometime, > then of course your design would be preferable. > > It surprised be, because until now `ref_transaction` has been a plain > old data structure unconnected with a particular `ref_store`. I would > have expected `ref_transaction_commit()` to gain a more general sibling, > `refs_ref_transaction_commit()` [*], that takes an additional `ref_store > *` object argument, and for `ref_transaction_commit()` to call that > function. I almost went with that, but the question that stopped me was, "what if people do X on ref store A, then Y on ref store B, using the same transaction?". I suppose we could even support that (which basically turns one transaction into two), or check for same ref store and reject. It's too complicated though. So I went a safe way, making sure people won't be able to change ref store half way. > It would feel slightly more natural to me, given that > `transaction_commit` is a virtual method of the `ref_store` class, for > `refs_ref_transaction_commit()` to take an explicit `refs` pointer > rather than having that pointer buried in the `ref_transaction`. > > I think this is mostly an aesthetic issue (about which opinions can > legitimately differ) rather than a concrete problem. I haven't yet had > any difficulties working with your interface. But I wanted to mention my > observation anyway. As far as I'm concerned, you don't need to change it. > >> [...] > > Michael > > [*] The name could obviously be improved, but you get the idea. > -- Duy