Re: [PATCH v4 2/2] tests(mingw): avoid very slow `mingw_test_cmp`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 18, 2022, 13:32 UTC
Message-ID: <qp0556o2-6q18-8556-0n8s-p28831qr6qs6@tzk.qr>
In-Reply-To: <Y3LD1CtKBoDhPdSZ@nand.local>

Hi Taylor,

On Mon, 14 Nov 2022, Taylor Blau wrote:

Show 29 quoted lines

> On Mon, Nov 14, 2022 at 02:06:52PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It is more performant to run `git diff --no-index` than running the
> > `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for
> > Windows uses. And a lot more readable.
> >
> > The original reason why Git's test suite needs the `mingw_test_cmp`
> > function at all (and why `cmp` is not good enough) is that Git's test
> > suite is not actually trying to compare binary files when it calls
> > `test_cmp`, but it compares text files. And those text files can contain
> > CR/LF line endings depending on the circumstances.
> >
> > Note: The original fix in the Git for Windows project implemented a test
> > helper that avoids the overhead of the diff machinery, in favor of
> > implementing a behavior that is more in line with what `mingw_test_cmp`
> > does now. This was done to minimize the risk in using something as
> > complex as the diff machinery to perform something as simple as
> > determining whether text output is identical to the expected output or
> > not. This approach has served Git for Windows well for years, but the
> > attempt to upstream this saw a lot of backlash and distractions during
> > the review, was disliked by the Git maintainer and was therefore
> > abandoned. For full details, see the thread at
> > https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t
>
> In the previous round, you wrote that this paragraph:
>
>     It explains why we replace a relatively simple logic with a relatively
>     complex one.

Yes. And I obviously got myself misunderstood.

The simple logic I refer to is the `t/helper/test-cmp.c` that I abandoned.

The complex logic is the diff machinery we now call, which is a lot more involved and goes through a lot more code paths.

> But I'm not sure the rewritten version does what you claim, at least in
> my own personal opinion.
>
> It is not helpful to say the original approach "saw a lot of backlash".

It is the nicest thing I can say about it.

And I want people who are curious what happened to Git for Windows' custom code to have an explanation in the commit messages.

Show 6 quoted lines

> It is helpful, on the other hand, to say what about the original
> approach gave reviewers pause, and why that alternative approach isn't
> pursued here.
>
> Maybe I'm splitting hairs here, but I really do stand by my original
> suggestion from back in [1].

We can also keep hitting a dead horse, but I don't think that will make anything any better.

Thanks, Johannes

Show 6 quoted lines

>
> Thanks,
> Taylor
>
> [1]: https://lore.kernel.org/git/Y3B36HjDJhIY5jNz@nand.local/
>