Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'
On Fri, Jun 3, 2016 at 8:03 PM, Junio C Hamano wrote:
> Christian Couder writes:
>
>> This is to replace:
>>
>> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct
>> apply_state'"
>>
>> from the "libify apply and use lib in am, part 1" patch series.
>
> Thanks; will replace the tip 2 patches and requeue.
>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 5027f1b..cc635eb 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -52,6 +52,12 @@ struct apply_state {
>> const char *prefix;
>> int prefix_length;
>>
>> + /*
>> + * Since lockfile.c keeps a linked list of all created
>> + * lock_file structures, it isn't safe to free(lock_file).
>> + */
>> + struct lock_file *lock_file;
>
> Is this even an issue for this thing anymore? It is the
> responsibilty of the API user to manage the lifetime of what
> lock_file points at. The caller may have it on heap and let it
> leak, or it may have in BSS in which case it won't even dream about
> freeing it.
>
> If a comment were needed for this field, it should say "when
> discarding/freeing apply_state, just discard this pointer without
> touching what the pointer points at; the storage the pointer points
> at does not belong to the API implementation but belongs to the API
> user".
>
> But I do not think such a comment is needed, because the situation
> is the same as other fields like *prefix, and for the same reason we
> do not do anything to these fields in clear_apply_state().
Ok, I just resent without this comment.
I still don't understand why there is an added:
From: Christian Couder
at the beginning of the emails.
> Other than that this looks great.
Thanks,
Christian.
--
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 v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'
Christian Couder writes:
> This is to replace:
>
> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct
> apply_state'"
>
> from the "libify apply and use lib in am, part 1" patch series.
Thanks; will replace the tip 2 patches and requeue.
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 5027f1b..cc635eb 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -52,6 +52,12 @@ struct apply_state {
> const char *prefix;
> int prefix_length;
>
> + /*
> + * Since lockfile.c keeps a linked list of all created
> + * lock_file structures, it isn't safe to free(lock_file).
> + */
> + struct lock_file *lock_file;
Is this even an issue for this thing anymore? It is the
responsibilty of the API user to manage the lifetime of what
lock_file points at. The caller may have it on heap and let it
leak, or it may have in BSS in which case it won't even dream about
freeing it.
If a comment were needed for this field, it should say "when
discarding/freeing apply_state, just discard this pointer without
touching what the pointer points at; the storage the pointer points
at does not belong to the API implementation but belongs to the API
user".
But I do not think such a comment is needed, because the situation
is the same as other fields like *prefix, and for the same reason we
do not do anything to these fields in clear_apply_state().
Other than that this looks great.
--
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 v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'
On Fri, Jun 3, 2016 at 6:58 PM, Christian Couder wrote: > From: Christian Couder Sorry for this spurious "From:" line. It looks like send-email added it, and I don't understand why it does it now. -- 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
