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

From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Mar 31, 2026, 01:29 UTC
Message-ID: <CAH=ZcbA_1pZYDjg0Q7bEB11vY8-T76o-r-v9g--NUSwbfZigsQ@mail.gmail.com>
In-Reply-To: <xmqqtstxdr6v.fsf@gitster.g>

On Mon, Mar 30, 2026 at 1:59 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

>
> Ezekiel Newren <ezekielnewren@gmail.com> writes:
>
> > On Fri, Mar 27, 2026 at 5:01 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> Updated code, when nm is not zero, does something different.  if
> >> need_min is true, mlim1 is set to -1 and presumably nm is a count or
> >> length that is bounded on its lower end with 0, so it is larger than
> >> mlim1 (== -1), and we always take INVESTIGATE and never KEEP.
> >>
> >> So the rewritten code is broken when need_min is true?
> >>
> >> I suspect the remainder of the patch is broken exactly the same way,
> >> so the remedy would be similar?
> >
> > Your assessment is correct, PTRDIFF_MAX should be used instead of
> > SIZE_MAX. I realized my mistake a few hours after I pushed. This will
> > be fixed in the next version.
>
> Yeah, using PTRDIFF_MAX is fine.  When I reported the breakage I was
> hinting that everything may want to become unsigned, but since the
> original does use signed quantities and variables, it is far safer
> to stick to signed arithmetic---until a full audit says it is safe
> to switch to size_t of course.

I would prefer to make everything size_t, but dend can be negative if the number of lines in a file is 0 and that breaks the current code if unsigned is forced. I can cleanup the code to use unsigned, but I didn't want to distract from the readability, of this patch series, of xdl_cleanup_records() with other refactorings.

In fact dstart is never negative, but I thought that it would be more confusing to change dstart to unsigned and keep dend signed and explain why there is a discrepancy in types between the 2.