From: Taylor Blau Date: Wed, 14 Jan 2026 22:03:56 GMT Subject: Re: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()` Message-ID: In-Reply-To: On Wed, Jan 14, 2026 at 01:49:00PM -0800, Junio C Hamano wrote: > Taylor Blau writes: > > > A future commit will want to add two 32-bit unsigned values together > > while checking for overflow. Introduce a variant of the u64_add() > > function for operating on 32-bit inputs. > > > > Signed-off-by: Taylor Blau > > --- > > git-compat-util.h | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index b0673d1a450..db62a6f25c5 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -641,6 +641,14 @@ static inline int cast_size_t_to_int(size_t a) > > return (int)a; > > } > > > > +static inline uint32_t u32_add(uint32_t a, uint32_t b) > > +{ > > + if (unsigned_add_overflows(a, b)) > > + die("uint32_t overflow: %"PRIuMAX" + %"PRIuMAX, > > + (uintmax_t)a, (uintmax_t)b); > > + return a + b; > > +} > > Neither this one, nor the original u64_add(), seem to me a > particularly good API. > > When things might overflow, it is a given that we should give a > controlled death rather than nonsense behaviour and/or corrupt > output, but shouldn't the diagnosis message given to the end-user > when we find an overflow be given at a bit higher layer? At this > level, you do not even know what quantities you are adding together > *means*. Even though your caller might be able to give a more > intelligible message like "the number of packs to combine exceeds > 2^32 that is too many". But dying in these functions means the > callers have no chance to do so. The only thing the user sees is > some code tried to add two u32 and overflowed---without any hint > what these quantities were or what the addition was trying to > compute. I agree with your sentiment here. Let's avoid proliferating an API pattern that encourages non-descriptive error message by dropping the sole caller of u32_add() in this series with the following: --- 8< --- diff --git a/midx-write.c b/midx-write.c index afa077a09cc..99e7116f4d0 100644 --- a/midx-write.c +++ b/midx-write.c @@ -990,7 +990,10 @@ static uint32_t compactible_packs_between(const struct multi_pack_index *from, ASSERT(from && to); - nr = u32_add(to->num_packs, to->num_packs_in_base); + if (unsigned_add_overflows(to->num_packs, to->num_packs_in_base)) + die(_("too many packs, unable to compact")); + + nr = to->num_packs + to->num_packs_in_base; if (nr < from->num_packs_in_base) BUG("unexpected number of packs in base during compaction: " "%"PRIu32" < %"PRIu32, nr, from->num_packs_in_base); --- >8 --- As for removing u64_add(), that should be straightforward as well since there is also a single caller. Let me know if you think that makes sense to take up as part of this series, or if you would prefer it done separately. I tend to prefer the latter, since the state after applying the above is that we avoid adding any new callers. Removing st_add() and st_mult(), on the other hand, will be much more involved since they are extremely widely used by comparison. Thanks, Taylor