Re: [PATCH 00/11] various lockfile-leaks and -fixes

From: Martin Ågren <martin.agren@gmail.com>
Date: Oct 2, 2017, 10:19 UTC
Message-ID: <CAN0heSqsZXoiVs9s7sZ-d_X4ghU4ciG_=8ppY+w0XG535tt2EA@mail.gmail.com>
In-Reply-To: <xmqqbmlqoydg.fsf@gitster.mtv.corp.google.com>

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

Show 15 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> I've read through each and with the exception of patches 10 and 11, they
>> all look good to me.
>>
>> For 10 and 11, I think you've convinced me that the current behavior is
>> buggy. I do wonder if the subtleties might be easier to understand as a
>> single patch, but I'm OK either way.
>
> I found it was not too hard to understand what is going on with
> 10/11 as separate patches, and I suspect that it would be also OK if
> they were a single patch (but I cannot easily "unsee" them, so this
> is merely a suspicion).
>
> Thanks, both.  Let's merge this to 'next' soonish.

Thanks both of you for your comments. Based on them, I have made the following notes:

Patch 1: Make the logic a bit more future-proof/safe.

Patch 9: Instead of the two flags and the `HAS_SINGLE_BIT`-"cuteness", just drop `CLOSE_LOCK` altogether. That should simplify things a bit further. It might also help me come up with better explanations for patches 10-11.

Patches 10-11: I'm sorry that I didn't succeed better in explaining this. They have a couple of different things going on, but they are obviously related and even a bit entangled. Let me try and see what I can do, I'll try squashing them also. Maybe with `CLOSE_LOCK` gone, I'll do better.

Especially 9-11 make me feel like wanting to re-roll this, for future readers if nothing else. I expect to be able to do so in the middle of this week (I don't know how this interferes with Junio's definition of "soonish").

As fallout that could be taken care of now or later, I've noted this:

Patch 2: A follow-up could teach `update_index_if_able()` to always commit or roll back.

Patch 5: A follow-up could clean up a similar construct in `cmd_checkout_index()`.

Thanks again for your input. It's much appreciated.

Martin