Re: [PATCH v7 22/28] refs: new transaction related ref-store api

On 03/26/2017 04:42 AM, Nguyễn Thái Ngọc Duy wrote:

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. 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.