Re: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()`

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 14, 2026, 22:03 UTC
Message-ID: <aWgSzI30k0BZfZ4Q@nand.local>
In-Reply-To: <xmqqpl7beugj.fsf@gitster.g>

On Wed, Jan 14, 2026 at 01:49:00PM -0800, Junio C Hamano wrote:

Show 42 quoted lines

> Taylor Blau <me@ttaylorr.com> 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 <me@ttaylorr.com>
> > ---
> >  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