[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress i
From:       William Baker <williamtbakeremail () gmail ! com>
Date:       2019-09-30 16:36:20
Message-ID: 07d054d2-c37e-5a8a-9550-c1d90bb80af7 () gmail ! com
[Download RAW message or body]

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



[prev in list] [next in list] [prev in thread] [next in thread]