On 9/27/19 8:49 PM, Junio C Hamano wrote:
>>>> diff --git a/midx.c b/midx.c
>>>> index b2673f52e8..54e4e93b2b 100644
>>>> --- a/midx.c
>>>> +++ b/midx.c
>>>> @@ -449,6 +449,8 @@ struct pack_list {
>>>> uint32_t nr;
>>>> uint32_t alloc;
>>>> struct multi_pack_index *m;
>>>> + struct progress *progress;
>>>> + uint32_t pack_paths_checked;
>>>> };
>>
>> I went with u32 here to match the data type used to track how many
>> entries are in the pack_list ('nr' is a u32).
>
> That kind of parallel is valid when you could compare nr with this
> new thing (or put it differently, the existing nr and this new thing
> counts the same). Are they both about the number of packs?
>
Both 'nr' and 'pack_paths_checked' are about the number of packs.
'nr' tracks the number of packs in the multi-pack-index and it grows
as add_pack_to_midx() finds new packs to add. 'pack_paths_checked' is
the number of pack ".idx" files that have been checked by add_pack_to_midx().
>> I could switch to this to an unsigned but we would run the (extremely
>> unlikely) risk of having more than 65k packs on a system where
>> unsigned is 16 bits.
>
> Why? If an arch is small enough that the natural integer size is 16-bit,
> then limiting the total number of packs to 65k sound entirely
> sensible.> The only reason why you'd want fixed (across platforms and
> architectures) type is when you want to make sure that a file
> storing the literal values taken from these fields are portable and
> everybody is limited the same way. If a platform's natural integer
> is 64-bit, by artificially limiting the size of this field to u32,
> you are making disservice to the platform users, and more
> importantly, you are wasting developers' time by forcing them to
> wonder if there is a reason behind the choice of u32 (does it really
> need to be able to store up to 4G, even on a smaller machines? Is
> it necessary to refuse to store more than 4G? What are the
> reasons?), like me wondering about these questions and writing them
> down here.
>
> So, unless there is a reason why this must be of fixed type, I'd say
> just an unsigned would be the most reasonable choice.
>
I agree that it's best to avoid using a fixed type here unless there's
a reason that it must be. Do you think that both 'nr' and
'pack_paths_checked' being about the number of packs is a strong enough
reason to use u32 for 'pack_paths_checked'? If not, I will update
'pack_paths_checked' in the next path series to be an unsigned int.
Thanks,
William
