From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 21 Jan 2026 23:55:55 GMT
Subject: Re: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()`
Message-ID: <aXFni2tE7vn1dKFp@nand.local>
In-Reply-To: <aXCTkVpjJkTabx_0@pks.im>

On Wed, Jan 21, 2026 at 09:51:36AM +0100, Patrick Steinhardt wrote:
> > diff --git a/midx-write.c b/midx-write.c
> > index 87b97c70872..6006b6569c8 100644
> > --- a/midx-write.c
> > +++ b/midx-write.c
> > @@ -1738,8 +1738,19 @@ static void fill_included_packs_batch(struct repository *r,
> >  		 */
> >  		expected_size = (uint64_t)pack_info[i].referenced_objects << 14;
> >  		expected_size /= p->num_objects;
> > -		expected_size = u64_mult(expected_size, p->pack_size);
> > -		expected_size = u64_add(expected_size, 1u << 13) >> 14;
> > +
> > +		if (unsigned_mult_overflows(expected_size,
> > +					    (uint64_t)p->pack_size))
> > +			die(_("overflow during fixed-point multiply (%"PRIu64" "
> > +			      "* %"PRIu64")"),
> > +			    expected_size, (uint64_t)p->pack_size);
> > +		expected_size = expected_size * p->pack_size;
> > +
> > +		if (unsigned_add_overflows(expected_size, 1u << 13))
> > +			die(_("overflow during fixed-point rounding (%"PRIu64" "
> > +			      " + %"PRIu64")"),
> > +			    expected_size, (uint64_t)(1ul << 13));
> > +		expected_size = (expected_size + (1u << 13)) >> 14;
>
> One downside this pattern has is that we repeat the computation, which
> makes it easy to get it wrong or forget updating either the check or the
> computation.
>
> I think ideally, we would have interfaces that combine the two
> approaches in `u64_mult()` and `unsigned_mult_overflows()`. Something
> like this for example:
>
>     static intline bool u64_mult(uint64_t a, uint64_t b, uint64_t *out)
>     {
>         if (unsigned_mult_overflows(a, b))
>             return false;
>         *out = a * b;
>         return true;
>     }

I had considered this approach when writing, but ultimately decided
against it, since it felt a little clunky to have to pass a pointer in
to do a simple arithmetic operation. But I think your point about
ensuring that we actually do:

    if (unsigned_mult_overflows(a, b))
      die(...);
    result = a * b;

and not "result = a * c" or some other expression which is not "a * b"
is a good one.

I dunno. The spots in this patch are the only uses of u64_mult() and
u64_add(), so I'm hesitant to keep a helper function around just for
that sole use-case. I wonder if we should do what you suggest here for
the much more frequently used st_add() / st_mult() / st_sub() functions?

> This would let the caller handle the failure and is thus quite flexible,
> which results in the following code:
>
> 	if (!u64_mult(expected_size, (uint64_t)p->pack_size, &expected_size))
> 		die(_("overflow during fixed-point multiply (%"PRIu64" "
> 		      "* %"PRIu64")"), expected_size, (uint64_t)p->pack_size);

It does read quite cleanly, so I think I'm convinced.

Thanks,
Taylor