Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

From: Jeff King <peff@peff.net>
Date: May 8, 2018, 18:08 UTC
Message-ID: <20180508180840.GA7210@sigill.intra.peff.net>
In-Reply-To: <01AF002E-C9E2-48FB-97AB-D9D903866310@novalis.org>

On Mon, May 07, 2018 at 05:12:27AM -0600, David Turner wrote:

Show 15 quoted lines

> >Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on
> >heap, 2017-09-05) this is safe though. Quite a few locks have already
> >been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles
> >non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify
> >locking
> >logic, 2017-10-05).  I could add a note to the commit message to make
> >this clear, like "After 076aa2cbda, locks no longer need to be static."
> 
> I am going to reply now to keep the thread moving, but I am on my
> phone with bad connectivity (few cell towers in Bears Ears), so I
> can't really check the code. Feel free to disregard if I am still
> wrong.
> 
> I saw that patch, but I thought the new logic required that cleanup
> funtions be called before the lock goes out of scope.

No, it should be fine. After 422a21c6a0 (tempfile: remove deactivated list entries, 2017-09-05) it became _possible_ to use a non-static tempfile. But it was dangerous, because if you failed to clean up, bad things would happen. So right after that in 076aa2cbda we switched to using the heap, which means the tempfile code takes full ownership, and the local lockfile variable is just a pointer to that storage.

-Peff