Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

> Christian Couder <christian.couder@gmail.com> 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().