Re: [PATCH 09/23] midx: write pack names in chunk

> On 6/7/2018 2:26 PM, Duy Nguyen wrote:
>> On Thu, Jun 7, 2018 at 4:03 PM, Derrick Stolee <stolee@gmail.com> wrote:
>>> @@ -74,6 +80,31 @@ struct midxed_git *load_midxed_git(const char *object_dir)
>>>          m->num_chunks = *(m->data + 6);
>>>          m->num_packs = get_be32(m->data + 8);
>>>
>>> +       for (i = 0; i < m->num_chunks; i++) {
>>> +               uint32_t chunk_id = get_be32(m->data + 12 + MIDX_CHUNKLOOKUP_WIDTH * i);
>>> +               uint64_t chunk_offset = get_be64(m->data + 16 + MIDX_CHUNKLOOKUP_WIDTH * i);
>> Would be good to reduce magic numbers like 12 and 16, I think you have
>> some header length constants for those already.
>>
>>> +               switch (chunk_id) {
>>> +                       case MIDX_CHUNKID_PACKNAMES:
>>> +                               m->chunk_pack_names = m->data + chunk_offset;
>>> +                               break;

>>> +                       case 0:
>>> +                               die("terminating MIDX chunk id appears earlier than expected");
>> _()
>
> This die() and others like it are not marked for translation on
> purpose, as they should never be seen by an end-user.

>>> +                       default:
>>> +                               /*
>>> +                                * Do nothing on unrecognized chunks, allowing future
>>> +                                * extensions to add optional chunks.
>>> +                                */
>> I wrote about the chunk term reminding me of PNG format then deleted
>> it. But it may help to do similar to PNG here. The first letter can
>> let us know if the chunk is optional and can be safely ignored. E.g.
>> uppercase first letter cannot be ignored, lowercase go wild.
>
> That's an interesting way to think about it. That way you could add a
> new "required" chunk and earlier versions could die() realizing they
> don't know how to parse that required chunk.