Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear
- From
Ezekiel Newren <ezekielnewren@gmail.com>
- Date
- Apr 14, 2026, 21:58 UTC
- 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:
Show 48 quoted lines
>
> 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.