Re: [PATCH 03/10] xdiff: don't waste time guessing the number of lines

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 22, 2026, 10:16 UTC
Message-ID: <8f7ec565-f91c-4950-91d7-781a31d6fb6e@gmail.com>
In-Reply-To: <CAH=ZcbCbz6MB9-9Ehskk2+27GMXXewmAzRcGyN_bBi8s5Ksxjg@mail.gmail.com>

On 21/01/2026 21:12, Ezekiel Newren wrote:

Show 21 quoted lines

> On Tue, Jan 20, 2026 at 8:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 02/01/2026 18:52, Ezekiel Newren via GitGitGadget wrote:
>>> From: Ezekiel Newren <ezekielnewren@gmail.com>
>>>
>>> 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.

Show 5 quoted lines

> 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

Show 10 quoted lines

> 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."