Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer
On 04/29/2014 08:58 PM, Ronnie Sahlberg wrote: > On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty > wrote: >> [...] >> Unless we want to make ref_transaction objects reusable. [...] > 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 ? Yes, reusable transaction objects would be pretty straightforward to retrofit, so I agree that there is no need to implement it now. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer
On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty wrote:
> On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
>> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty
>> 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
> mhag...@alum.mit.edu
> http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer
On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty
> 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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer
On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty 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, ...)) {
ref_transaction_rollback(t);
}
ref_transaction_free(t);
>
> Regarding patches 01-18, I agree with Jonathan's comments and made a few
> of my own. I definitely don't think that code like "!(t = NULL)" should
> stay in our code base as more than a transitional state. But I'd have
> to play with the alternatives before I can form an opinion on your
> suggestion in this patch.
>
> Thanks for working on this!
>
> Michael
>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> branch.c | 4 ++--
>> builtin/commit.c | 2 +-
>> builtin/replace.c| 2 +-
>> builtin/tag.c| 2 +-
>> builtin/update-ref.c | 2 +-
>> fast-import.c| 7 +++
>> refs.c | 18 ++
>> refs.h | 3 ++-
>> sequencer.c | 7 +++
>> 9 files changed, 24 insertions(+), 23 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 23cde1e..5d68467 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -303,14 +303,14 @@ void create_branch(const char *head,
>> if (!transaction ||
>> ref_transaction_update(transaction, ref.buf, sha1,
>> NULL, 0, 0) ||
>> - ref_transaction_commit(transaction, msg, &err))
>> + ref_transaction_commit(&transaction, msg, &err))
>> die_errno(_("%s: failed to write ref: %s"),
>> ref.buf, err);
>> } else {
>> if (!transaction ||
>> ref_transaction_create(transaction, ref.buf, sha1,
>> 0) ||
>> - ref_transaction_commit(transaction, msg, &err))
>> + ref_transaction_commit(&transaction, msg, &err))
>> die_errno(_("%s: failed to write ref: %s"),
>> ref.buf, err);
>> }
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 7e4c306..3142827 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char
>> *prefix)
>> current_head ?
>> current_head->object.sha1 : NULL,
>> 0, !!current_head) ||
>> - ref_transaction_commit(transaction, sb.buf, &err)) {
>> + ref_transaction_commit(&transaction, sb.buf, &err)) {
>> rollback_index_files();
>> die(_("HEAD: cannot update ref: %s"), err);
>> }
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index cf0f56d..51e9ddf 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const
>> char *replace_ref,
>> if (!transaction ||
>> ref_transaction_update(transaction, ref, repl, prev,
>>
Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer
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.
Regarding patches 01-18, I agree with Jonathan's comments and made a few
of my own. I definitely don't think that code like "!(t = NULL)" should
stay in our code base as more than a transitional state. But I'd have
to play with the alternatives before I can form an opinion on your
suggestion in this patch.
Thanks for working on this!
Michael
> Signed-off-by: Ronnie Sahlberg
> ---
> branch.c | 4 ++--
> builtin/commit.c | 2 +-
> builtin/replace.c| 2 +-
> builtin/tag.c| 2 +-
> builtin/update-ref.c | 2 +-
> fast-import.c| 7 +++
> refs.c | 18 ++
> refs.h | 3 ++-
> sequencer.c | 7 +++
> 9 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 23cde1e..5d68467 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -303,14 +303,14 @@ void create_branch(const char *head,
> if (!transaction ||
> ref_transaction_update(transaction, ref.buf, sha1,
> NULL, 0, 0) ||
> - ref_transaction_commit(transaction, msg, &err))
> + ref_transaction_commit(&transaction, msg, &err))
> die_errno(_("%s: failed to write ref: %s"),
> ref.buf, err);
> } else {
> if (!transaction ||
> ref_transaction_create(transaction, ref.buf, sha1,
> 0) ||
> - ref_transaction_commit(transaction, msg, &err))
> + ref_transaction_commit(&transaction, msg, &err))
> die_errno(_("%s: failed to write ref: %s"),
> ref.buf, err);
> }
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 7e4c306..3142827 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
> current_head ?
> current_head->object.sha1 : NULL,
> 0, !!current_head) ||
> - ref_transaction_commit(transaction, sb.buf, &err)) {
> + ref_transaction_commit(&transaction, sb.buf, &err)) {
> rollback_index_files();
> die(_("HEAD: cannot update ref: %s"), err);
> }
> diff --git a/builtin/replace.c b/builtin/replace.c
> index cf0f56d..51e9ddf 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const
> char *replace_ref,
> if (!transaction ||
> ref_transaction_update(transaction, ref, repl, prev,
> 0, !is_null_sha1(prev)) ||
> - ref_transaction_commit(transaction, NULL, &err))
> + ref_transaction_commit(&transaction, NULL, &err))
> die(_("%s: failed to replace ref: %s"), ref, err);
>
> return 0;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index dd53fb4..60b57a1 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char
> *prefix)
> if (!transaction ||
> ref_transaction_update(transaction, ref.buf, object, prev,
> 0, !is_null_sha1(prev)) ||
> - ref_transaction_commit(transaction, NULL, &err))
> + ref_transaction_commit(&transaction, NULL, &err))
> die(_("%s: cannot update the ref: %s"), ref.buf, err);
> if (force && !is_null_sha1(prev) && hashcmp(prev, object))
> printf(_("Updated tag '%s' (was %s)\n"), tag,
> fin
