Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:

On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty [off-list ref] wrote:

quoted

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:

quoted

Change ref_transaction_commit to take a pointer to a pointer for the
transaction. This allows us to clear the transaction pointer from within
ref_transaction_commit so that it becomes NULL in the caller.

This makes transaction handling in the callers much nicer since instead of
having to write horrible code like :
      t = ref_transaction_begin();
      if ((!t ||
          ref_transaction_update(t, refname, sha1, oldval, flags,
                                 !!oldval)) ||
          (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
              ref_transaction_rollback(t);

we can now just do the much nicer
      t = ref_transaction_begin();
      if (!t ||
          ref_transaction_update(t, refname, sha1, oldval, flags,
                                 !!oldval) ||
          ref_transaction_commit(&t, action, &err)) {
              ref_transaction_rollback(t);

I understand the motivation for this change, but passing
pointer-to-pointer is unconventional in a case like this.  Unfortunately
I ran out of steam for the night before I could think about alternatives.

I see.
Yes passing a pointer to pointer is not ideal.
But I still want to be able to use the pattern
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(t, ...)) {
               ref_transaction_rollback(t);

Maybe the problem is that ref_transaction_commit() implicitely also
frees the transaction.


What about changing ref_transaction_commit() would NOT free the
transaction and thus a caller would
always have to explicitely free the transaction afterwards?

Something like this :
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(&t, ...)) {

You wouldn't need the "&" here then, right?

               ref_transaction_rollback(t);
       }
       ref_transaction_free(t);

That sounds like a better solution.  We would want to make sure that
ref_transaction_commit() / ref_transaction_rollback() leaves the
ref_transaction in a state that if it is accidentally passed to
ref_transaction_update() or its friends, the function calls die("BUG: ...").

Unless we want to make ref_transaction objects reusable.  But then we
would need an explicit "allocation" step in the boilerplate code:

    t = ref_transaction_alloc();
    while (something) {
            if (ref_transaction_begin(t) ||
                 ref_transaction_update(t, ...) ||
                 ref_transaction_commit(t, ...)) {
                    ref_transaction_rollback(t);
            }
    }
    ref_transaction_free(t);

Note that ref_transaction_begin() should in this case be converted to
return 0-on-OK, negative-on-error for consistency.

This would bring us back to the familiar pattern alloc...use...free.

I was going to say that the extra boilerplate is not worth it, and
anyway reusing ref_transaction objects won't save any significant work.
 But then it occurred to me that ref_transaction_alloc() might be a
place to do more expensive work, like creating a connection to a
database, so reuse could potentially be a bigger win.

All in all, either way is OK with me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/