On Wed, Jun 1, 2016 at 7:23 PM, Junio C Hamano <gits...@pobox.com> wrote: > Christian Couder <christian.cou...@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.
Ok, I will leave this as-is. > 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. Ok, I will remove the allocation in case the caller pass NULL. > Other than that, all 49 patches look sensible to me. Ok, I think I will resend only this patch (48/49) with the changes you suggest, and maybe the next one 49/49 to avoid spamming the list. > Thanks for working on this. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
