Re: [PATCH 4/5] lock_file: make function-local locks non-static

From: Duy Nguyen <pclouds@gmail.com>
Date: May 6, 2018, 17:42 UTC
Message-ID: <CACsJy8Btuc2J4aCTymkvLYyMV5zJrdMUdtV5NDnPqXOjsTVw4w@mail.gmail.com>
In-Reply-To: <CACsJy8DyRhQ0DKy8UyK+r7Kmw=0hHD=W6aXXKutk4e-wtGTdNg@mail.gmail.com>

On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 33 quoted lines

> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> 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@gmail.com>
>> ---
>>  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