On Mon, Apr 25, 2016 at 7:55 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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). > 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. > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html