From: Phillip Wood Date: Thu, 22 Jan 2026 10:16:02 GMT Subject: Re: [PATCH 03/10] xdiff: don't waste time guessing the number of lines Message-ID: <8f7ec565-f91c-4950-91d7-781a31d6fb6e@gmail.com> In-Reply-To: On 21/01/2026 21:12, Ezekiel Newren wrote: > On Tue, Jan 20, 2026 at 8:02 AM Phillip Wood wrote: >> >> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote: >>> From: Ezekiel Newren >>> >>> All lines must be read anyway, so classify them after they're read in. >>> Also move the memset() into xdl_init_classifier(). >> >> So instead of looping over the input lines one and a bit times (the bit >> being from xdl_guess_lines) we now loop over them twice as we split them >> first and then classify them in a separate loop. It does save some work >> not to call xdl_guess_lines but it is unclear if that offsets >> classifying them in a separate loop. >> >>> + for (size_t i = 0; i < xe->xdf1.nrec; i++) { >>> + xrecord_t *rec = &xe->xdf1.recs[i]; >>> + xdl_classify_record(1, &cf, rec); >> >> We seem to have lost the error handling if xdl_classify_record() fails. > > The error handling was not "lost" it was deliberately removed. That's the sort of thing that needs to be explained in the commit message. > The > only way in which xdl_classify_record() could fail is by a failed > memory allocation. On the Rust side this would result in a panic > (panic means something different in Rust vs C) in which case C could > not possibly recover. There is no rust code in xdiff at the moment so we don't panic on failure. In git we'll die() because xdl_malloc() and friends are defined as xmalloc() etc. which die on allocation failure. However anyone else picking up this code and using a different allocator that does not die on allocation failure will expect the error to be propagated. If you want to stop supporting other allocators then you should propose a patch to do so, not silently slip the change into this patch. Thanks Phillip > Also for operations like Vec.push() in Rust it's > assumed that memory management functions will never fail and if they > do they crash the program with no chance of recovery (unless you > account for panic unwinding which is really ugly). It seems a lot of > arguments about ivec and my xdiff cleanups are "We don't do things > this way in Git/C" I'm aware of many of these arguments and I'm trying > to address them with a more specific answer of "Yes, but that's not > how things are done in Rust and all of this is to prepare the code for > conversion to Rust and some things shouldn't, or even, cannot be done > the C way in Rust."