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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 14, 2022, 13:47 UTC
Message-ID: <998821n6-6sp3-30q9-95s6-p7rp5o8nr5rr@tzk.qr>
In-Reply-To: <8ec8f3f5-0827-25a3-464d-638378b65ddb@dunelm.org.uk>

Hi Phillip,

On Mon, 14 Nov 2022, Phillip Wood wrote:

Show 13 quoted lines

> On 12/11/2022 22:07, 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.
>
> This makes me wonder about the implications for our diff tests. We have
> ~200 calls to `test_cmp` in t*-diff-*.sh. I had a look at
> t4053-diff-no-index.sh and non of the tests that use `test_cmp` look critical
> for functionality that is used by `test_cmp` and there are tests for the exit
> code. I suspect that if `diff --no-index` breaks we'll end up with confusing
> test failures rather than misleading passes.

I was surprised how little of a dent the overhead makes (it was not even measurable within the noise) when I tested this a couple of weeks ago (but then ran out of time and motivation to send a new iteration).

It would probably still be better to use less complex code, but Junio made his preference abundantly clear, and if we wanted to save on CI time, there are much bigger construction sites that we have to open. Point in case: our CI runtime goes up, up, up, with no real restraint going on. A randomly picked run from 6 weeks ago took 7h14m of build time (https://github.com/git/git/actions/runs/3175983767/usage), the current `seen` run took 8h48m (https://github.com/git/git/actions/runs/3448435246/usage). If there is considerate use of resources going on then please help me because I cannot see it.

> A side effect of this change is that on windows `test_cmp` will now except
> directories as well as files but I don't think that matters.

Indeed, I would believe that the non-Windows test jobs would catch that failure, so we do not need any extra code to check specifically for that.

Show 12 quoted lines

> > Note: Earlier attempts at fixing this involved 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, but that
> > attempt saw a lot of backlash and distractions during review and was
> > therefore abandoned.
>
> Hopefully most of the files we feed into `test_cmp` are small enough that the
> absolute difference in run-time is not too big. There is an optimization for
> -U0 that trims the common tail from the files before calling xdl_diff() but
> that does not help here because we need to use --ignore-cr-at-eol (otherwise
> `git diff --no-index -U0 || git diff --no-index` might speed up the common
> case of matching files).

There is similar code in xdiff to "shrink the box" to avoid unnecessary work, i.e. skipping identical lines at both ends of the line range: https://github.com/git/git/blob/v2.38.1/xdiff/xdiffi.c#L261-L265

I share your concern, but my current concern is to cut the loss of time spent on this patch series on my part, and that concern is currently even bigger.

Ciao, Dscho