On Thu, Aug 07, 2025 at 09:38:46AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
