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

[Taylor Blau <me@ttaylorr.com>]

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.

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

Thanks,
Taylor

[1]: https://lore.kernel.org/git/Y3B36HjDJhIY5jNz@nand.local/