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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Aug 07, 2025 at 09:38:46AM -0700, Junio C Hamano wrote:
> 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".

This to me is the most compelling argument against a "count_t" typedef
or something similar. Different callers have different needs (the ones
you pointed out above are the ones that I thought of as most relevant),
and we shouldn't force them to all use the same type, or pretend that
one type is best for all of them.

>  * 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.

I wonder if we should push more people towards ALLOC_GROW_BY() for that
reason. We could do something like recommend that callers use
ALLOC_GROW_BY() instead of ALLOC_GROW() in cases like:

    @@
    expression array, nr, n, alloc;
    @@
    - ALLOC_GROW(array, nr + n, alloc)
    + ALLOC_GROW_BY(array, nr, n, alloc)

, but I'm not sure that's a good idea as a blanket rule, since it's
changing the behavior away from using alloc_nr() to instead grow by a
fixed amount.

We have definitely talked before about adding overflow checks to
alloc_nr() before, but I think the slow-down made it a non-starter
(IIRC). I wonder if something like this:

    diff --git a/git-compat-util.h b/git-compat-util.h
    index 9408f463e31..22b8701b40d 100644
    --- a/git-compat-util.h
    +++ b/git-compat-util.h
    @@ -852,11 +852,14 @@ static inline void move_array(void *dst, const void *src, size_t n, size_t size)
      */
     #define ALLOC_GROW(x, nr, alloc) \
      do { \
    +		size_t __alloc__ = alloc; \
        if ((nr) > alloc) { \
          if (alloc_nr(alloc) < (nr)) \
            alloc = (nr); \
          else \
            alloc = alloc_nr(alloc); \
    +			if (alloc < __alloc__) \
    +				BUG("negative growth in ALLOC_GROW"); \
          REALLOC_ARRAY(x, alloc); \
        } \
      } while (0)

would be a reasonable compromise? It's not quite as careful as checking
each step of the computation done by alloc_nr(), but it's better than
not checking at all.

So perhaps we should do some combination of the two ;-).

>  * 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().

...meaning that ALLOC_INCR() and ALLOC_INCR_BY() would use thing##_nr? I
do like the idea of standardizing on that naming scheme, but the
thing##_nr approach is a bit magical for my taste.

Thanks,
Taylor