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

[Junio C Hamano <gitster@pobox.com>]

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().

Other than that this looks great.