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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>> 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/