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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2016, 18:03 UTC
Message-ID: <xmqq4m9ap2ub.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160603165852.12399-1-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 5 quoted lines

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

Show 13 quoted lines

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