Re: [PATCH 14/17] midx: use chunk-format read API

[Derrick Stolee <stolee@gmail.com>]

On 1/26/2021 10:06 PM, Taylor Blau wrote:
> On Tue, Jan 26, 2021 at 04:01:23PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Instead of parsing the table of contents directly, use the chunk-format
>> API methods read_table_of_contents() and pair_chunk(). In particular, we
>> can use the return value of pair_chunk() to generate an error when a
>> required chunk is missing.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  midx.c                      | 103 ++++++++++++++++++++----------------
>>  t/t5319-multi-pack-index.sh |   6 +--
>>  2 files changed, 60 insertions(+), 49 deletions(-)
>>
>> diff --git a/midx.c b/midx.c
>> index 0bfd2d802b6..dd019c00795 100644
>> --- a/midx.c
>> +++ b/midx.c
>> @@ -54,6 +54,51 @@ static char *get_midx_filename(const char *object_dir)
>>  	return xstrfmt("%s/pack/multi-pack-index", object_dir);
>>  }
>>
>> +static int midx_read_pack_names(const unsigned char *chunk_start,
>> +				size_t chunk_size, void *data)
>> +{
>> +	struct multi_pack_index *m = (struct multi_pack_index *)data;
>> +	m->chunk_pack_names = chunk_start;
>> +	return 0;
>> +}
> 
> There are a lot of these callbacks that just assign some 'void **' to
> point at chunk_start.
> 
> Maybe a good use of the "pair_chunk" name would be something like:
> 
>     int pair_chunk(struct chunkfile *cf, uint32_t id, const unsigned char **p);
> 
> which does the same as what you wrote here. So instead of what you
> wrote, you could instead:
> 
>     pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names);
> 
> This would be in addition to the richer callback-style function which
> allows the caller greater flexibility (e.g., for the Bloom filter
> related readers in the commit-graph code).

You're right that _most_ callers just want to assign a pointer,
so this mechanism would be better. I'll make a different function,
read_chunk() perhaps, that relies on a callback for advanced users.

Thanks,
-Stolee