Re: [PATCH 02/11] treewide: prefer lockfiles on the stack

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 2, 2017, 04:12 UTC
Message-ID: <CAN0heSq-7CzyRV2KhzvC8CymoyMOWydPDi0kS5cg1BS7=oCELQ@mail.gmail.com>
In-Reply-To: <xmqqlgkuqkyi.fsf@gitster.mtv.corp.google.com>

On 2 October 2017 at 05:37, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
> This is not a show-stopper for this patch series, but just something
> I noticed, something that used to be unavoidable in the old world
> order that requires us to leak lockfiles, but now it becomes more
> obvious.
>
> update_index_if_able() calls write_lock_index() with the COMMIT_LOCK
> option, but it does so conditionally.  When it does not make the call,
> the lockfile is left behind to be cleaned up by the atexit() handler,
> but when it does, it is closed and released.
>
> Perhaps update_index_if_able() needs to be further cleaned up to
> always release the resource held by the lockfile structure?  Its
> caller cannot differentiate (and more importantly, its caller does
> not want to care) if the _if_able call actually has updated the
> index or not and act differently afterwards.

Ah, indeed. I should be able to do that rather easily on top. I tend to agree that the caller shouldn't have to worry about it since the function is just a one-off "if_able" without any return value. One caller actually does roll back the lock, but the others don't.