Re: [PATCH v2 2/3] t/*: avoid "whitelist"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 15, 2022, 11:02 UTC
Message-ID: <220715.86o7xqzkt3.gmgdl@evledraar.gmail.com>
In-Reply-To: <3c3c8c20bcb4e570d25a676ad1f29877762adb82.1657852722.git.gitgitgadget@gmail.com>

On Fri, Jul 15 2022, Derrick Stolee via GitGitGadget wrote:

Show 37 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> The word "whitelist" has cultural implications that are not inclusive.
> Thankfully, it is not difficult to reword and avoid its use.
>
> Focus on changes in the test scripts, since most of the changes are in
> comments and test names. The renamed test_allow_var helper is only used
> once inside the widely-used test_proto helper.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/README                        | 9 ++++-----
>  t/lib-proto-disable.sh          | 6 +++---
>  t/t5812-proto-disable-http.sh   | 2 +-
>  t/t5815-submodule-protos.sh     | 4 ++--
>  t/t9400-git-cvsserver-server.sh | 2 +-
>  t/test-lib-functions.sh         | 2 +-
>  t/test-lib.sh                   | 2 +-
>  7 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/t/README b/t/README
> index 309a31133c6..56d5ebb5798 100644
> --- a/t/README
> +++ b/t/README
> @@ -367,11 +367,10 @@ GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
>  test suite. Accept any boolean values that are accepted by git-config.
>  
>  GIT_TEST_PASSING_SANITIZE_LEAK=<boolean> when compiled with
> -SANITIZE=leak will run only those tests that have whitelisted
> -themselves as passing with no memory leaks. Tests can be whitelisted
> -by setting "TEST_PASSES_SANITIZE_LEAK=true" before sourcing
> -"test-lib.sh" itself at the top of the test script. This test mode is
> -used by the "linux-leaks" CI target.
> +SANITIZE=leak will run only those tests that have marked themselves as
> +passing with no memory leaks by setting "TEST_PASSES_SANITIZE_LEAK=true"
> +before sourcing "test-lib.sh" itself at the top of the test script. This
> +test mode is used by the "linux-leaks" CI target.

It's hard to improve your own verbage, but I think in this case my original version can be improved still:

	GIT_TEST_PASSING_SANITIZE_LEAK=<bool> when compiled with
	SANITIZE=leak will, when true, only run those tests that declare
	themselves leak-free by setting "TEST_PASSES_SANITIZE_LEAK=true"
	before sourcing "test-lib.sh". This test mode is used by the
	"linux-leaks" CI target.

> -test_expect_success 'curl redirects respect whitelist' '
> +test_expect_success 'curl redirects respect allowed protocols' '

Isn't the real problem here that this is inaccurate with regards to "curl", i.e. AFAIK from browsing transport.c the whitelist of protocols has nothing to do with curl, we parse that out and apply it before we ever get to the specific transport layer.

So this should just be "http(s) transport respects GIT_ALLOW_PROTOCOL", no?

> -test_description='test protocol whitelisting with submodules'
> +test_description='test protocol restrictions with submodules'

Minor: I think this shows the awkwardness of using a word derived from
"allow". Before we could use "whitelist" and "whitelisting"
consistentlry, but now you have "allowed", "allowlist", "restrictions"
etc.

I guess you could say "test protocol allowances..." or something? Meh.

> -test_expect_success 'user can override whitelist' '
> +test_expect_success 'user can override with environment variable' '

Override what? This is a non-improvement. Saying "how" with "environment variable" is good, but we dropped any mention of what's being tested.

> -test_expect_success 'req_Root failure (export-all w/o whitelist)' \
> +test_expect_success 'req_Root failure (export-all w/o directory)' \
>    '! (cat request-anonymous | git-cvsserver --export-all pserver >log 2>&1 || false)'

Is it really that we don't have any directiory whatsoever, in that case I think this is a canditate for splitting up. I.e. the test didn't test what it was intending to test?

But if it's really "without <whatever you call the list of stuff you don't want>" we should still say that, no?

> -		# (Temporary?) whitelist of things we can't easily
> +		# (Temporary?) list of things we can't easily
>  		# pretend not to support

We've had this since my dfe1a17df9b (tests: add a special setup where prerequisites fail, 2019-05-13), so we can probably just drop "(Temporary?)" there while we're at it...

> -# skip non-whitelisted tests when compiled with SANITIZE=leak
> +# skip unmarked tests when compiled with SANITIZE=leak

This subtly changes the meaning in a way I didn't intend when writing this. I.e. the existing wording could be improved, but some of what we're skipping isn't "unmarked", it's explicitly marked as "false" (which we assume by default).

I.e. I read this new wording as saying "skip <unset>", whereas the previous can encompas "<unset,false>", just not "<true>"...