From: Ezekiel Newren Date: Tue, 31 Mar 2026 01:29:57 GMT Subject: Re: [PATCH v3 4/6] xdiff/xdl_cleanup_records: make limits more clear Message-ID: In-Reply-To: On Mon, Mar 30, 2026 at 1:59 PM Junio C Hamano wrote: > > Ezekiel Newren writes: > > > On Fri, Mar 27, 2026 at 5:01 PM Junio C Hamano 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.