{"thread":{"id":"55330","subject":"Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION","startedAt":"2021-03-16T09:46:27Z","lastAt":"2021-03-18T21:54:11Z","messageCount":10,"participants":["Son Luong Ngoc","Taylor Blau","Jonathan Tan","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"419293","messageId":"CAL3xRKfSXDd0ucO4zaM5_WZeQfq10Hqpyk3nL+Zw8ttgfN0ZhA@mail.gmail.com","threadId":"55330","inReplyTo":null,"subject":"Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2021-03-16T09:45:20Z","receivedAt":"2021-03-16T09:46:27Z","isPatch":false,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi folks,\n\nRunning the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and\nnext) seem to result in some failures:\n\n  Test Summary Report\n -------------------\n  t5300-pack-object.sh (Wstat: 256 Tests: 46 Failed: 2)\n  Failed tests: 35-36\n  Non-zero exit status: 1\n  t7810-grep.sh (Wstat: 256 Tests: 229 Failed: 1)\n  Failed test: 160\n  Non-zero exit status: 1\n  Files=924, Tests=22400, 422 wallclock secs (12.57 usr 2.52 sys +\n601.02 cusr 1047.02 csys = 1663.13 CPU)\n  Result: FAIL\n\nA quick git-bisect run seems to point back to this commit:\n\n3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit\ncommit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec\nAuthor: Taylor Blau <me@ttaylorr.com>\nDate:   Tue Dec 8 17:03:14 2020 -0500\n\n    ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW()\n\n    'ewah/ewah_bitmap.c:buffer_grow()' is responsible for growing the buffer\n    used to store the bits of an EWAH bitmap. It is essentially doing the\n    same task as the 'ALLOC_GROW()' macro, so use that instead.\n\n    This simplifies the callers of 'buffer_grow()', who no longer have to\n    ask for a specific size, but rather specify how much of the buffer they\n    need. They also no longer need to guard 'buffer_grow()' behind an if\n    statement, since 'ALLOC_GROW()' (and, by extension, 'buffer_grow()') is\n    a noop if the buffer is already large enough.\n\n    But, the most significant change is that this fixes a bug when calling\n    buffer_grow() with both 'alloc_size' and 'new_size' set to 1. In this\n    case, truncating integer math will leave the new size set to 1, causing\n    the buffer to never grow.\n\n    Instead, let alloc_nr() handle this, which asks for '(new_size + 16) * 3\n    / 2' instead of 'new_size * 3 / 2'.\n\n    Signed-off-by: Taylor Blau <me@ttaylorr.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n ewah/ewah_bitmap.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\nI also found that a test is failing with protocol V1 set\n(GIT_TEST_PROTOCOL_VERSION=1)\n\n  Test Summary Report\n  -------------------\n  t5606-clone-options.sh (Wstat: 256 Tests: 15 Failed: 1)\n  Failed test: 14\n  Non-zero exit status: 1\n  Files=924, Tests=22852, 568 wallclock secs (12.69 usr 2.73 sys +\n842.87 cusr 1322.91 csys = 2181.20 CPU)\n  Result: FAIL\n\nWhich git-bisect is telling me that was caused by the same commit.\n\nRegards,\nSon Luong.\n"},{"id":"419300","messageId":"YFC33vfLb36pRCO6@nand.local","threadId":"55330","inReplyTo":"CAL3xRKfSXDd0ucO4zaM5_WZeQfq10Hqpyk3nL+Zw8ttgfN0ZhA@mail.gmail.com","subject":"Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-03-16T13:52:47Z","receivedAt":"2021-03-16T13:53:42Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nOn Tue, Mar 16, 2021 at 10:45:20AM +0100, Son Luong Ngoc wrote:\n> Hi folks,\n>\n> Running the test suit with GIT_TEST_FAIL_PREREQS=1 on master (and\n> next) seem to result in some failures:\n\nAck, thanks for reporting.\n\n> A quick git-bisect run seems to point back to this commit:\n>\n> 3b1ca60f8f317b483c8c1805ab500ff2b014cbec is the first bad commit\n> commit 3b1ca60f8f317b483c8c1805ab500ff2b014cbec\n> Author: Taylor Blau <me@ttaylorr.com>\n> Date:   Tue Dec 8 17:03:14 2020 -0500\n\nHmm. I get a different result.\n\n    $ cat run.sh\n    #!/bin/sh\n    make -j20 DEVELOPER=1 O=0 && cd t && GIT_TEST_FAIL_PREREQS=1 ./t5300-*.sh -i\n    $ git bisect start\n    $ git bisect bad\n    $ git bisect good v2.30.0\n    Bisecting: 453 revisions left to test after this (roughly 9 steps)\n    $ git bisect run $PWD/run.sh\n    [snip]\n    a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit\n    commit a926c4b904bdc339568c2898af955cdc61b31542\n    Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    Date:   Thu Feb 11 02:53:51 2021 +0100\n\n        tests: remove most uses of C_LOCALE_OUTPUT\n\n>     ewah/ewah_bitmap.c: avoid open-coding ALLOC_GROW()\n\nI'll take a look at what's going on here, but Ævar (cc'd) will probably\nbeat me to it.\n\nIs it possible that your bisection script doesn't report success\nproperly? Bisecting the same range (v2.30.0..v2.31.0) with\n\n    $ cat run.sh\n    #!/bin/sh\n    false\n\ndoes say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding\nALLOC_GROW(), 2020-12-08) is the first bad commit.\n\nThanks,\nTaylor\n"},{"id":"419468","messageId":"YFIGSo3U5u7zy9fq@C02YX140LVDN.corpad.adbkng.com","threadId":"55330","inReplyTo":"YFC33vfLb36pRCO6@nand.local","subject":"Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2021-03-17T13:38:18Z","receivedAt":"2021-03-17T13:39:03Z","isPatch":false,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Taylor,\n\nOn Tue, Mar 16, 2021 at 09:52:47AM -0400, Taylor Blau wrote:\n> Hi,\n> \n> Is it possible that your bisection script doesn't report success\n> properly? Bisecting the same range (v2.30.0..v2.31.0) with\n> \n>     $ cat run.sh\n>     #!/bin/sh\n>     false\n> \n> does say that my 3b1ca60f8f (ewah/ewah_bitmap.c: avoid open-coding\n> ALLOC_GROW(), 2020-12-08) is the first bad commit.\n\nYou are spot on.  It was a busy day and I only had a few minutes to\nlook at our internal pipeline of the test suite.  I guess I was doing\nsomething along the line of.\n\n      $ git bisect start HEAD v2.30.0\n      $ git bisect run 'cd t && GIT_TEST_PROTOCOL_VERSION=1 ./t5606-clone-options.sh'\n\nWhich does indeed errored out and pointed to your commit.\n\n> \n> Thanks,\n> Taylor\n\nI have properly re-run the bisection in a './test.sh' bash script and\nhere are the suspicious commits:\n\n1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1:\n\n    a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit\n    commit a926c4b904bdc339568c2898af955cdc61b31542\n    Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    Date:   Thu Feb 11 02:53:51 2021 +0100\n\n        tests: remove most uses of C_LOCALE_OUTPUT\n\n        As a follow-up to d162b25f956 (tests: remove support for\n        GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now\n        always true C_LOCALE_OUTPUT prerequisite from those tests which\n        declare it as an argument to test_expect_{success,failure}.\n\n        Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n        Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n2. For failing t5606 while 'GIT_TEST_PROTOCOL_VERSION=1' was used:\n\n    4f37d45706514a4b3d0259d26f719678a0cf3521 is the first bad commit\n    commit 4f37d45706514a4b3d0259d26f719678a0cf3521\n    Author: Jonathan Tan <jonathantanmy@google.com>\n    Date:   Fri Feb 5 12:48:49 2021 -0800\n\n        clone: respect remote unborn HEAD\n\n        Teach Git to use the \"unborn\" feature introduced in a previous patch as\n        follows: Git will always send the \"unborn\" argument if it is supported\n        by the server. During \"git clone\", if cloning an empty repository, Git\n        will use the new information to determine the local branch to create. In\n        all other cases, Git will ignore it.\n\n        Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n        Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n     Documentation/config/init.txt |  2 +-\n     builtin/clone.c               | 16 ++++++++++++++--\n     connect.c                     | 28 ++++++++++++++++++++++++++--\n     t/t5606-clone-options.sh      |  8 +++++---\n     t/t5702-protocol-v2.sh        | 25 +++++++++++++++++++++++++\n     transport.h                   |  8 ++++++++\n     6 files changed, 79 insertions(+), 8 deletions(-)\n\n\nThanks,\nSon Luong.\n"},{"id":"419474","messageId":"20210317154200.2656837-1-jonathantanmy@google.com","threadId":"55330","inReplyTo":"YFIGSo3U5u7zy9fq@C02YX140LVDN.corpad.adbkng.com","subject":"[PATCH] t5606: run clone branch name test with protocol v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-03-17T15:42:00Z","receivedAt":"2021-03-17T15:50:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"4f37d45706 (\"clone: respect remote unborn HEAD\", 2021-02-05) introduces\na new feature (if the remote has an unborn HEAD, e.g. when the remote\nrepository is empty, use it as the name of the branch) that only works\nin protocol v2, but did not ensure that one of its tests always uses\nprotocol v2, and thus that test would fail if\nGIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add \"-c\nprotocol.version=2\" to the appropriate test.\n\n(The rest of the tests from that commit have \"-c protocol.version=2\"\nalready added.)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks, Son Luong, for noticing this. Here's a fix for the\nGIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also\napplies cleanly on master.\n\n t/t5606-clone-options.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex ca6339a5fb..5e30772735 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' '\n \tgit -c init.defaultBranch=foo init --bare empty &&\n \ttest_config -C empty lsrefs.unborn advertise &&\n \tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n-\tgit -c init.defaultBranch=up clone empty whats-up &&\n+\tgit -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up &&\n \ttest refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&\n \ttest refs/heads/foo = $(git -C whats-up config branch.foo.merge)\n '\n-- \n2.31.0.rc2.261.g7f71774620-goog\n\n"},{"id":"419485","messageId":"YFI/fxqUoBeCINgv@coredump.intra.peff.net","threadId":"55330","inReplyTo":"20210317154200.2656837-1-jonathantanmy@google.com","subject":"Re: [PATCH] t5606: run clone branch name test with protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-17T17:42:23Z","receivedAt":"2021-03-17T17:43:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 17, 2021 at 08:42:00AM -0700, Jonathan Tan wrote:\n\n> 4f37d45706 (\"clone: respect remote unborn HEAD\", 2021-02-05) introduces\n> a new feature (if the remote has an unborn HEAD, e.g. when the remote\n> repository is empty, use it as the name of the branch) that only works\n> in protocol v2, but did not ensure that one of its tests always uses\n> protocol v2, and thus that test would fail if\n> GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add \"-c\n> protocol.version=2\" to the appropriate test.\n> \n> (The rest of the tests from that commit have \"-c protocol.version=2\"\n> already added.)\n\nThanks, this looks like the obvious and correct fix (and clearly makes\nthe test pass ;) ).\n\n-Peff\n"},{"id":"419490","messageId":"YFJCUOCQGKHX2/So@coredump.intra.peff.net","threadId":"55330","inReplyTo":"YFIGSo3U5u7zy9fq@C02YX140LVDN.corpad.adbkng.com","subject":"Re: Tests failed with GIT_TEST_FAIL_PREREQS and/or GIT_TEST_PROTOCOL_VERSION","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-17T17:54:24Z","receivedAt":"2021-03-17T17:55:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 17, 2021 at 02:38:18PM +0100, Son Luong Ngoc wrote:\n\n> 1. For t7810 and t5300 failing when GIT_TEST_FAIL_PREREQS=1:\n> \n>     a926c4b904bdc339568c2898af955cdc61b31542 is the first bad commit\n>     commit a926c4b904bdc339568c2898af955cdc61b31542\n>     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>     Date:   Thu Feb 11 02:53:51 2021 +0100\n> \n>         tests: remove most uses of C_LOCALE_OUTPUT\n> \n>         As a follow-up to d162b25f956 (tests: remove support for\n>         GIT_TEST_GETTEXT_POISON, 2021-01-20) remove those uses of the now\n>         always true C_LOCALE_OUTPUT prerequisite from those tests which\n>         declare it as an argument to test_expect_{success,failure}.\n> \n>         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>         Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nI looked at the one in t5300, and I don't think it _ever_ worked, nor\ncan it be made to work in this mode.\n\nIt is expecting that running:\n\n  git index-pack --threads=2\n\nwill issue a warning in a build without pthread support. But in the fake\n\"pretend the pthread prereq is not satisfied\" mode, it will of course\nnot do that, because the build itself is not aware that it's supposed to\nbe pretending that pthreads aren't supported!\n\nBefore the patch mentioned above, its prereqs were:\n\n  !PTHREADS,C_LOCALE_OUTPUT\n\nwhich would never be satisfied under the \"pretend\" mode, because\nit would _also_ disable C_LOCALE_OUTPUT, and we'd always skip it.\n\nSo I think something like this creates a similar situation;\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d586fdc7a9..87d26bb70c 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n-test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \\\n+\t'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n \ttest_must_fail git index-pack --threads=2 2>err &&\n \tgrep ^warning: err >warnings &&\n \ttest_line_count = 1 warnings &&\n@@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh\n \tgrep -F \"no threads support, ignoring pack.threads\" err\n '\n \n-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n+test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \\\n+\t'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n \tgit pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&\n \tgrep ^warning: err >warnings &&\n \ttest_line_count = 1 warnings &&\n\nbut I think this points to a failing of the FAIL_PREREQS mode. It is\ngenerally OK to say \"skip this test by pretending you do not have a\nprereq satisfied\" (and that is the point: to see if skipping a test\nconfuses later tests). But given a negated prereq here, it is not OK to\nsay \"run this test that we usually wouldn't\", because it is almost\ncertainly going to be mismatched with the actual build.\n\nSo I think the FAIL_PREREQS mode should probably be treating negated\nprereqs differently (and always pretending that yes, we have them).\n\nI hadn't investigated the t7810 case yet, but looking at it now, it\nseems to be the exact same thing.\n\n-Peff\n"},{"id":"419500","messageId":"xmqqo8fhwsmk.fsf@gitster.g","threadId":"55330","inReplyTo":"20210317154200.2656837-1-jonathantanmy@google.com","subject":"Re: [PATCH] t5606: run clone branch name test with protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-17T18:18:59Z","receivedAt":"2021-03-17T18:19:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> 4f37d45706 (\"clone: respect remote unborn HEAD\", 2021-02-05) introduces\n> a new feature (if the remote has an unborn HEAD, e.g. when the remote\n> repository is empty, use it as the name of the branch) that only works\n> in protocol v2, but did not ensure that one of its tests always uses\n> protocol v2, and thus that test would fail if\n> GIT_TEST_PROTOCOL_VERSION=0 (or 1) is used. Therefore, add \"-c\n> protocol.version=2\" to the appropriate test.\n>\n> (The rest of the tests from that commit have \"-c protocol.version=2\"\n> already added.)\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Thanks, Son Luong, for noticing this. Here's a fix for the\n> GIT_TEST_PROTOCOL_VERSION part. This was built on 4f37d45706 but also\n> applies cleanly on master.\n\nMakes sense.  And I do not see need for any other changes, like\ntest_expect_failure with protocol 0 (or 1).\n\nWill queue as a candidate for maint-2.31.\n\nThanks.\n\n\n\n>\n>  t/t5606-clone-options.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> index ca6339a5fb..5e30772735 100755\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -106,7 +106,7 @@ test_expect_success 'chooses correct default initial branch name' '\n>  \tgit -c init.defaultBranch=foo init --bare empty &&\n>  \ttest_config -C empty lsrefs.unborn advertise &&\n>  \tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \\\n> -\tgit -c init.defaultBranch=up clone empty whats-up &&\n> +\tgit -c init.defaultBranch=up -c protocol.version=2 clone empty whats-up &&\n>  \ttest refs/heads/foo = $(git -C whats-up symbolic-ref HEAD) &&\n>  \ttest refs/heads/foo = $(git -C whats-up config branch.foo.merge)\n>  '\n"},{"id":"419607","messageId":"YFKG52H090l/GbP7@coredump.intra.peff.net","threadId":"55330","inReplyTo":"YFJCUOCQGKHX2/So@coredump.intra.peff.net","subject":"[PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-17T22:47:03Z","receivedAt":"2021-03-17T22:47:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 17, 2021 at 01:54:25PM -0400, Jeff King wrote:\n\n> -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n> +test_expect_success !PTHREADS,IGNORE_FAIL_PREREQS \\\n> +\t'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n>  \tgit pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&\n>  \tgrep ^warning: err >warnings &&\n>  \ttest_line_count = 1 warnings &&\n> \n> but I think this points to a failing of the FAIL_PREREQS mode. It is\n> generally OK to say \"skip this test by pretending you do not have a\n> prereq satisfied\" (and that is the point: to see if skipping a test\n> confuses later tests). But given a negated prereq here, it is not OK to\n> say \"run this test that we usually wouldn't\", because it is almost\n> certainly going to be mismatched with the actual build.\n> \n> So I think the FAIL_PREREQS mode should probably be treating negated\n> prereqs differently (and always pretending that yes, we have them).\n> \n> I hadn't investigated the t7810 case yet, but looking at it now, it\n> seems to be the exact same thing.\n\nIt looks like the problem is indeed somewhat widespread, and there is a\nmagic prereq already to skip such tests.\n\nI do still think that this is a fundamental failing of the FAIL_PREREQS\nmode, but it probably makes sense to annotate these tests in the\nmeantime (I don't plan on looking further into it myself).\n\nAnother rough edge I noticed: if you set GIT_TEST_HTTPD or\nGIT_TEST_GIT_DAEMON to \"yes\" in your config.mak, these play quite badly\nwith GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so\nrefuse to start httpd, and then complain that the setup fails (and the\npoint of \"yes\" for those values is to loudly complain when setup fails,\nrather than quietly skipping the tests).\n\n-- >8 --\nSubject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS\n\nSome tests in t5300 and t7810 expect us to complain about a \"--threads\"\nargument when Git is compiled without pthread support. Running these\nunder GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to\nthe tests that there is no pthread support, so they expect the warning,\nbut of course the actual build is perfectly happy to respect the\n--threads argument.\n\nWe never noticed before the recent a926c4b904 (tests: remove most uses\nof C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as\nrequiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never\nhave run in FAIL_PREREQS mode, since it would always pretend that the\nlocale prereq was not satisfied.\n\nThese tests can't possibly work in this mode; it is a mismatch between\nwhat the tests expect and what the build was told to do. So let's just\nmark them to be skipped, using the special prereq introduced by\ndfe1a17df9 (tests: add a special setup where prerequisites fail,\n2019-05-13).\n\nReported-by: Son Luong Ngoc <sluongng@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5300-pack-object.sh | 6 ++++--\n t/t7810-grep.sh        | 3 ++-\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex d586fdc7a9..e830a37a38 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '\n \ttest_path_is_file foo.idx\n '\n \n-test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n+test_expect_success !PTHREADS,!FAIL_PREREQS \\\n+\t'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n \ttest_must_fail git index-pack --threads=2 2>err &&\n \tgrep ^warning: err >warnings &&\n \ttest_line_count = 1 warnings &&\n@@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh\n \tgrep -F \"no threads support, ignoring pack.threads\" err\n '\n \n-test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n+test_expect_success !PTHREADS,!FAIL_PREREQS \\\n+\t'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n \tgit pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&\n \tgrep ^warning: err >warnings &&\n \ttest_line_count = 1 warnings &&\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex edfaa9a6d1..5830733f3d 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -969,7 +969,8 @@ do\n \t\"\n done\n \n-test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' '\n+test_expect_success !PTHREADS,!FAIL_PREREQS \\\n+\t'grep --threads=N or pack.threads=N warns when no pthreads' '\n \tgit grep --threads=2 Hello hello_world 2>err &&\n \tgrep ^warning: err >warnings &&\n \ttest_line_count = 1 warnings &&\n-- \n2.31.0.559.g509d4a088b\n\n"},{"id":"419655","messageId":"xmqqblbgrwkg.fsf@gitster.g","threadId":"55330","inReplyTo":"YFKG52H090l/GbP7@coredump.intra.peff.net","subject":"Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-18T21:17:19Z","receivedAt":"2021-03-18T21:18:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> So I think the FAIL_PREREQS mode should probably be treating negated\n>> prereqs differently (and always pretending that yes, we have them).\n>> \n>> I hadn't investigated the t7810 case yet, but looking at it now, it\n>> seems to be the exact same thing.\n>\n> It looks like the problem is indeed somewhat widespread, and there is a\n> magic prereq already to skip such tests.\n>\n> I do still think that this is a fundamental failing of the FAIL_PREREQS\n> mode, but it probably makes sense to annotate these tests in the\n> meantime (I don't plan on looking further into it myself).\n\nThe README file in t/ directory claims that this \"is useful for\ndiscovering issues with the tests where say a later test implicitly\ndepends on an optional earlier test.\" but apparently it does not\nwork well with these negated prerequisites.  Its implementation\nprobably should force a safe bypass of the whole test_have_prereq()\netc. done in test_skip by hooking into test_verify_prereq and\noverwrite any non-empty test_prereq with a single hardcoded\nPRETEND_FAIL_PREREQ prerequisite that is never satisfied, or\nsomething.\n\n> Another rough edge I noticed: if you set GIT_TEST_HTTPD or\n> GIT_TEST_GIT_DAEMON to \"yes\" in your config.mak, these play quite badly\n> with GIT_TEST_FAIL_PREREQS. We think NOT_ROOT is not satisfied, so\n> refuse to start httpd, and then complain that the setup fails (and the\n> point of \"yes\" for those values is to loudly complain when setup fails,\n> rather than quietly skipping the tests).\n\n... and I think this would also be gone, as the NOT_ROOT test is\ndone with test_have_prereq that we wouldn't be mucking with if we\nlimit the FAIL_PREREQS only to tweak the test_expect_* prereqs.\n\nIn short, the biggest mistake in the current FAIL_PREREQS design is\nto hook into test_have_prereq while the stated objective only needs\nto futz with the prerequisite given to the test_expect_* functions,\nI would think.\n\n> -- >8 --\n> Subject: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS\n>\n> Some tests in t5300 and t7810 expect us to complain about a \"--threads\"\n> argument when Git is compiled without pthread support. Running these\n> under GIT_TEST_FAIL_PREREQS produces a confusing failure: we pretend to\n> the tests that there is no pthread support, so they expect the warning,\n> but of course the actual build is perfectly happy to respect the\n> --threads argument.\n>\n> We never noticed before the recent a926c4b904 (tests: remove most uses\n> of C_LOCALE_OUTPUT, 2021-02-11), because the tests also were marked as\n> requiring the C_LOCALE_OUTPUT prerequisite. Which means they'd never\n> have run in FAIL_PREREQS mode, since it would always pretend that the\n> locale prereq was not satisfied.\n>\n> These tests can't possibly work in this mode; it is a mismatch between\n> what the tests expect and what the build was told to do. So let's just\n> mark them to be skipped, using the special prereq introduced by\n> dfe1a17df9 (tests: add a special setup where prerequisites fail,\n> 2019-05-13).\n>\n> Reported-by: Son Luong Ngoc <sluongng@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5300-pack-object.sh | 6 ++++--\n>  t/t7810-grep.sh        | 3 ++-\n>  2 files changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index d586fdc7a9..e830a37a38 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -427,7 +427,8 @@ test_expect_success 'index-pack --strict <pack> works in non-repo' '\n>  \ttest_path_is_file foo.idx\n>  '\n>  \n> -test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n> +test_expect_success !PTHREADS,!FAIL_PREREQS \\\n> +\t'index-pack --threads=N or pack.threads=N warns when no pthreads' '\n>  \ttest_must_fail git index-pack --threads=2 2>err &&\n>  \tgrep ^warning: err >warnings &&\n>  \ttest_line_count = 1 warnings &&\n> @@ -445,7 +446,8 @@ test_expect_success !PTHREADS 'index-pack --threads=N or pack.threads=N warns wh\n>  \tgrep -F \"no threads support, ignoring pack.threads\" err\n>  '\n>  \n> -test_expect_success !PTHREADS 'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n> +test_expect_success !PTHREADS,!FAIL_PREREQS \\\n> +\t'pack-objects --threads=N or pack.threads=N warns when no pthreads' '\n>  \tgit pack-objects --threads=2 --stdout --all </dev/null >/dev/null 2>err &&\n>  \tgrep ^warning: err >warnings &&\n>  \ttest_line_count = 1 warnings &&\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index edfaa9a6d1..5830733f3d 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -969,7 +969,8 @@ do\n>  \t\"\n>  done\n>  \n> -test_expect_success !PTHREADS 'grep --threads=N or pack.threads=N warns when no pthreads' '\n> +test_expect_success !PTHREADS,!FAIL_PREREQS \\\n> +\t'grep --threads=N or pack.threads=N warns when no pthreads' '\n>  \tgit grep --threads=2 Hello hello_world 2>err &&\n>  \tgrep ^warning: err >warnings &&\n>  \ttest_line_count = 1 warnings &&\n"},{"id":"419659","messageId":"YFPL0F4C7mfDazOW@coredump.intra.peff.net","threadId":"55330","inReplyTo":"xmqqblbgrwkg.fsf@gitster.g","subject":"Re: [PATCH] t: annotate !PTHREADS tests with !FAIL_PREREQS","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-18T21:53:20Z","receivedAt":"2021-03-18T21:54:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 18, 2021 at 02:17:19PM -0700, Junio C Hamano wrote:\n\n> In short, the biggest mistake in the current FAIL_PREREQS design is\n> to hook into test_have_prereq while the stated objective only needs\n> to futz with the prerequisite given to the test_expect_* functions,\n> I would think.\n\nYeah, that matches my intuition of the problem, too.\n\n-Peff\n"}]}