Re: [PATCH] git-compat-util: introduce `count_t` typedef

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2025, 16:38 UTC
Message-ID: <xmqqa54bqe7d.fsf@gitster.g>
In-Reply-To: <20250807-pks-introduce-count-t-v1-1-e96be52d8db1@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

>  For C programs:
>  
> + - We use `size_t` to count the number of bytes and `count_t` to count the
> +   number of entities of a given type.

I am not interested in this specific implementation at all for a number of reasons, but I am excited to see people thinking about the issues. The following is a random list of things, both positive and negative, that came to my mind after skimming the changes.

 * We do not want to pretend that one size fits all.  If it were a
   good idea for developers to express "This variable is a simple
   counter that counts up from 0 and never goes negative" by using
   an unsigned type (which is dubious), it should be equally, or not
   more, a good idea to allow them to say "We will not have more
   than 256 fan-out directories under .git/objects/ and this is a
   counter to count them, so I know 'unsigned short' is big enough
   on any platforms".

 * As far as I can tell, the patch does not seem to address the
   biggest concern of unsigned integer wraparound.  We often see

	ALLOC_GROW(thing.entry, thing.nr + 1, thing.alloc);
    
   with the arithmetic "thing.nr + 1" checked by nobody.
   ALLOC_GROW_BY() is slightly better in this regard, but nobody
   uses it with only small exceptions.  And of course, alloc_nr()
   does even riskier arithmetic that is unchecked.

 * Standardising the names used for <item[], item_nr, item_alloc>
   somehow is very much welcome (we can see an example in the change
   to builtin/rm.c below).  Such a naming convention would allow us
   to write

	#define ALLOC_INCR(thing) ALLOC_INCR_BY(thing, 1)
	ALLOC_INCR_BY(thing, increment)

   that do ALLOC_GROW(thing, thing_nr + increment, thing_alloc) more
   safely than what the current code does, perhaps?  Also, we should
   be able to use any unsigned integral type and perform sensible
   bound checking with typeof().

 * The codebase avoids inventing a new type with typedef, with the
   exception of callback function type, following old tradition we
   inherited from the Linux kernel project.  And even when we create
   a new type, of course, we do not want to give it a name that ends
   with "_t".

Show 15 quoted lines

> diff --git a/builtin/rm.c b/builtin/rm.c
> index 05d89e98c3..99b845cf34 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -33,11 +33,11 @@ static const char * const builtin_rm_usage[] = {
>  };
>  
>  static struct {
> -	int nr, alloc;
>  	struct {
>  		const char *name;
>  		char is_submodule;
>  	} *entry;
> +	count_t entry_nr, entry_alloc;
>  } list;