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

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 28, 2016, 16:30 UTC
Message-ID: <CAP8UFD1=w2D-5q9bWrYqzL3v1q7fYi9imy1UhG2OaW2vB=2ECA@mail.gmail.com>
In-Reply-To: <xmqqzishlgj1.fsf@gitster.mtv.corp.google.com>

On Mon, Apr 25, 2016 at 7:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       /*
>>> +        * 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 there ever a time when lock_file is removed from the list (such as
>> upon successful write of the index), in which case it would be safe to
>> free() this?
>
> I do not think you need to think about "free"ing.

Yeah, lockfile.h says:

 * * Automatic cruft removal. If the program exits after we lock a
 *   file but before the changes have been committed, we want to make
 *   sure that we remove the lockfile. This is done by remembering the
 *   lockfiles we have created in a linked list and setting up an
 *   `atexit(3)` handler and a signal handler that clean up the
 *   lockfiles. This mechanism ensures that outstanding lockfiles are
 *   cleaned up if the program exits (including when `die()` is
 *   called) or if the program is terminated by a signal.

and:

 * The caller:
 *
 * * Allocates a `struct lock_file` either as a static variable or on
 *   the heap, initialized to zeros. Once you use the structure to
 *   call the `hold_lock_file_for_*()` family of functions, it belongs
 *   to the lockfile subsystem and its storage must remain valid
 *   throughout the life of the program (i.e. you cannot use an
 *   on-stack variable to hold this structure).

Show 10 quoted lines

> Even if the libified version of the apply internal can be called
> multiple times to process multiple patch inputs, there is no need to
> run multiple instances of it yet.  And a lockfile, after the caller
> finishes interacting with one file using it by calling commit or
> rollback, can be reused to interact with another file.
>
> So the cleanest would be to:
>
>  * make the caller of apply API responsible for preparing a static
>    or (allocating and leaking) lockfile instance,

Ok.

>  * make it pass a pointer to the lockfile to the apply API so that
>    it can store it in apply_state, and

Ok, I will add a new "struct lock_file *" parameter to init_apply_state(), for the caller of the apply API to do that. Though I think it should be ok for the caller to pass a NULL pointer and in this case have init_apply_state() allocate the lockfile file instance.

>  * have the caller use apply API feeding one patch at a time in a
>    loop, allowing the same lockfile instance used for each
>    "invocation".

Ok, the same lockfile instance will be used for each invocation.

Show 6 quoted lines

> I sounded as if I were advocating non-reentrant implementation in
> the introductory paragraph, but that is not the intention.  If you
> want to do N threads, you would prepare N lockfile instances, give
> them to N apply API instances to be stored in N apply_state
> instances, and you would have N parallel applications, if you wanted
> to.

Thanks, Christian.