Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION

Hi folks,

Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and next) seem to result in some failures:

  Test Summary Report
 -------------------
  t5300-pack-object.sh (Wstat: 256 Tests: 46 Failed: 2)
  Failed tests: 35-36
  Non-zero exit status: 1
  t7810-grep.sh (Wstat: 256 Tests: 229 Failed: 1)
  Failed test: 160
  Non-zero exit status: 1
  Files=924, Tests=22400, 422 wallclock secs (12.57 usr 2.52 sys +
601.02 cusr 1047.02 csys = 1663.13 CPU)
  Result: FAIL

A quick git-bisect run seems to point back to this commit:

3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit
commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec
Author: Taylor Blau <me@ttaylorr.com>
Date:   Tue Dec 8 17:03:14 2020 -0500

    ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW()

    'ewah/ewah_bitmap.c:buffer_grow()' is responsible for growing the buffer
    used to store the bits of an EWAH bitmap. It is essentially doing the
    same task as the 'ALLOC_GROW()' macro, so use that instead.

    This simplifies the callers of 'buffer_grow()', who no longer have to
    ask for a specific size, but rather specify how much of the buffer they
    need. They also no longer need to guard 'buffer_grow()' behind an if
    statement, since 'ALLOC_GROW()' (and, by extension, 'buffer_grow()') is
    a noop if the buffer is already large enough.

    But, the most significant change is that this fixes a bug when calling
    buffer_grow() with both 'alloc_size' and 'new_size' set to 1. In this
    case, truncating integer math will leave the new size set to 1, causing
    the buffer to never grow.

    Instead, let alloc_nr() handle this, which asks for '(new_size + 16) * 3
    / 2' instead of 'new_size * 3 / 2'.

    Signed-off-by: Taylor Blau <me@ttaylorr.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

 ewah/ewah_bitmap.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

I also found that a test is failing with protocol V1 set (GIT_TEST_PROTOCOL_VERSION=1)

  Test Summary Report
  -------------------
  t5606-clone-options.sh (Wstat: 256 Tests: 15 Failed: 1)
  Failed test: 14
  Non-zero exit status: 1
  Files=924, Tests=22852, 568 wallclock secs (12.69 usr 2.73 sys +
842.87 cusr 1322.91 csys = 2181.20 CPU)
  Result: FAIL

Which git-bisect is telling me that was caused by the same commit.

Regards, Son Luong.

Hi,

On Tue, Mar 16, 2021 at 10:45:20AM +0100, Son Luong Ngoc wrote:

Ack, thanks for reporting.

Hmm. I get a different result.

    $ cat run.sh
    #!/bin/sh
    make -j20 DEVELOPER=1 O=0 && cd t && GIT_TEST_FAIL_PREREQS=1 ./t5300-*.sh -i
    $ git bisect start
    $ git bisect bad
    $ git bisect good v2.30.0
    Bisecting: 453 revisions left to test after this (roughly 9 steps)
    $ git bisect run $PWD/run.sh
    [snip]
    a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit
    commit a926c4b904bdc339568c2898af955cdc61b31542
    Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Date:   Thu Feb 11 02:53:51 2021 +0100

        tests: remove most uses of C_LOCALE_OUTPUT

I'll take a look at what's going on here, but Ævar (cc'd) will probably beat me to it.

Is it possible that your bisection script doesn't report success properly? Bisecting the same range (v2.30.0..v2.31.0) with

    $ cat run.sh
    #!/bin/sh
    false

does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW(), 2020-12-08) is the first bad commit.

Thanks, Taylor

Hi Taylor,

On Tue, Mar 16, 2021 at 09:52:47AM -0400, Taylor Blau wrote:

You are spot on. It was a busy day and I only had a few minutes to look at our internal pipeline of the test suite. I guess I was doing something along the line of.

      $ git bisect start HEAD v2.30.0
      $ git bisect run 'cd t && GIT_TEST_PROTOCOL_VERSION=1 ./t5606-clone-options.sh'

Which does indeed errored out and pointed to your commit.

I have properly re-run the bisection in a './test.sh' bash script and here are the suspicious commits:

