Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> 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@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)?

> +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;
>  }