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 [off-list ref]
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 [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

 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:

Hi folks,

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

Ack, thanks for reporting.

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

3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit
commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec
Author: Taylor Blau [off-list ref]
Date:   Tue Dec 8 17:03:14 2020 -0500

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 [off-list ref]
    Date:   Thu Feb 11 02:53:51 2021 +0100

        tests: remove most uses of C_LOCALE_OUTPUT

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

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:

Hi,

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.

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.

Thanks,
Taylor

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 [off-list ref]
    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 [off-list ref]
        Signed-off-by: Junio C Hamano [off-list ref]

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

    4f37d45706514a4b3d0259d26f719678a0cf3521 is the first bad commit
    commit 4f37d45706514a4b3d0259d26f719678a0cf3521
    Author: Jonathan Tan [off-list ref]
    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 [off-list ref]
        Signed-off-by: Junio C Hamano [off-list ref]

     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 <redacted>
---
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(-)

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index ca6339a5fb..5e30772735 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh

@@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' '
 	git -c init.defaultBranch=foo init --bare empty &&
 	test_config -C empty lsrefs.unborn advertise &&
 	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
-	git -c init.defaultBranch=up clone empty whats-up &&
+	git -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up &&
 	test refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&
 	test refs/heads/foo = $(git -C whats-up config branch.foo.merge)
 '

-- 
2.31.0.rc2.261.g7f71774620-goog

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

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

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

-Peff

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

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 [off-list ref]
    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 [off-list ref]
        Signed-off-by: Junio C Hamano [off-list ref]

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;

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d586fdc7a9..87d26bb70c 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh

@@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
-test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \
+	'index-pack --threads=N or pack.threads=N warns when no pthreads' '
 	test_must_fail git index-pack --threads=2 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

@@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh
 	grep -F "no threads support, ignoring pack.threads" err
 '
 
-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \
+	'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
 	git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

but I think this points to a failing of the FAIL_PREREQS mode. It is
generally OK to say "skip this test by pretending you do not have a
prereq satisfied" (and that is the point: to see if skipping a test
confuses later tests). But given a negated prereq here, it is not OK to
say "run this test that we usually wouldn't", because it is almost
certainly going to be mismatched with the actual build.

So I think the FAIL_PREREQS mode should probably be treating negated
prereqs differently (and always pretending that yes, we have them).

I hadn't investigated the t7810 case yet, but looking at it now, it
seems to be the exact same thing.

-Peff

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

-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \
+	'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
 	git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

but I think this points to a failing of the FAIL_PREREQS mode. It is
generally OK to say "skip this test by pretending you do not have a
prereq satisfied" (and that is the point: to see if skipping a test
confuses later tests). But given a negated prereq here, it is not OK to
say "run this test that we usually wouldn't", because it is almost
certainly going to be mismatched with the actual build.

So I think the FAIL_PREREQS mode should probably be treating negated
prereqs differently (and always pretending that yes, we have them).

I hadn't investigated the t7810 case yet, but looking at it now, it
seems to be the exact same thing.

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 <redacted>
Signed-off-by: Jeff King <redacted>
---
 t/t5300-pack-object.sh | 6 ++++--
 t/t7810-grep.sh        | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index d586fdc7a9..e830a37a38 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh

@@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '
 	test_path_is_file foo.idx
 '
 
-test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,!FAIL_PREREQS \
+	'index-pack --threads=N or pack.threads=N warns when no pthreads' '
 	test_must_fail git index-pack --threads=2 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

@@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh
 	grep -F "no threads support, ignoring pack.threads" err
 '
 
-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,!FAIL_PREREQS \
+	'pack-objects --threads=N or pack.threads=N warns when no pthreads' '
 	git pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index edfaa9a6d1..5830733f3d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh

@@ -969,7 +969,8 @@ do
 	"
 done
 
-test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' '
+test_expect_success !PTHREADS,!FAIL_PREREQS \
+	'grep --threads=N or pack.threads=N warns when no pthreads' '
 	git grep --threads=2 Hello hello_world 2>err &&
 	grep ^warning: err >warnings &&
 	test_line_count = 1 warnings &&

-- 
2.31.0.559.g509d4a088b