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

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

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.