Re: [PATCH 0/2] pack-write,repack: prevent opening packs too early

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 7, 2021, 19:48 UTC
Message-ID: <877dfskvxc.fsf@evledraar.gmail.com>
In-Reply-To: <cover.1630461918.git.me@ttaylorr.com>

On Tue, Aug 31 2021, Taylor Blau wrote:

Show 22 quoted lines

> This pair of patches fixes a race where the .idx is moved into place
> before the .rev file, allowing the pack to be in a state where it
> appears a .rev file wasn't generated.
>
> This can cause Git to inadvertently take the slow "generate the
> reverse index on-the-fly", which does not impact correctness, but is
> unnecessarily slow when compared to reading the .rev file.
>
> The race is fixed by moving the .idx into place only after all other
> pack-related files have already been written. The first patch fixes
> the direct `pack-objects` case, and the second patch fixes `repack`
> (which also renames pack files around).
>
> Thanks in advance for your review.
>
> Taylor Blau (2):
>   pack-write.c: rename `.idx` file into place last
>   builtin/repack.c: move `.idx` files into place last
>
>  builtin/repack.c |  2 +-
>  pack-write.c     | 12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)

I've reviewed this to the point of coming up with my own series on top of it, but it can be considered separately: https://lore.kernel.org/git/cover-0.3-00000000000-20210907T193600Z-avarab@gmail.com/

The only change (if any) would be to perhaps update the commit message(s) to note that the *.bitmap case is left, and that the promise of "closing the race" is still subject to the vagaries of our non-use of fsync() here, so pedantically speaking the race is still there.

But even without those suggested changes:

Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>