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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 1, 2016 at 7:23 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]