Re: [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2022, 21:38 UTC
Message-ID: <xmqqfskyfw68.fsf@gitster.g>
In-Reply-To: <cover.1653418457.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

>          verbatim reuse (c.f., `write_reused_pack_verbatim()`).

Unlike "e.g." and "i.e.", I think these should all be "cf." (there are many others).

Show 6 quoted lines

>     +    This patch handles the "preferred" pack (c.f., the section
>     +    "multi-pack-index reverse indexes" in
>     +    Documentation/technical/pack-format.txt) specially, since pack-objects
>     +    depends on reusing exact chunks of that pack verbatim in
>     +    reuse_partial_packfile_from_bitmap(). So if that pack cannot be loaded,
>     +    the utility of a bitmap is significantly diminished.

It explains why we care about the "preferred" pack, which is a nice clarification. It hints that the other packs do not matter as much, and it is clearly stated that how they are handled is ...

Show 10 quoted lines

>     +    Similar to dc1daacdcc, we could technically just add this check in
>     +    reuse_partial_packfile_from_bitmap(), since it's possible to use a MIDX
>     +    .bitmap without needing to open any of its packs. But it's simpler to do
>     +    the check as early as possible, covering all direct uses of the
>     +    preferred pack. Note that doing this check early requires us to call
>     +    prepare_midx_pack() early, too, so move the relevant part of that loop
>     +    from load_reverse_index() into open_midx_bitmap_1().
>     +
>     +    Subsequent patches handle the non-preferred packs in a slightly
>     +    different fashion.

... left for later steps.

Excellent write-up.

Show 14 quoted lines

>          Signed-off-by: Taylor Blau <me@ttaylorr.com>
>      
>     @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>      +
>      +	preferred = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];
>      +	if (!is_pack_valid(preferred)) {
>     -+		close(fd);
>      +		warning(_("preferred pack (%s) is invalid"),
>      +			preferred->pack_name);
>      +		goto cleanup;
> 2:  9adf6e1341 < -:  ---------- builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects
> -:  ---------- > 2:  2719d33f32 builtin/pack-objects.c: avoid redundant NULL check
> -:  ---------- > 3:  cdc3265ec2 builtin/pack-objects.c: ensure included `--stdin-packs` exist
> -:  ---------- > 4:  3fc3a95517 builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects