Re: js/drop-mingw-test-cmp, was Re: What's cooking in git.git (Dec 2022, #03; Sun, 11)

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 25, 2022, 15:44 UTC
Message-ID: <ef79236a-1c47-8775-3acb-aa23b7a300f9@kdbg.org>
In-Reply-To: <2090204b-52e9-a22f-f0c9-f812d1231863@web.de>

Am 24.12.22 um 14:31 schrieb René Scharfe:

Show 23 quoted lines

> Am 24.12.22 um 09:10 schrieb Johannes Sixt:
>> Am 21.12.22 um 14:05 schrieb Junio C Hamano:
>>> I know we have been operating under such a test environment, but
>>> after seeing the exchange between Réne and J6t, I was hoping that we
>>> do not have to keep being sloppy.
>>
>> Things did not turn out to be as simple. After ripping out all
>> special-casing of GIT_TEST_CMP from a MinGW build, I notice at least one
>> case that needs special treatment (it's `tar tf` that writes CRLF
>> output).
> 
> That would affect t6132 and perhaps t9502, right?
> 
> How can I reproduce it?  I get only LF:
> 
>    $ uname -rs
>    MINGW64_NT-10.0-22621 3.3.6-341.x86_64
> 
>    $ git archive HEAD Makefile | tar tf - | hexdump.exe -C
>    00000000  4d 61 6b 65 66 69 6c 65  0a                       |Makefile.|
>    00000009
> 
> Is there some configuration option that I need to set?

Good catch! Looks like I am wrong. In my environment I give the Windows tools precedence over the MinGW tools. There is a tar.exe in C:\Windows\System32 that writes CRLF (instead of just LF) that I was using in my test runs.

> NATIVE_CRLF seems intended to track the macro of the same name, so it
> probably makes sense to mirror config.mak.uname, but a test helper (or
> "git version --build-options" line) that returns the actual value would
> probably be more robust.

Yes, indeed, NATIVE_CRLF has nothing to do with the CRLF vs LF behavior of the toolset. We can ignore it in this discussion. Sorry for bringing it to the table.

Show 7 quoted lines

>> For the time being, I suggest to take Dscho's patch.
> 
> The patch is intended to make comparisons faster.  That works for big
> files, but the test suite compares small ones.   The total duration of
> a test suite run is about one minute longer with the patch than without
> it for me [1].  I retried with 7c2ef319c5 (The first batch for 2.40,
> 2022-12-19), and that's still the case.  Do you get different numbers?

I'm not going to measure it (one full run takes ~2 hours). I trusted Dscho's argument that this patch brings a much better improvement based on the one case that is cited in the commit message, but if that was just an extraordinary outlier, I am not sure anymore...

-- Hannes