1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1:

    a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit
    commit a926c4b904bdc339568c2898af955cdc61b31542
    Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Date:   Thu Feb 11 02:53:51 2021 +0100

        tests: remove most uses of C_LOCALE_OUTPUT

        As a follow-up to d162b25f956 (tests: remove support for
        GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now
        always true C_LOCALE_OUTPUT prerequisite from those tests which
        declare it as an argument to test_expect_{success,failure}.

        Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

2. For failing t5606 while 'GIT_TEST_PROTOCOL_VERSION=1' was used:

    4f37d45706514a4b3d0259d26f719678a0cf3521 is the first bad commit
    commit 4f37d45706514a4b3d0259d26f719678a0cf3521
    Author: Jonathan Tan <jonathantanmy@google.com>
    Date:   Fri Feb 5 12:48:49 2021 -0800

        clone: respect remote unborn HEAD

        Teach Git to use the "unborn" feature introduced in a previous patch as
        follows: Git will always send the "unborn" argument if it is supported
        by the server. During "git clone", if cloning an empty repository, Git
        will use the new information to determine the local branch to create. In
        all other cases, Git will ignore it.

        Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

     Documentation/config/init.txt |  2 +-
     builtin/clone.c               | 16 ++++++++++++++--
     connect.c                     | 28 ++++++++++++++++++++++++++--
     t/t5606-clone-options.sh      |  8 +++++---
     t/t5702-protocol-v2.sh        | 25 +++++++++++++++++++++++++
     transport.h                   |  8 ++++++++
     6 files changed, 79 insertions(+), 8 deletions(-)

Thanks, Son Luong.

4f37d45706 ("clone: respect remote unborn HEAD", 2021-02-05) introduces a new feature (if the remote has an unborn HEAD, e.g. when the remote repository is empty, use it as the name of the branch) that only works in protocol v2, but did not ensure that one of its tests always uses protocol v2, and thus that test would fail if GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add "-c protocol.version=2" to the appropriate test.

(The rest of the tests from that commit have "-c protocol.version=2" already added.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks, Son Luong, for noticing this. Here's a fix for the
GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also
applies cleanly on master.

 t/t5606-clone-options.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Wed, Mar 17, 2021 at 08:42:00AM -0700, Jonathan Tan wrote:

Thanks, this looks like the obvious and correct fix (and clearly makes the test pass ;) ).

-Peff

Jonathan Tan <jonathantanmy@google.com> writes:

Makes sense. And I do not see need for any other changes, like test_expect_failure with protocol 0 (or 1).

Will queue as a candidate for maint-2.31.

Thanks.

On Wed, Mar 17, 2021 at 02:38:18PM +0100, Son Luong Ngoc wrote:

I looked at the one in t5300, and I don't think it _ever_ worked, nor can it be made to work in this mode.

It is expecting that running:

  git index-pack --threads=2

will issue a warning in a build without pthread support. But in the fake "pretend the pthread prereq is not satisfied" mode, it will of course not do that, because the build itself is not aware that it's supposed to be pretending that pthreads aren't supported!

Before the patch mentioned above, its prereqs were:

  !PTHREADS,C_LOCALE_OUTPUT

which would never be satisfied under the "pretend" mode, because it would _also_ disable C_LOCALE_OUTPUT, and we'd always skip it.

So I think something like this creates a similar situation;

On Wed, Mar 17, 2021 at 01:54:25PM -0400, Jeff King wrote:

It looks like the problem is indeed somewhat widespread, and there is a magic prereq already to skip such tests.

I do still think that this is a fundamental failing of the FAIL_PREREQS mode, but it probably makes sense to annotate these tests in the meantime (I don't plan on looking further into it myself).

Another rough edge I noticed: if you set GIT_TEST_HTTPD or GIT_TEST_GIT_DAEMON to "yes" in your config.mak, these play quite badly with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so refuse to start httpd, and then complain that the setup fails (and the point of "yes" for those values is to loudly complain when setup fails, rather than quietly skipping the tests).

-- >8 --
Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS

Some tests in t5300 and t7810 expect us to complain about a "--threads" argument when Git is compiled without pthread support. Running these under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to the tests that there is no pthread support, so they expect the warning, but of course the actual build is perfectly happy to respect the --threads argument.

We never noticed before the recent a926c4b904 (tests: remove most uses of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never have run in FAIL_PREREQS mode, since it would always pretend that the locale prereq was not satisfied.

These tests can't possibly work in this mode; it is a mismatch between what the tests expect and what the build was told to do. So let's just mark them to be skipped, using the special prereq introduced by dfe1a17df9 (tests: add a special setup where prerequisites fail, 2019-05-13).

Reported-by: Son Luong Ngoc <sluongng@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5300-pack-object.sh | 6 ++++--
 t/t7810-grep.sh        | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

Jeff King <peff@peff.net> writes:

The README file in t/ directory claims that this "is useful for discovering issues with the tests where say a later test implicitly depends on an optional earlier test." but apparently it does not work well with these negated prerequisites. Its implementation probably should force a safe bypass of the whole test_have_prereq() etc. done in test_skip by hooking into test_verify_prereq and overwrite any non-empty test_prereq with a single hardcoded PRETEND_FAIL_PREREQ prerequisite that is never satisfied, or something.

... and I think this would also be gone, as the NOT_ROOT test is done with test_have_prereq that we wouldn't be mucking with if we limit the FAIL_PREREQS only to tweak the test_expect_* prereqs.

In short, the biggest mistake in the current FAIL_PREREQS design is to hook into test_have_prereq while the stated objective only needs to futz with the prerequisite given to the test_expect_* functions, I would think.

On Thu, Mar 18, 2021 at 02:17:19PM -0700, Junio C Hamano wrote:

Yeah, that matches my intuition of the problem, too.

-Peff