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: ...").
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@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