Taylor Blau <me@xxxxxxxxxxxx> writes:
> 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@xxxxxxxxxxxx>
> ---
> 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)?
> +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.
> 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?
> @@ -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?
> 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.
> @@ -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;
> }
