Re: [PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2016, 17:23 UTC
Message-ID: <xmqq8tyozuul.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160524081126.16973-49-chriscool@tuxfamily.org>

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

> We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
> keeps a linked list of all created lock_file structures. So let's make the
> 'lock_file' variable a pointer to a 'struct lock_file'

Good.

> As the same instance of this struct can be reused, let's add an argument
> to init_apply_state(), so that the caller can supply the same instance to
> different calls.

Good.

> And let's alloc an instance in init_apply_state(), if the
> caller doesn't want to supply one.

This is questionable.

> -static struct lock_file lock_file;

I'd rather leave this as-is, and pass the pointer to it to init_apply_state(). For the purpose of "cmd_apply()" which is the first user of the "(semi-)libified apply API", that reads a single patch and applies it before exiting, that is sufficient.

As to the "if the caller does not want to supply one, we will allocate one that will definitely be leaked", I am mildly opposed.

By making it the responsibility of the caller, whenever a new caller of the libified apply API is written, those who write it is _forced_ to think about not leaking the lockfile structure, which is a good thing.

Other than that, all 49 patches look sensible to me.

Thanks for working on this.