From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Tue, 14 Apr 2026 21:58:03 GMT
Subject: Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear
Message-ID: <CAH=ZcbCX8FEs4ueU7+groQp8XhiaP0QPHMeGqT+Ap1FjeW9foQ@mail.gmail.com>
In-Reply-To: <32c34d0d-9358-43e3-9d58-5999b3ffd6c2@gmail.com>

On Tue, Mar 31, 2026 at 3:44 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ezekiel
>
> On 30/03/2026 18:00, Ezekiel Newren via GitGitGadget wrote:
> > From: Ezekiel Newren <ezekielnewren@gmail.com>
> >
> > Make the handling of per-file limits and the minimal-case clearer.
> >    * Use explicit per-file limit variables (mlim1, mlim2) and initialize
> >      them.
> >    * The additional condition `!need_min` is redudant now, remove it.
> > Best viewed with --color-words.
> >
> > Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> > ---
> >   xdiff/xprepare.c | 19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> > index 386668a92d..bd8baf214d 100644
> > --- a/xdiff/xprepare.c
> > +++ b/xdiff/xprepare.c
> > @@ -268,7 +268,7 @@ static bool xdl_clean_mmatch(uint8_t const *action, ptrdiff_t i, ptrdiff_t s, pt
> >    * might be potentially discarded if they appear in a run of discardable.
> >    */
> >   static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {
> > -     ptrdiff_t i, nm, mlim;
> > +     ptrdiff_t i, nm, mlim1, mlim2;
> >       xdlclass_t *rcrec;
> >       uint8_t *action1 = NULL, *action2 = NULL;
> >       bool need_min = !!(cf->flags & XDF_NEED_MINIMAL);
> > @@ -287,25 +287,30 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd
> >               goto cleanup;
> >       }
> >
> > +     if (need_min) {
> > +             /* i.e. infinity */
> > +             mlim1 = PTRDIFF_MAX;
> > +             mlim2 = PTRDIFF_MAX;
>
> This is a nice improvement as it simplifies the checks below
>
> > +     } else {
> > +             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.