From: Ezekiel Newren <ezekielnewren@gmail.com>
Date: Wed, 21 Jan 2026 21:05:42 GMT
Subject: Re: [PATCH 02/10] xdiff: make classic diff explicit by creating xdl_do_classic_diff()
Message-ID: <CAH=ZcbAb-iQM81Sd79KtFV0nf1gv4gfnBBvJ2AvcxCTN9xOr7Q@mail.gmail.com>
In-Reply-To: <1c46f551-0040-481e-9476-bc1b85f92636@gmail.com>

On Tue, Jan 20, 2026 at 8:01 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>
> >
> > Later patches will prepare xdl_cleanup_records() to be moved into xdiffi.c
> > since only the classic diff uses that function.
>
> I assume that's to make it easier to covert the myers implementation to
> rust without affecting the rest of the code? If so it would be nice to
> say that.

Making it easier to port to Rust is a side effect. The primary goal is
to simplify the job of xprepare to only parsing and hashing lines in a
file. xdl_cleanup_records() is only used by classic diff
(myers/minimal) which means it doesn't belong in xprepare because it's
part of a diff algorithm and isn't relevant to preparing the file for
a diff algorithm. Perhaps xdl_trim_ends() should be moved into
xdl_do_diff() too...

> > Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com>
>
> > +int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
> > +             xdfenv_t *xe) {
> > +     int res;
> > +
> > +     if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
> > +             return -1;
> > +
> > +     if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
> > +             res = xdl_do_patience_diff(xpp, xe);
> > +             goto out;
> > +     }
> > +
> > +     if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
> > +             res = xdl_do_histogram_diff(xpp, xe);
> > +             goto out;
> > +     }
> > +
> > +     res = xdl_do_classic_diff(xe, xpp->flags);
>
> This might be clearer that we're calling only one of the three functions
> if we wrote this as
>
>         if (XDF_DIFF_ALG(xpp->flags) == XDIF_PATIENCE_DIFF)
>                 res = xdl_do_patience_diff(xpp, xe);
>         else if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
>                 res = xdl_do_histogram_diff(xpp, xe);
>         else
>                 res = xdl_do_classic_diff(xe, xpp->flags);
>
> and then we can drop the out: label

In a later cleanup, I make this exact change :)