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

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

On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:

Show 27 quoted lines

> 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.

-- 
Duy