Subject: Re: [PATCH v2 0/3] prevent opening packs too early

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 09 Sep 2021 04:36:17 +0200

On Thu, Sep 09 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Sep 08 2021, Taylor Blau wrote:
>
>> On Thu, Sep 09, 2021 at 02:50:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Thu, Sep 09 2021, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> > On Wed, Sep 08 2021, Taylor Blau wrote:
>>> >
>>> >> Here is a very small reroll of a couple of patches to make sure that packs are
>>> >> not read before all of their auxiliary files exist and are moved into place, by
>>> >> renaming the .idx file into place last.
>>> >>
>>> >> New since the original version is a patch to apply similar treatment to
>>> >> index-pack, as noticed by Peff in [1]. This should be queued before Ævar's
>>> >> series on top.
>>> >
>>> > I read Junio's earlier <[email-masked]>[1] as a suggestion
>>> > that we combine the two into a singe series. I do think that yields a
>>> > better end-result, in particular your 1/3 is much more readable if the
>>> > refactoring in my 2/4.
>>> >
>>> > I'm mot of the way done with such a rewrite, which also incorporates
>>> > your suggestion for how to manage memory in rename_tmp_packfile(), but
>>> > I'll hold of on what you & Junio have to say about next steps before
>>> > adding to the re-roll competition Junio mentioned...
>>> >
>>> > 1. https://lore.kernel.org/git/[email-masked]
>>>
>>> I've got that at
>>> https://github.com/git/git/compare/master...avar:avar-tb/idx-rename-race-3
>>
>> Beautiful :-).
>>
>> I mentioned in my response to [1] that I missed that message when
>> sending v2 of my series to fix a couple of these races. And I was even
>> happy to unify our two series, but you did all of the work for me while
>> I was eating dinner. Thank you!
>>
>> I fetched your branch, reviewed, and am happy with the result. So I
>> would be content to apply my s-o-b to your patches and resubmit them as
>> a unified series.
>
> Sounds good to me, also in particular any typo/grammar etc. fixes to my
> changes are most welcome, I tend to sneak those in, and any code changes
> you see fit of course.
>
>> But I did wonder if you wanted to include [2] in this series as well.
>> It's kind of different, but also related enough that it probably makes
>> sense to just pull it in. So I'm inclined to just do that, unless you
>> have any objections.
>
> In this latest push-out of "next" Junio's merged that one down already,
> see 1972c5931bb (Merge branch 'ab/reverse-midx-optim' into next,
> 2021-09-08).
>
> So I think at this point it could be built on top of that, but given
> that the two don't conflict in any way (textually or semantically) it's
> probably better to base this larger topic on "master" and proceed with
> them independently.
>
>> [2]: https://lore.kernel.org/git/[email-masked]/

Also: That avar-tb/idx-rename-race-3 has a bug where I didn't update the
bulk-checkin.c code accordingly, missed it because I was running a glob
of tests that included "*pack*,*rev*", but missed "*large*". I've got a
avar-tb/idx-rename-race-4 which fixes it. Sorry about that.

https://github.com/git/git/compare/master...avar:avar-tb/idx-rename-race-4