From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 21 Oct 2025 08:33:55 GMT
Subject: Re: [PATCH 5/9] xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash
Message-ID: <aPdFczDXrlu6CJLZ@pks.im>
In-Reply-To: <CAH=ZcbAjX=V_VvJsRzvQEA+CMM7dWQx6E5=d4FL5CD3s+ozjBg@mail.gmail.com>

On Mon, Oct 20, 2025 at 05:29:25PM -0600, Ezekiel Newren wrote:
> On Wed, Oct 15, 2025 at 3:18 PM Ezekiel Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Ezekiel Newren <ezekielnewren@gmail.com>
> >
> > The ha field is serving two different purposes, which makes the code
> > harder to read. At first glance it looks like many places assume
> > there could never be hash collisions between lines of the two input
> > files. In reality, line_hash is used together with xdl_recmatch() to
> > ensure correct comparisons of lines, even when collisions occur.
> >
> > To make this clearer, the old ha field has been split:
> >   * line_hash: The straightforward hash of a line, requiring no
> >     additional context.
> >   * minimal_perfect_hash: Not a new concept, but now a separate
> >     field. It comes from the classifier's general-purpose hash table,
> >     which assigns each line a unique and minimal hash across the two
> >     files.
> >
> > Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
> 
> I'm a bit surprised that nobody has commented on this patch. I thought
> that someone would have criticized the length of the name
> "minimal_perfect_hash" or asked me why I was splitting one field into
> two.

I actually appreciate the longer name. I'm not a fan of abbreviations
that are hard to understand myself. Sure, they are easier to type, but
in many cases they end up making the code way harder to understand if
you are not deeply familiar with it. There's of course exceptions to
this, but I don't really think that your patch falls into them.

Patrick