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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 29, 2014, 09:25 UTC
Message-ID: <535F6FFD.90004@alum.mit.edu>
In-Reply-To: <CAL=YDWkSdiUd-6A60ncGaDrFV2pc5WtRMv8iCSHHqFLkKH=pfw@mail.gmail.com>

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

Show 49 quoted lines

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