Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2026, 22:15 UTC
Message-ID: <xmqqik9t89yt.fsf@gitster.g>
In-Reply-To: <CAH=ZcbCX8FEs4ueU7+groQp8XhiaP0QPHMeGqT+Ap1FjeW9foQ@mail.gmail.com>

Ezekiel Newren <ezekielnewren@gmail.com> writes:

Show 13 quoted lines

>> > +             mlim1 = XDL_MIN(xdl_bogosqrt(xdf1->nrec), XDL_MAX_EQLIMIT);
>> > +             mlim2 = XDL_MIN(xdl_bogosqrt(xdf2->nrec), XDL_MAX_EQLIMIT);
>>
>> As Junio has pointed out we now evaluate xdl_bogosqrt() twice which is
>> unfortunate. It would have been nice to mention that in the commit
>> message and explain why it does not matter.
>
> It doesn't matter because xdl_bogosqrt() was being called twice before
> and is being called twice now. There is no change in that regard.
> That's why I split mlim into 2 variables to make it more clear.
>
> It looks like you and Junio have both missed that xdl_bogo_sqrt() is
> being called on different values.

I think Phillip's point is that XDL_MIN(a, b) would evaluate (a) twice.

	#define XDL_MIN(a, b) ((a) < (b) ? (a): (b))

So the code you have above

	mlim1 = XDL_MIN(xdl_bogosqrt(xdf1->nrec), XDL_MAX_EQLIMIT);

actually is

	mlim1 = ((xdl_bogosqrt(xdf1->nrec) < XDL_MAX_EQLIMIT) 
		? xdl_bogosqrt(xdf1->nrec)
		: XDL_MAX_EQLIMIT);

If you are lucky and xdf1->nrec is so large, there is only one call to xdl_bogosqrt() before mlim1 gets assigned XDL_MAX_EQLIMIT, but usually you'll call it on the same xdf1->nrec twice before you assign the result to mlim1, no?

The original lost by the patch looked like this:

 	/*
 	 * Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
 	 */
-	if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf1->nrec)) > XDL_MAX_EQLIMIT)
-		mlim = XDL_MAX_EQLIMIT;

which computed it once, assigned it to mlim, and then clamped.