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

On Sat, Apr 1, 2017 at 11:54 AM, Michael Haggerty [off-list ref] wrote:

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

quoted

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.

quoted

[...]

Michael

[*] The name could obviously be improved, but you get the idea.

-- 
Duy