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