| Message ID | 3c3c8c20bcb4e570d25a676ad1f29877762adb82.1657852722.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove use of "whitelist" | expand |
On Fri, Jul 15 2022, Derrick Stolee via GitGitGadget wrote: > 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>"...
On 7/15/2022 7:02 AM, Ævar Arnfjörð Bjarmason wrote: >> 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. Another iteration: GIT_TEST_PASSING_SANITIZE_LEAK=<bool> focuses the test suite on finding memory leaks. When the variable is true and Git is compiled with SANITIZE=leak, 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? Sounds good. >> -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. Perhaps "filtering" is the best way to describe the higher-level feature that these lists help to implement. Thanks, -Stolee
On Tue, Jul 19 2022, Derrick Stolee wrote: > On 7/15/2022 7:02 AM, Ævar Arnfjörð Bjarmason wrote: >>> 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. > > Another iteration: > > GIT_TEST_PASSING_SANITIZE_LEAK=<bool> focuses the test suite on finding > memory leaks. In some ways it does the opposite of "focusing on finding memory leaks", we're explicitly finding fewer memory leaks, what we're doing is asserting that the tests we've whitelisted still pass. > SANITIZE=leak, 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. Perhaps: GIT_TEST_PASSING_SANITIZE_LEAK=true skips those tests that haven't declared themselves as 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. Anyway, do you mind if this part is dropped from this series? I have a set of patches I was meaning to submit to add a tri-state [false,true,check] support for this, which this conflicts with. "check" being a mode where we check that the tests in "false" aren't yet passing. So for that I'll need to re-word this whole thing anyway. I'll rephrase this while I'm at it to something I think you'll be OK with. >>> -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. > > Perhaps "filtering" is the best way to describe the higher-level > feature that these lists help to implement. *Shrug*. I think "filtering" more naturally refers to the process, i.e. in Makefile syntax: $(filter-out $(A),$(LIST)) $(filter $(B),$(LIST)) I'd call $(B) a whitelist (or whatever), $(A) a blacklist (or whatever), but "filtering" the process of producing them.
On 7/19/2022 11:26 AM, Ævar Arnfjörð Bjarmason wrote: > Anyway, do you mind if this part is dropped from this series? > > I have a set of patches I was meaning to submit to add a tri-state > [false,true,check] support for this, which this conflicts with. > > "check" being a mode where we check that the tests in "false" aren't yet > passing. So for that I'll need to re-word this whole thing anyway. I'll > rephrase this while I'm at it to something I think you'll be OK with. I had just split the changes regarding SANITIZE=leak into its own patch, so it will be easy for me to drop it from my series. Thanks for working on it. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> 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. > > Another iteration: > > GIT_TEST_PASSING_SANITIZE_LEAK=<bool> focuses the test suite on finding > memory leaks. When the variable is true and Git is compiled with > SANITIZE=leak, 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. Sounds good. These scripts opt into the sanitize-leak tests by declaring themselves to be leak-free and that is captured very well without using the *list word ;-)
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. GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version' default to n. diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh index 83babe57d95..890622be816 100644 --- a/t/lib-proto-disable.sh +++ b/t/lib-proto-disable.sh @@ -1,7 +1,7 @@ # Test routines for checking protocol disabling. -# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist -test_whitelist () { +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL environment variable +test_allow_var () { desc=$1 proto=$2 url=$3 @@ -183,7 +183,7 @@ test_config () { # $2 - machine-readable name of the protocol # $3 - the URL to try cloning test_proto () { - test_whitelist "$@" + test_allow_var "$@" test_config "$@" } diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh index af8772fadaa..bbeebee02f1 100755 --- a/t/t5812-proto-disable-http.sh +++ b/t/t5812-proto-disable-http.sh @@ -16,7 +16,7 @@ test_expect_success 'create git-accessible repo' ' test_proto "smart http" http "$HTTPD_URL/smart/repo.git" -test_expect_success 'curl redirects respect whitelist' ' +test_expect_success 'curl redirects respect allowed protocols' ' test_must_fail env GIT_ALLOW_PROTOCOL=http:https \ GIT_SMART_HTTP=0 \ git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr && diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh index 06f55a1b8a0..990f034149d 100755 --- a/t/t5815-submodule-protos.sh +++ b/t/t5815-submodule-protos.sh @@ -1,6 +1,6 @@ #!/bin/sh -test_description='test protocol whitelisting with submodules' +test_description='test protocol restrictions with submodules' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-proto-disable.sh @@ -36,7 +36,7 @@ test_expect_success 'update of ext not allowed' ' test_must_fail git -C dst submodule update ext-module ' -test_expect_success 'user can override whitelist' ' +test_expect_success 'user can override with environment variable' ' GIT_ALLOW_PROTOCOL=ext git -C dst submodule update ext-module ' diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 210ddf09e30..51b798cb493 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -221,7 +221,7 @@ test_expect_success 'req_Root (export-all)' \ 'cat request-anonymous | git-cvsserver --export-all pserver "$WORKDIR" >log 2>&1 && sed -ne \$p log | grep "^I LOVE YOU\$"' -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)' test_expect_success 'req_Root (everything together)' \ diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 6da7273f1d5..6fe62329d8b 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -651,7 +651,7 @@ test_set_prereq () { # test_unset_prereq() !*) ;; - # (Temporary?) whitelist of things we can't easily + # (Temporary?) list of things we can't easily # pretend not to support SYMLINKS) ;; diff --git a/t/test-lib.sh b/t/test-lib.sh index 55857af601b..fff85f4b425 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1408,7 +1408,7 @@ then test_done fi -# skip non-whitelisted tests when compiled with SANITIZE=leak +# skip unmarked tests when compiled with SANITIZE=leak if test -n "$SANITIZE_LEAK" then if test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false