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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 19, 2022, 12:03 UTC
Message-ID: <221119.86sfifdu4n.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y3hFITt+8VUubC8v@nand.local>

On Fri, Nov 18 2022, Taylor Blau wrote:

Show 18 quoted lines

> On Fri, Nov 18, 2022 at 03:15:08PM -0800, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>>
>> > One thing that the commit message doesn't allude to (that is covered in
>> > the earlier discussion) is why it is important to pass
>> > `--ignore-cr-at-eol`. I think that is worth mentioning here.
>>
>> Isn't it because Git on the platform is expected to use CRLF in
>> certain places, unlike on other platforms where LF is used, but the
>> platform port hasn't adjusted tests to match that expectation?  And
>> vice versa, where Git is expected to produce LF terminated text
>> everywhere but the expected output is not "ported" to force LF
>> termination and instead produces CRLF terminated text on platforms
>> whose native line ending is CRLF?
>
> Yes, I think that's right. My suggestion to Johannes was to (a) make
> sure that your and my understanding is correct, and (b) to memorialize
> that understanding in the commit message itself.

It's not right, but mostly right. I.e. the "--ignore-cr-at-eol" is surely needed to avoid a deluge of errors on Windows, but as I noted in [1] you'll also find that the tests don't pass on a *nix box without it.

Which is apparently because "--ignore-cr-at-eol" is also conflating "is binary?" with "should I replace \r\n", or something.

Show 9 quoted lines

>> Use of "ignore-cr-at-eol" may allow such tests that are not ported
>> correctly to prepare expected output with a "wrong" line ending and
>> still pass, and I do think it may be an expedite way to make tests
>> appear to pass.
>>
>> But I worry that it may not be a good thing for the health of the
>> Windows port in the longer term.
>
> I share your concerns, too.

I don't really care about that, and would tend to defer to one or more Johannes in this thread on that question.

But as noted in [1] the upthread patch seems to be replacing some existing newline munging with not-quite-the-same "diff" behavior.

Which I think is something that needs addressing, either by the commit explaining why that's desired & OK.

Which is also a good reason to pick one of our *nix CI jobs and run it with "git diff --no-index", but without "--ignore-cr-at-eol", as I suggested. It would tease out exactly that difference, both for current and future tests.

1. https://lore.kernel.org/git/221114.86pmdplbs5.gmgdl@evledraar.gmail.com/