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

[Duy Nguyen]

On Sat, Apr 1, 2017 at 11:54 AM, Michael Haggerty  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

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

[Michael Haggerty]

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

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

[Nguyễn Thái Ngọc Duy]

The transaction struct now takes a ref store at creation and will
operate on that ref store alone.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 55 
 refs.h   |  9 +
 refs/refs-internal.h |  1 +
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index c103f90b35..50ea24bd75 100644
--- a/refs.c
+++ b/refs.c
@@ -630,16 +630,20 @@ static int delete_pseudoref(const char *pseudoref, const 
unsigned char *old_sha1
return 0;
 }
 
-int delete_ref(const char *msg, const char *refname,
-  const unsigned char *old_sha1, unsigned int flags)
+int refs_delete_ref(struct ref_store *refs, const char *msg,
+   const char *refname,
+   const unsigned char *old_sha1,
+   unsigned int flags)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+   if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
return delete_pseudoref(refname, old_sha1);
+   }
 
-   transaction = ref_transaction_begin();
+   transaction = ref_store_transaction_begin(refs, );
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
   flags, msg, ) ||
@@ -654,6 +658,13 @@ int delete_ref(const char *msg, const char *refname,
return 0;
 }
 
+int delete_ref(const char *msg, const char *refname,
+  const unsigned char *old_sha1, unsigned int flags)
+{
+   return refs_delete_ref(get_main_ref_store(), msg, refname,
+  old_sha1, flags);
+}
+
 int copy_reflog_msg(char *buf, const char *msg)
 {
char *cp = buf;
@@ -813,11 +824,20 @@ int read_ref_at(const char *refname, unsigned int flags, 
unsigned long at_time,
return 1;
 }
 
-struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
+   struct strbuf *err)
 {
+   struct ref_transaction *tr;
assert(err);
 
-   return xcalloc(1, sizeof(struct ref_transaction));
+   tr = xcalloc(1, sizeof(struct ref_transaction));
+   tr->ref_store = refs;
+   return tr;
+}
+
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+{
+   return ref_store_transaction_begin(get_main_ref_store(), err);
 }
 
 void ref_transaction_free(struct ref_transaction *transaction)
@@ -934,18 +954,20 @@ int update_ref_oid(const char *msg, const char *refname,
old_oid ? old_oid->hash : NULL, flags, onerr);
 }
 
-int update_ref(const char *msg, const char *refname,
-  const unsigned char *new_sha1, const unsigned char *old_sha1,
-  unsigned int flags, enum action_on_err onerr)
+int refs_update_ref(struct ref_store *refs, const char *msg,
+   const char *refname, const unsigned char *new_sha1,
+   const unsigned char *old_sha1, unsigned int flags,
+   enum action_on_err onerr)
 {
struct ref_transaction *t = NULL;
struct strbuf err = STRBUF_INIT;
int ret = 0;
 
if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
ret = write_pseudoref(refname, new_sha1, old_sha1, );
} else {
-   t = ref_transaction_begin();
+   t = ref_store_transaction_begin(refs, );
if (!t ||
ref_transaction_update(t, refname, new_sha1, old_sha1,
   flags, msg, ) ||
@@ -976,6 +998,15 @@ int update_ref(const char *msg, const char *refname,
return 0;
 }
 
+int update_ref(const char *msg, const char *refname,
+  const unsigned char *new_sha1,
+  const unsigned char *old_sha1,
+  unsigned int flags, enum action_on_err onerr)
+{
+   return refs_update_ref(get_main_ref_store(), msg, refname, new_sha1,
+  old_sha1, flags, onerr);
+}
+
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
int i;
@@ -1607,7 +1638,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_main_ref_store();
+   struct ref_store *refs = transaction->ref_store;
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1726,7 +1757,7 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store