Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 29, 2025, 20:47 UTC
- Message-ID
- <xmqqr007jg9b.fsf@gitster.g>
- In-Reply-To
- <d3508d3cfbddb512dbca4c2177731fffb5827084.1748473122.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 15 quoted lines
> Accessing a MIDX's 'pack_names' array is somewhat error-prone when > dealing with incremental MIDX chains, where the (global) pack_int_id for > some pack may differ from the containing layer's index for that pack. > > Introduce `nth_midxed_pack_name()` in an effort to reduce a common > source of errors by discouraging external callers from accessing a > layer's `pack_names` array directly. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > midx.c | 7 +++++++ > midx.h | 2 ++ > pack-bitmap.c | 4 ++-- > t/helper/test-read-midx.c | 7 ++++--- > 4 files changed, 15 insertions(+), 5 deletions(-)
Hmph, I am not sure if an accessor really makes it harder to make mistakes, but I'd expect it to be mechanical rewrite from a[n] to fn(a, n)?
Show 6 quoted lines
> +const char *nth_midxed_pack_name(struct multi_pack_index *m,
> + uint32_t pack_int_id)
> +{
> + uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);
> + return m->pack_names[local_pack_int_id];
> +}OK, midx_for_pack() takes a pack_int_id, finds the midx that contains the pack (by updating the 'm' via its pointer arg), and turns pack_int_id into local offset into m->pack_names[] array, and returns that string.
Show 10 quoted lines
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index b9f1d86604..8ddc150778 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> for (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {
> if (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {
> warning(_("could not open pack %s"),
> - bitmap_git->midx->pack_names[i]);
> + nth_midxed_pack_name(bitmap_git->midx, i));This loop runs from 0 to (num_packs + num_packs_in_base). I understand if it runs from num_packs_in_base to (num_packs + num_packs_in_base), iterating only on this layer, but probably this just tries to open everything (i.e. in addition to num_packs we have, we know num_packs_in_base packs are there in our base layer(s), so we iterate from 0 to that number).
The updated code converts the global 'i', which runs from 0 to "everything under us" num_packs + num_packs_in_base, to corresponding layer midx plus offset in it, so it looks good, but then, is the original reference to bitmap_git->midx->pack_names[i] even correct? If we have a base, i can run larger than bitmap_git->midx->num_packs, which is the size of the array bitmap_git->midx->pack_names[].
Or, unlike how the proposed log message portrayed this change as (i.e. code clean up), does this patch fix real bugs that manifest only when midx files are chained?
Show 9 quoted lines
> @@ -2469,7 +2469,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,
> struct bitmapped_pack pack;
> if (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {
> warning(_("unable to load pack: '%s', disabling pack-reuse"),
> - bitmap_git->midx->pack_names[i]);
> + nth_midxed_pack_name(bitmap_git->midx, i));
> free(packs);
> return;
> }Similar to the above, this is also in a loop that runs from 0 to num_packs+num_packs_in_base. Is the array access to find the name for the error message in the original even correct when midx are chained?
Show 13 quoted lines
> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> index ac81390899..fbed0f6919 100644
> --- a/t/helper/test-read-midx.c
> +++ b/t/helper/test-read-midx.c
> @@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,
> printf("\nnum_objects: %d\n", m->num_objects);
>
> printf("packs:\n");
> - for (i = 0; i < m->num_packs; i++)
> - printf("%s\n", m->pack_names[i]);
> + for (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;
> + i++)
> + printf("%s\n", nth_midxed_pack_name(m, i));OK. This used to iterate from 0 to num_packs using the local offset. Now it iterates from num_packs_in_base to num_packs_in_base+num_packs, meaning we iterate over packs in the given midx. No change in behaviour, as accesses to m->pack_names[i] using the local offset in the original was correct, and the updated code iterates using the global offset. This is not a bugfix but is a code cleanup.
Show 6 quoted lines
> @@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)
> return 1;
> }
>
> - printf("%s\n", midx->pack_names[preferred_pack]);
> + printf("%s\n", nth_midxed_pack_name(midx, preferred_pack));Again, is the original buggy when midx are chained?
> close_midx(midx); > return 0; > }