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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
>> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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: ...").

Thanks!

Good idea.
I will add a transaction->status field that can track OPEN/CLOSED/ERROR
and die(BUG:...) appropriately in _commit/_create/_delete/_update if
it has the wrong value.


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

ACK, but I don't think we need reusable transaction yet. Once the API
is cleaned up
it should be reasonably easy to add in the future if we see a need for it.
Sounds reasonable to you ?


>
> All in all, either way is OK with me.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@xxxxxxxxxxxx
> http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]