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

> On Mon, May 07, 2018 at 05:24:05PM +0200, Duy Nguyen wrote:
>
>> >>>> -       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.
>> >
>> > Thank you Duy for your comments. How about I write the commit message
>> > like so:
>>
>> +Jeff. Since he made it possible to remove lock file from the global
>> linked list, he probably knows well what to check when switching from
>> a static lock file to a stack-local one.
>
> It should be totally safe. If you look at "struct lock_file", it is now
> simply a pointer to a tempfile allocated on the heap (in fact, I thought
> about getting rid of lock_file entirely, but the diff is noisy and it
> actually has some value as an abstraction over a pure tempfile).
>
> If you fail to call a release function, it will just hang around until
> program exit, which is more or less what the static version would do.
> The big difference is that if we re-enter the function while still
> holding the lock, then the static version would BUG() on trying to use
> the already-active lockfile. Whereas after this series, we'd try to
> create a new lockfile and say "woah, somebody else is holding the lock".