On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
>> These `struct lock_file`s are local to their respective functions and we
>> can drop their staticness.
>>
>> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
>> ---
>> apply.c | 2 +-
>> builtin/describe.c | 2 +-
>> builtin/difftool.c | 2 +-
>> builtin/gc.c | 2 +-
>> builtin/merge.c | 4 ++--
>> builtin/receive-pack.c | 2 +-
>> bundle.c | 2 +-
>> fast-import.c | 2 +-
>> refs/files-backend.c | 2 +-
>> shallow.c | 2 +-
>> 10 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/apply.c b/apply.c
>> index 7e5792c996..07b14d1127 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, struct patch *list)
>> {
>> struct patch *patch;
>> struct index_state result = { NULL };
>> - static struct lock_file lock;
>> + struct lock_file lock = LOCK_INIT;
>
> Is it really safe to do this? I vaguely remember something about
> (global) linked list and signal handling which could trigger any time
> and probably at atexit() time too (i.e. die()). You don't want to
> depend on stack-based variables in that case.
So I dug in a bit more about this. The original implementation does
not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
Implement git-checkout-cache -u to update stat information in the
cache. - 2005-05-15). The situation has changed since 422a21c6a0
(tempfile: remove deactivated list entries - 2017-09-05). At the end
of that second commit, Jeff mentioned "We can clean them up
individually" which I guess is what these patches do. Though I do not
know if we need to make sure to call "release" function or something/
Either way you need more explanation and assurance than just "we can
drop their staticness" in the commit mesage.
--
Duy
