{"thread":{"id":"53777","subject":"[PATCH 1/5] t3701: stop using `env` in force_color()","startedAt":"2020-06-30T15:03:33Z","lastAt":"2020-07-07T22:29:45Z","messageCount":29,"participants":["Denton Liu","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"400816","messageId":"67d5b93fdaab7f73f352293372ee3d71fb7c1409.1593529394.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593529394.git.liu.denton@gmail.com","subject":"[PATCH 1/5] t3701: stop using `env` in force_color()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-30T15:03:15Z","receivedAt":"2020-06-30T15:03:33Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future patch, we plan on making the test_must_fail()-family of\nfunctions accept only git commands. Even though force_color() wraps an\ninvocation of `env git`, test_must_fail() will not be able to figure\nthis out since it will assume that force_color() is just some random\nfunction which is disallowed.\n\nInstead of using `env` in force_color() (which does not support shell\nfunctions), export the environment variables in a subshell. Write the\ninvocation as `force_color test_must_fail git ...` since shell functions\nare now supported.\n\nSigned-off-by: Denton Liu \n---\n t/t3701-add-interactive.sh | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 49decbac71..c4c1e9b603 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -31,7 +31,13 @@ diff_cmp () {\n # indicates a dumb terminal, so we set that variable, too.\n \n force_color () {\n-\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n+\t(\n+\t\tGIT_PAGER_IN_USE=true &&\n+\t\texport GIT_PAGER_IN_USE &&\n+\t\tTERM=vt100 &&\n+\t\texport TERM &&\n+\t\t\"$@\"\n+\t)\n }\n \n test_expect_success 'setup (initial)' '\n@@ -604,7 +610,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \techo content >test &&\n \ttest_config interactive.diffFilter \"sed 1d\" &&\n \tprintf y >y &&\n-\ttest_must_fail force_color git add -p \n---\n t/t7107-reset-pathspec-file.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh\nindex cad3a9de9e..15ccb14f7e 100755\n--- a/t/t7107-reset-pathspec-file.sh\n+++ b/t/t7107-reset-pathspec-file.sh\n@@ -22,7 +22,12 @@ restore_checkpoint () {\n \n verify_expect () {\n \tgit status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&\n-\ttest_cmp expect actual\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\t! test_cmp expect actual\n+\telse\n+\t\ttest_cmp expect actual\n+\tfi\n }\n \n test_expect_success '--pathspec-from-file from stdin' '\n@@ -131,7 +136,7 @@ test_expect_success 'quotes not compatible with --pathspec-file-nul' '\n \tcat >expect <<-\\EOF &&\n \t D fileA.t\n \tEOF\n-\ttest_must_fail verify_expect\n+\tverify_expect !\n '\n \n test_expect_success 'only touches what was listed' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400819","messageId":"aae48a89e50ce1841df33edbbeb17ae932c10d16.1593529394.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593529394.git.liu.denton@gmail.com","subject":"[PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-30T15:03:16Z","receivedAt":"2020-06-30T15:03:36Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_might_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `run_with_limited_open_files` comes before `test_might_fail`. This\nway, `test_might_fail` operates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t5324-split-commit-graph.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 269d0964a3..9b850ea907 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -399,7 +399,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion'\n \t\tfor i in $(test_seq 64)\n \t\tdo\n \t\t\ttest_commit $i &&\n-\t\t\ttest_might_fail run_with_limited_open_files git commit-graph write \\\n+\t\t\trun_with_limited_open_files test_might_fail git commit-graph write \\\n \t\t\t\t--split=no-merge --reachable || return 1\n \t\tdone\n \t)\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400820","messageId":"1287798e691bd3be1ac094bacae5149560c9e6df.1593529394.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593529394.git.liu.denton@gmail.com","subject":"[PATCH 4/5] t9834: remove use of `test_might_fail p4`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-30T15:03:18Z","receivedAt":"2020-06-30T15:03:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\na compound command wrapping the old p4 invocation that always returns 0.\n\nSigned-off-by: Denton Liu \n---\n t/t9834-git-p4-file-dir-bug.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh\nindex 031e1f8668..dac67e89d7 100755\n--- a/t/t9834-git-p4-file-dir-bug.sh\n+++ b/t/t9834-git-p4-file-dir-bug.sh\n@@ -10,7 +10,7 @@ repository.'\n \n test_expect_success 'start p4d' '\n \tstart_p4d &&\n-\ttest_might_fail p4 configure set submit.collision.check=0\n+\t{ p4 configure set submit.collision.check=0 || :; }\n '\n \n test_expect_success 'init depot' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400821","messageId":"01e29450fe51a4ba13e07c611d8795ffd0282b9e.1593529394.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593529394.git.liu.denton@gmail.com","subject":"[PATCH 5/5] test-lib-functions: restrict test_must_fail usage","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-30T15:03:19Z","receivedAt":"2020-06-30T15:03:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In previous commits, we removed the usage of test_must_fail() for most\ncommands except for a set of pre-approved commands. Since that's done,\nonly allow test_must_fail() to run those pre-approved commands.\n\nObviously, we should allow `git`.\n\nWe allow `__git*` as some completion functions return an error code that\ncomes from a git invocation. It's good to avoid using test_must_fail\nunnecessarily but it wouldn't hurt to err on the side of caution when\nwe're potentially wrapping a git command (like in these case).\n\nWe also allow `test-tool` and `test-svn-fe` because these are helper\ncommands that are written by us and we want to catch their failure.\n\nFinally, we allow `test_terminal` because `test_terminal` just wraps\naround git commands. Also, we cannot rewrite\n`test_must_fail test_terminal` as `test_terminal test_must_fail` because\ntest_must_fail() is a shell function and as a result, it cannot be\ninvoked from the test-terminal Perl script.\n\nWe opted to explicitly list the above tools instead of using a catch-all\nsuch as `test[-_]*` because we want to be as restrictive as possible so\nthat in the future, someone would not accidentally introduce an\nunrelated usage of test_must_fail() on an \"unapproved\" command.\n\nSigned-off-by: Denton Liu \n---\n t/t0000-basic.sh | 18 ++++++++++++++++++\n t/test-lib-functions.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2ff176cd5d..f5e4fb515d 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '\n \ttest $len = 4098\n '\n \n+test_expect_success 'test_must_fail on a failing git command' '\n+\ttest_must_fail git notacommand\n+'\n+\n+test_expect_success 'test_must_fail on a failing git command with env' '\n+\ttest_must_fail env var1=a var2=b env var3=c git notacommand\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command' '\n+\t! test_must_fail grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command with env' '\n+\t! test_must_fail env var1=a var2=b env var3=c grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3103be8a32..16596b28ba 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -798,6 +798,31 @@ list_contains () {\n \treturn 1\n }\n \n+# Returns success if the arguments indicate that a command should be\n+# accepted by test_must_fail(). If the command is run with env, the env\n+# and its corresponding variable settings will be stripped before we\n+# test the command being run.\n+test_must_fail_acceptable () {\n+\twhile test \"$1\" = \"env\"\n+\tdo\n+\t\tshift\n+\t\twhile test $# -gt 0\n+\t\tdo\n+\t\t\tcase \"$1\" in *?=*) ;; *) break ;; esac\n+\t\t\tshift\n+\t\tdone\n+\tdone\n+\n+\tcase \"$1\" in\n+\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n+\t\treturn 0\n+\t\t;;\n+\t*)\n+\t\treturn 1\n+\t\t;;\n+\tesac\n+}\n+\n # This is not among top-level (test_expect_success | test_expect_failure)\n # but is a prefix that can be used in the test script, like:\n #\n@@ -817,6 +842,15 @@ list_contains () {\n # Multiple signals can be specified as a comma separated list.\n # Currently recognized signal names are: sigpipe, success.\n # (Don't use 'success', use 'test_might_fail' instead.)\n+#\n+# Do not use this to run anything but \"git\" and other specific testable\n+# commands (see test_must_fail_acceptable()). We are not in the\n+# business of vetting system supplied commands -- in other words, this\n+# is wrong:\n+#\n+# test_must_fail grep pattern output\n+#\n+# Just use '!' instead.\n \n test_must_fail () {\n \tcase \"$1\" in\n@@ -828,6 +862,11 @@ test_must_fail () {\n \t\t_test_ok=\n \t\t;;\n \tesac\n+\tif ! test_must_fail_acceptable \"$@\"\n+\tthen\n+\t echo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n+\t return 1\n+\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400855","messageId":"CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com","threadId":"53777","inReplyTo":"01e29450fe51a4ba13e07c611d8795ffd0282b9e.1593529394.git.liu.denton@gmail.com","subject":"Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-30T20:56:22Z","receivedAt":"2020-06-30T20:56:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2020 at 11:03 AM Denton Liu wrote:\n> In previous commits, we removed the usage of test_must_fail() for most\n> commands except for a set of pre-approved commands. Since that's done,\n> only allow test_must_fail() to run those pre-approved commands.\n>\n> Obviously, we should allow `git`.\n>\n> We allow `__git*` as some completion functions return an error code that\n> comes from a git invocation. It's good to avoid using test_must_fail\n> unnecessarily but it wouldn't hurt to err on the side of caution when\n> we're potentially wrapping a git command (like in these case).\n\ns/case/cases/\n\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> +# Returns success if the arguments indicate that a command should be\n> +# accepted by test_must_fail(). If the command is run with env, the env\n> +# and its corresponding variable settings will be stripped before we\n> +# test the command being run.\n> +test_must_fail_acceptable () {\n> + while test \"$1\" = \"env\"\n\nI was surprised to see a 'while' loop for stripping 'env'. Did you\nactually run across cases in the test suite in which 'env' was\ninvoking 'env'? If so, were such cases legitimate (as opposed to\naccidental)? Perhaps the commit message or an in-code comment could\nhelp readers understand why it needs to strip multiple 'env's.\n\n> + do\n> + shift\n> + while test $# -gt 0\n> + do\n> + case \"$1\" in *?=*) ;; *) break ;; esac\n> + shift\n> + done\n> + done\n\nIsn't '*?=*' the same as '?=', or am I misunderstanding the intention?\nAlso, I wonder how important it is to insist that there must be at\nleast one character before the '=' sign. (It doesn't necessarily hurt,\nbut I'm curious if it is protecting against legitimate weird cases.)\n\nThis logic would be easier to follow written this way:\n\n case \"$1\" in\n =) shift ;;\n *) break ;;\n esac\n\nThat is, place the 'shift' in the appropriate case-arm rather than\nsuspending it below all cases.\n\n> + case \"$1\" in\n> + git|__git*|test-tool|test-svn-fe|test_terminal)\n> + return 0\n> + ;;\n> + *)\n> + return 1\n> + ;;\n> + esac\n> +}\n\nWould it make sense to error out if \"$1\" has no value? That is, if the\nauthor wrote:\n\n test_must_fail &&\n\nor\n\n test_must_fail env foo=bar &&\n\nthen that surely is a programmer error, which could be diagnosed here\n(though the original 'test_must_fail' didn't bother diagnosing that\nproblem so it may be overkill and outside the scope of this series to\ndo so here).\n\n> @@ -817,6 +842,15 @@ list_contains () {\n> +# Do not use this to run anything but \"git\" and other specific testable\n> +# commands (see test_must_fail_acceptable()). We are not in the\n> +# business of vetting system supplied commands -- in other words, this\n> +# is wrong:\n> +#\n> +# test_must_fail grep pattern output\n> +#\n> +# Just use '!' instead.\n\nI find this somewhat ambiguous; it's not clear at first sight what I'm\nsupposed to do with '!'. t/README is slightly clearer by saying \"use\n'! cmd' instead\". It might be even clearer to spell it out explicitly\nwith an example:\n\n Instead use '!':\n\n ! grep pattern output\n"},{"id":"400856","messageId":"CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com","threadId":"53777","inReplyTo":"67d5b93fdaab7f73f352293372ee3d71fb7c1409.1593529394.git.liu.denton@gmail.com","subject":"Re: [PATCH 1/5] t3701: stop using `env` in force_color()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-30T21:48:44Z","receivedAt":"2020-06-30T21:48:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2020 at 11:03 AM Denton Liu wrote:\n> In a future patch, we plan on making the test_must_fail()-family of\n> functions accept only git commands. Even though force_color() wraps an\n> invocation of `env git`, test_must_fail() will not be able to figure\n> this out since it will assume that force_color() is just some random\n> function which is disallowed.\n>\n> Instead of using `env` in force_color() (which does not support shell\n> functions), export the environment variables in a subshell. Write the\n> invocation as `force_color test_must_fail git ...` since shell functions\n> are now supported.\n>\n> Signed-off-by: Denton Liu \n> ---\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> @@ -31,7 +31,13 @@ diff_cmp () {\n> force_color () {\n> - env GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n> + (\n> + GIT_PAGER_IN_USE=true &&\n> + export GIT_PAGER_IN_USE &&\n> + TERM=vt100 &&\n> + export TERM &&\n> + \"$@\"\n> + )\n> }\n\nI'm having trouble understanding why this function was transformed the\nway it was. I presume the subshell is to ensure that the variable\nassignments don't escape the function context since you're dropping\n'env', however, it seems the following would be simpler:\n\n force_color () {\n (GIT_PAGER_IN_USE=true TERM=vt100 \"$@\")\n }\n\nOr, is there something non-obvious going on that I'm missing?\n\nBy the way, I'm wondering if the subshell deserves an in-code comment.\nWhereas, we have somewhat settled upon the idiom 'test_must_fail env\nFOO=bar ...' when we need to make sure variable assignments don't\nescape the local context -- since 'FOO=bar test_must_fail ...' doesn't\nmake that guarantee under all shells -- the use of a subshell here to\nachieve the same (if I'm understanding correctly) is not nearly so\nobvious. The non-obviousness is due to \"$@\" being abstract -- someone\nreading the code won't necessarily realize that the first element of\n\"$@\" might be a shell function, thus would not necessarily understand\nthe use of a subshell.\n"},{"id":"400857","messageId":"CAPig+cQdqYkCW7UED+sn+P-is+tDDGtY-KConhgJZW=CCHAk9A@mail.gmail.com","threadId":"53777","inReplyTo":"CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com","subject":"Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-30T21:59:17Z","receivedAt":"2020-06-30T21:59:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 30, 2020 at 4:56 PM Eric Sunshine wrote:\n> On Tue, Jun 30, 2020 at 11:03 AM Denton Liu wrote:\n> > + case \"$1\" in *?=*) ;; *) break ;; esac\n>\n> Isn't '*?=*' the same as '?=', or am I misunderstanding the intention?\n\nNevermind. My brain blipped out while reading that. Obviously '?=' is\nnot the same as '*?=*' here.\n\nThe rest of the review comments still stand (I think).\n"},{"id":"400858","messageId":"xmqqlfk4dx9h.fsf@gitster.c.googlers.com","threadId":"53777","inReplyTo":"CAPig+cTYJwDL_uGfSR0OmU4fYANbqQ5jLkyN29WXDvq6gWG_Zg@mail.gmail.com","subject":"Re: [PATCH 1/5] t3701: stop using `env` in force_color()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-30T22:06:18Z","receivedAt":"2020-06-30T22:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Jun 30, 2020 at 11:03 AM Denton Liu wrote:\n>> In a future patch, we plan on making the test_must_fail()-family of\n>> functions accept only git commands. Even though force_color() wraps an\n>> invocation of `env git`, test_must_fail() will not be able to figure\n>> this out since it will assume that force_color() is just some random\n>> function which is disallowed.\n>>\n>> Instead of using `env` in force_color() (which does not support shell\n>> functions), export the environment variables in a subshell. Write the\n>> invocation as `force_color test_must_fail git ...` since shell functions\n>> are now supported.\n>>\n>> Signed-off-by: Denton Liu \n>> ---\n>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>> @@ -31,7 +31,13 @@ diff_cmp () {\n>> force_color () {\n>> - env GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n>> + (\n>> + GIT_PAGER_IN_USE=true &&\n>> + export GIT_PAGER_IN_USE &&\n>> + TERM=vt100 &&\n>> + export TERM &&\n>> + \"$@\"\n>> + )\n>> }\n>\n> I'm having trouble understanding why this function was transformed the\n> way it was. I presume the subshell is to ensure that the variable\n> assignments don't escape the function context since you're dropping\n> 'env', however, it seems the following would be simpler:\n>\n> force_color () {\n> (GIT_PAGER_IN_USE=true TERM=vt100 \"$@\")\n> }\n>\n> Or, is there something non-obvious going on that I'm missing?\n\nDenton wants to be able to write\n\n\tforce_color test_must_fail git foo blah\n\nThe only known kind of POSIX breaking behaviour by some shells makes\nthe assignment persist after the \"$@\" returns, and the subshell\nwould be a good way to protect the caller of force_color from the\neffect of the assignment leaking, so if we only care about that\nknown breakage, both Denton's and your simplification should work\nOK, but it is probably clearer to see that we are *not* relying on\nany one-shot environment assignment at all in Denton's version.\n\nI'd list two variables on a single export command, though ;-)\n\n> By the way, I'm wondering if the subshell deserves an in-code comment.\n> Whereas, we have somewhat settled upon the idiom 'test_must_fail env\n> FOO=bar ...' when we need to make sure variable assignments don't\n> escape the local context -- since 'FOO=bar test_must_fail ...' doesn't\n> make that guarantee under all shells -- the use of a subshell here to\n> achieve the same (if I'm understanding correctly) is not nearly so\n> obvious. The non-obviousness is due to \"$@\" being abstract -- someone\n> reading the code won't necessarily realize that the first element of\n> \"$@\" might be a shell function, thus would not necessarily understand\n> the use of a subshell.\n\nProbably.\n"},{"id":"400860","messageId":"20200630233939.GA1301999@generichostname","threadId":"53777","inReplyTo":"CAPig+cSNK1MDitZyh7Ax-eRAh6NjG_QsoF0feEo4475GjZ5ezw@mail.gmail.com","subject":"Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-30T23:39:39Z","receivedAt":"2020-06-30T23:39:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Tue, Jun 30, 2020 at 04:56:22PM -0400, Eric Sunshine wrote:\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > +# Returns success if the arguments indicate that a command should be\n> > +# accepted by test_must_fail(). If the command is run with env, the env\n> > +# and its corresponding variable settings will be stripped before we\n> > +# test the command being run.\n> > +test_must_fail_acceptable () {\n> > + while test \"$1\" = \"env\"\n> \n> I was surprised to see a 'while' loop for stripping 'env'. Did you\n> actually run across cases in the test suite in which 'env' was\n> invoking 'env'? If so, were such cases legitimate (as opposed to\n> accidental)? Perhaps the commit message or an in-code comment could\n> help readers understand why it needs to strip multiple 'env's.\n\nThere are no cases of nested envs. When I was writing this, I wanted to\nbe as permissive as possible in case someone wrote something like this.\nNow that you bring it up, though, I don't think it makes sense to\nsupport this use case because I can't come up with any legitimate reason\nto have nested envs. (If something comes up in the future, we can\nreinstate this behaviour.)\n\n> > + do\n> > + shift\n> > + while test $# -gt 0\n> > + do\n> > + case \"$1\" in *?=*) ;; *) break ;; esac\n> > + shift\n> > + done\n> > + done\n\n> Would it make sense to error out if \"$1\" has no value? That is, if the\n> author wrote:\n> \n> test_must_fail &&\n> \n> or\n> \n> test_must_fail env foo=bar &&\n> \n> then that surely is a programmer error, which could be diagnosed here\n> (though the original 'test_must_fail' didn't bother diagnosing that\n> problem so it may be overkill and outside the scope of this series to\n> do so here).\n\nThis is caught by the \n\n\tcase \"$1\" in\n\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n\t\treturn 0\n\t\t;;\n\t*)\n\t\treturn 1\n\t\t;;\n\tesac\n\npart. It only emits\n\n\ttest_must_fail: only 'git' is allowed:\n\nif this happens but it's probably fine as I don't think we should do\nmuch hand-holding for this case.\n\nAnd I'll use the remainder of your suggestions.\n\nThanks,\n\nDenton\n"},{"id":"400863","messageId":"cover.1593576601.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593529394.git.liu.denton@gmail.com","subject":"[PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-01T04:27:08Z","receivedAt":"2020-07-01T04:27:31Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the sixth and final(!) part. It cleans up instances of\n`test_must_fail` that were introduced since the effort began. In\naddition, it finally flips the switch and makes test_must_fail only\nallow a whitelist of commands.\n\nThis series is based on the merge of 'master' and\n'dl/test-must-fail-fixes-5'. In addition, this series was tested by\nmerging with 'seen~1' (to ignore the reftable failures) to ensure no\nin-flight topics will require more changes.\n\nThe first part can be found here[2]. The second part can be found\nhere[3]. The third part can be found here[4]. The fourth part can be\nfound here[5]. The fifth part can be found here[6].\n\nChanges since v1:\n\n* Add a code comment to force_color()\n\n* Do not allow nested env's in test_must_fail_acceptable()\n\n* Clean up the env-processing case code\n\n* Give an example on how to use `!`.\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/\n[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/\n[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/\n[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/\n[6]: https://lore.kernel.org/git/cover.1588162842.git.liu.denton@gmail.com/\n\nDenton Liu (5):\n t3701: stop using `env` in force_color()\n t5324: reorder `run_with_limited_open_files test_might_fail`\n t7107: don't use test_must_fail()\n t9834: remove use of `test_might_fail p4`\n test-lib-functions: restrict test_must_fail usage\n\n t/t0000-basic.sh | 18 +++++++++++++\n t/t3701-add-interactive.sh | 13 ++++++++--\n t/t5324-split-commit-graph.sh | 2 +-\n t/t7107-reset-pathspec-file.sh | 9 +++++--\n t/t9834-git-p4-file-dir-bug.sh | 2 +-\n t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++\n 6 files changed, 85 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1: 67d5b93fda ! 1: 654c864691 t3701: stop using `env` in force_color()\n @@ t/t3701-add-interactive.sh: diff_cmp () {\n \n force_color () {\n -\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n ++\t# The first element of $@ may be a shell function, as a result POSIX\n ++\t# does not guarantee that \"one-shot assignment\" will not persist after\n ++\t# the function call. Thus, we prevent these variables from escaping\n ++\t# this function's context with this subshell.\n +\t(\n +\t\tGIT_PAGER_IN_USE=true &&\n -+\t\texport GIT_PAGER_IN_USE &&\n +\t\tTERM=vt100 &&\n -+\t\texport TERM &&\n ++\t\texport GIT_PAGER_IN_USE TERM &&\n +\t\t\"$@\"\n +\t)\n }\n2: aae48a89e5 = 2: 9ba997f7c1 t5324: reorder `run_with_limited_open_files test_might_fail`\n3: 74bc29b18b = 3: eb2052bf64 t7107: don't use test_must_fail()\n4: 1287798e69 = 4: 92d3b38428 t9834: remove use of `test_might_fail p4`\n5: 01e29450fe ! 5: 3ebbda6c57 test-lib-functions: restrict test_must_fail usage\n @@ Commit message\n We allow `__git*` as some completion functions return an error code that\n comes from a git invocation. It's good to avoid using test_must_fail\n unnecessarily but it wouldn't hurt to err on the side of caution when\n - we're potentially wrapping a git command (like in these case).\n + we're potentially wrapping a git command (like in these cases).\n \n We also allow `test-tool` and `test-svn-fe` because these are helper\n commands that are written by us and we want to catch their failure.\n @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel\n +'\n +\n +test_expect_success 'test_must_fail on a failing git command with env' '\n -+\ttest_must_fail env var1=a var2=b env var3=c git notacommand\n ++\ttest_must_fail env var1=a var2=b git notacommand\n +'\n +\n +test_expect_success 'test_must_fail rejects a non-git command' '\n @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel\n +'\n +\n +test_expect_success 'test_must_fail rejects a non-git command with env' '\n -+\t! test_must_fail env var1=a var2=b env var3=c grep ^$ notafile 2>err &&\n ++\t! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&\n +\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n +'\n +\n @@ t/test-lib-functions.sh: list_contains () {\n +# and its corresponding variable settings will be stripped before we\n +# test the command being run.\n +test_must_fail_acceptable () {\n -+\twhile test \"$1\" = \"env\"\n -+\tdo\n ++\tif test \"$1\" = \"env\"\n ++\tthen\n +\t\tshift\n +\t\twhile test $# -gt 0\n +\t\tdo\n -+\t\t\tcase \"$1\" in *?=*) ;; *) break ;; esac\n -+\t\t\tshift\n ++\t\t\tcase \"$1\" in\n ++\t\t\t*?=*)\n ++\t\t\t\tshift\n ++\t\t\t\t;;\n ++\t\t\t*)\n ++\t\t\t\tbreak\n ++\t\t\t\t;;\n ++\t\t\tesac\n +\t\tdone\n -+\tdone\n ++\tfi\n +\n +\tcase \"$1\" in\n +\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n @@ t/test-lib-functions.sh: list_contains () {\n +#\n +# test_must_fail grep pattern output\n +#\n -+# Just use '!' instead.\n ++# Instead use '!':\n ++#\n ++# ! grep pattern output\n \n test_must_fail () {\n \tcase \"$1\" in\n @@ t/test-lib-functions.sh: test_must_fail () {\n \tesac\n +\tif ! test_must_fail_acceptable \"$@\"\n +\tthen\n -+\t echo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n -+\t return 1\n ++\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n ++\t\treturn 1\n +\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400864","messageId":"92d3b38428adaf0befc6eecb8a4669306d0d4276.1593576602.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593576601.git.liu.denton@gmail.com","subject":"[PATCH v2 4/5] t9834: remove use of `test_might_fail p4`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-01T04:27:12Z","receivedAt":"2020-07-01T04:27:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\na compound command wrapping the old p4 invocation that always returns 0.\n\nSigned-off-by: Denton Liu \n---\n t/t9834-git-p4-file-dir-bug.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh\nindex 031e1f8668..dac67e89d7 100755\n--- a/t/t9834-git-p4-file-dir-bug.sh\n+++ b/t/t9834-git-p4-file-dir-bug.sh\n@@ -10,7 +10,7 @@ repository.'\n \n test_expect_success 'start p4d' '\n \tstart_p4d &&\n-\ttest_might_fail p4 configure set submit.collision.check=0\n+\t{ p4 configure set submit.collision.check=0 || :; }\n '\n \n test_expect_success 'init depot' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400867","messageId":"9ba997f7c126ed0838ca7a72f33d87f40e173fd8.1593576602.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593576601.git.liu.denton@gmail.com","subject":"[PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-01T04:27:10Z","receivedAt":"2020-07-01T04:27:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_might_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `run_with_limited_open_files` comes before `test_might_fail`. This\nway, `test_might_fail` operates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t5324-split-commit-graph.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 269d0964a3..9b850ea907 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -399,7 +399,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion'\n \t\tfor i in $(test_seq 64)\n \t\tdo\n \t\t\ttest_commit $i &&\n-\t\t\ttest_might_fail run_with_limited_open_files git commit-graph write \\\n+\t\t\trun_with_limited_open_files test_might_fail git commit-graph write \\\n \t\t\t\t--split=no-merge --reachable || return 1\n \t\tdone\n \t)\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400865","messageId":"3ebbda6c57005967b718d834aed7e3eaf00d18b4.1593576602.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593576601.git.liu.denton@gmail.com","subject":"[PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-01T04:27:13Z","receivedAt":"2020-07-01T04:27:35Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In previous commits, we removed the usage of test_must_fail() for most\ncommands except for a set of pre-approved commands. Since that's done,\nonly allow test_must_fail() to run those pre-approved commands.\n\nObviously, we should allow `git`.\n\nWe allow `__git*` as some completion functions return an error code that\ncomes from a git invocation. It's good to avoid using test_must_fail\nunnecessarily but it wouldn't hurt to err on the side of caution when\nwe're potentially wrapping a git command (like in these cases).\n\nWe also allow `test-tool` and `test-svn-fe` because these are helper\ncommands that are written by us and we want to catch their failure.\n\nFinally, we allow `test_terminal` because `test_terminal` just wraps\naround git commands. Also, we cannot rewrite\n`test_must_fail test_terminal` as `test_terminal test_must_fail` because\ntest_must_fail() is a shell function and as a result, it cannot be\ninvoked from the test-terminal Perl script.\n\nWe opted to explicitly list the above tools instead of using a catch-all\nsuch as `test[-_]*` because we want to be as restrictive as possible so\nthat in the future, someone would not accidentally introduce an\nunrelated usage of test_must_fail() on an \"unapproved\" command.\n\nSigned-off-by: Denton Liu \n---\n t/t0000-basic.sh | 18 ++++++++++++++++\n t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2ff176cd5d..90bf1dbc8d 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '\n \ttest $len = 4098\n '\n \n+test_expect_success 'test_must_fail on a failing git command' '\n+\ttest_must_fail git notacommand\n+'\n+\n+test_expect_success 'test_must_fail on a failing git command with env' '\n+\ttest_must_fail env var1=a var2=b git notacommand\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command' '\n+\t! test_must_fail grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command with env' '\n+\t! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3103be8a32..b791933ffd 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -798,6 +798,37 @@ list_contains () {\n \treturn 1\n }\n \n+# Returns success if the arguments indicate that a command should be\n+# accepted by test_must_fail(). If the command is run with env, the env\n+# and its corresponding variable settings will be stripped before we\n+# test the command being run.\n+test_must_fail_acceptable () {\n+\tif test \"$1\" = \"env\"\n+\tthen\n+\t\tshift\n+\t\twhile test $# -gt 0\n+\t\tdo\n+\t\t\tcase \"$1\" in\n+\t\t\t*?=*)\n+\t\t\t\tshift\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tbreak\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n+\tfi\n+\n+\tcase \"$1\" in\n+\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n+\t\treturn 0\n+\t\t;;\n+\t*)\n+\t\treturn 1\n+\t\t;;\n+\tesac\n+}\n+\n # This is not among top-level (test_expect_success | test_expect_failure)\n # but is a prefix that can be used in the test script, like:\n #\n@@ -817,6 +848,17 @@ list_contains () {\n # Multiple signals can be specified as a comma separated list.\n # Currently recognized signal names are: sigpipe, success.\n # (Don't use 'success', use 'test_might_fail' instead.)\n+#\n+# Do not use this to run anything but \"git\" and other specific testable\n+# commands (see test_must_fail_acceptable()). We are not in the\n+# business of vetting system supplied commands -- in other words, this\n+# is wrong:\n+#\n+# test_must_fail grep pattern output\n+#\n+# Instead use '!':\n+#\n+# ! grep pattern output\n \n test_must_fail () {\n \tcase \"$1\" in\n@@ -828,6 +870,11 @@ test_must_fail () {\n \t\t_test_ok=\n \t\t;;\n \tesac\n+\tif ! test_must_fail_acceptable \"$@\"\n+\tthen\n+\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n+\t\treturn 1\n+\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"400866","messageId":"654c864691866f644ff5ec92de5bfa0701645419.1593576602.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593576601.git.liu.denton@gmail.com","subject":"[PATCH v2 1/5] t3701: stop using `env` in force_color()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-01T04:27:09Z","receivedAt":"2020-07-01T04:27:35Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future patch, we plan on making the test_must_fail()-family of\nfunctions accept only git commands. Even though force_color() wraps an\ninvocation of `env git`, test_must_fail() will not be able to figure\nthis out since it will assume that force_color() is just some random\nfunction which is disallowed.\n\nInstead of using `env` in force_color() (which does not support shell\nfunctions), export the environment variables in a subshell. Write the\ninvocation as `force_color test_must_fail git ...` since shell functions\nare now supported.\n\nSigned-off-by: Denton Liu \n---\n t/t3701-add-interactive.sh | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 49decbac71..fb73a847cb 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -31,7 +31,16 @@ diff_cmp () {\n # indicates a dumb terminal, so we set that variable, too.\n \n force_color () {\n-\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n+\t# The first element of $@ may be a shell function, as a result POSIX\n+\t# does not guarantee that \"one-shot assignment\" will not persist after\n+\t# the function call. Thus, we prevent these variables from escaping\n+\t# this function's context with this subshell.\n+\t(\n+\t\tGIT_PAGER_IN_USE=true &&\n+\t\tTERM=vt100 &&\n+\t\texport GIT_PAGER_IN_USE TERM &&\n+\t\t\"$@\"\n+\t)\n }\n \n test_expect_success 'setup (initial)' '\n@@ -604,7 +613,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \techo content >test &&\n \ttest_config interactive.diffFilter \"sed 1d\" &&\n \tprintf y >y &&\n-\ttest_must_fail force_color git add -p \n---\n t/t7107-reset-pathspec-file.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh\nindex cad3a9de9e..15ccb14f7e 100755\n--- a/t/t7107-reset-pathspec-file.sh\n+++ b/t/t7107-reset-pathspec-file.sh\n@@ -22,7 +22,12 @@ restore_checkpoint () {\n \n verify_expect () {\n \tgit status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&\n-\ttest_cmp expect actual\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\t! test_cmp expect actual\n+\telse\n+\t\ttest_cmp expect actual\n+\tfi\n }\n \n test_expect_success '--pathspec-from-file from stdin' '\n@@ -131,7 +136,7 @@ test_expect_success 'quotes not compatible with --pathspec-file-nul' '\n \tcat >expect <<-\\EOF &&\n \t D fileA.t\n \tEOF\n-\ttest_must_fail verify_expect\n+\tverify_expect !\n '\n \n test_expect_success 'only touches what was listed' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401070","messageId":"cover.1594101831.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1593576601.git.liu.denton@gmail.com","subject":"[RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T06:04:33Z","receivedAt":"2020-07-07T06:05:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The overall scope of these patches is to replace inappropriate uses of\ntest_must_fail. IOW, we should only allow test_must_fail to run on `git`\nand `test-tool`. Ultimately, we will conclude by making test_must_fail\nerror out on non-git commands. An advance view of the final series can\nbe found here[1].\n\nThis is the sixth and final(!) part. It cleans up instances of\n`test_must_fail` that were introduced since the effort began. In\naddition, it finally flips the switch and makes test_must_fail only\nallow a whitelist of commands.\n\nThis series is based on the merge of 'master' and\n'dl/test-must-fail-fixes-5'. In addition, this series was tested by\nmerging with 'seen~1' (to ignore the reftable failures) to ensure no\nin-flight topics will require more changes.\n\nThe first part can be found here[2]. The second part can be found\nhere[3]. The third part can be found here[4]. The fourth part can be\nfound here[5]. The fifth part can be found here[6].\n\nChanges since v1:\n\n* Add a code comment to force_color()\n\n* Do not allow nested env's in test_must_fail_acceptable()\n\n* Clean up the env-processing case code\n\n* Give an example on how to use `!`.\n\n[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2\n[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/\n[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/\n[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/\n[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/\n[6]: https://lore.kernel.org/git/cover.1588162842.git.liu.denton@gmail.com/\n\nDenton Liu (5):\n t3701: stop using `env` in force_color()\n t5324: reorder `run_with_limited_open_files test_might_fail`\n t7107: don't use test_must_fail()\n t9834: remove use of `test_might_fail p4`\n test-lib-functions: restrict test_must_fail usage\n\n t/t0000-basic.sh | 18 +++++++++++++\n t/t3701-add-interactive.sh | 13 ++++++++--\n t/t5324-split-commit-graph.sh | 2 +-\n t/t7107-reset-pathspec-file.sh | 9 +++++--\n t/t9834-git-p4-file-dir-bug.sh | 2 +-\n t/test-lib-functions.sh | 47 ++++++++++++++++++++++++++++++++++\n 6 files changed, 85 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n1: 67d5b93fda ! 1: 654c864691 t3701: stop using `env` in force_color()\n @@ t/t3701-add-interactive.sh: diff_cmp () {\n \n force_color () {\n -\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n ++\t# The first element of $@ may be a shell function, as a result POSIX\n ++\t# does not guarantee that \"one-shot assignment\" will not persist after\n ++\t# the function call. Thus, we prevent these variables from escaping\n ++\t# this function's context with this subshell.\n +\t(\n +\t\tGIT_PAGER_IN_USE=true &&\n -+\t\texport GIT_PAGER_IN_USE &&\n +\t\tTERM=vt100 &&\n -+\t\texport TERM &&\n ++\t\texport GIT_PAGER_IN_USE TERM &&\n +\t\t\"$@\"\n +\t)\n }\n2: aae48a89e5 = 2: 9ba997f7c1 t5324: reorder `run_with_limited_open_files test_might_fail`\n3: 74bc29b18b = 3: eb2052bf64 t7107: don't use test_must_fail()\n4: 1287798e69 = 4: 92d3b38428 t9834: remove use of `test_might_fail p4`\n5: 01e29450fe ! 5: 3ebbda6c57 test-lib-functions: restrict test_must_fail usage\n @@ Commit message\n We allow `__git*` as some completion functions return an error code that\n comes from a git invocation. It's good to avoid using test_must_fail\n unnecessarily but it wouldn't hurt to err on the side of caution when\n - we're potentially wrapping a git command (like in these case).\n + we're potentially wrapping a git command (like in these cases).\n \n We also allow `test-tool` and `test-svn-fe` because these are helper\n commands that are written by us and we want to catch their failure.\n @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel\n +'\n +\n +test_expect_success 'test_must_fail on a failing git command with env' '\n -+\ttest_must_fail env var1=a var2=b env var3=c git notacommand\n ++\ttest_must_fail env var1=a var2=b git notacommand\n +'\n +\n +test_expect_success 'test_must_fail rejects a non-git command' '\n @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel\n +'\n +\n +test_expect_success 'test_must_fail rejects a non-git command with env' '\n -+\t! test_must_fail env var1=a var2=b env var3=c grep ^$ notafile 2>err &&\n ++\t! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&\n +\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n +'\n +\n @@ t/test-lib-functions.sh: list_contains () {\n +# and its corresponding variable settings will be stripped before we\n +# test the command being run.\n +test_must_fail_acceptable () {\n -+\twhile test \"$1\" = \"env\"\n -+\tdo\n ++\tif test \"$1\" = \"env\"\n ++\tthen\n +\t\tshift\n +\t\twhile test $# -gt 0\n +\t\tdo\n -+\t\t\tcase \"$1\" in *?=*) ;; *) break ;; esac\n -+\t\t\tshift\n ++\t\t\tcase \"$1\" in\n ++\t\t\t*?=*)\n ++\t\t\t\tshift\n ++\t\t\t\t;;\n ++\t\t\t*)\n ++\t\t\t\tbreak\n ++\t\t\t\t;;\n ++\t\t\tesac\n +\t\tdone\n -+\tdone\n ++\tfi\n +\n +\tcase \"$1\" in\n +\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n @@ t/test-lib-functions.sh: list_contains () {\n +#\n +# test_must_fail grep pattern output\n +#\n -+# Just use '!' instead.\n ++# Instead use '!':\n ++#\n ++# ! grep pattern output\n \n test_must_fail () {\n \tcase \"$1\" in\n @@ t/test-lib-functions.sh: test_must_fail () {\n \tesac\n +\tif ! test_must_fail_acceptable \"$@\"\n +\tthen\n -+\t echo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n -+\t return 1\n ++\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n ++\t\treturn 1\n +\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401071","messageId":"654c864691866f644ff5ec92de5bfa0701645419.1594101831.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1594101831.git.liu.denton@gmail.com","subject":"[RESEND PATCH v2 1/5] t3701: stop using `env` in force_color()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T06:04:34Z","receivedAt":"2020-07-07T06:05:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future patch, we plan on making the test_must_fail()-family of\nfunctions accept only git commands. Even though force_color() wraps an\ninvocation of `env git`, test_must_fail() will not be able to figure\nthis out since it will assume that force_color() is just some random\nfunction which is disallowed.\n\nInstead of using `env` in force_color() (which does not support shell\nfunctions), export the environment variables in a subshell. Write the\ninvocation as `force_color test_must_fail git ...` since shell functions\nare now supported.\n\nSigned-off-by: Denton Liu \n---\n t/t3701-add-interactive.sh | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 49decbac71..fb73a847cb 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -31,7 +31,16 @@ diff_cmp () {\n # indicates a dumb terminal, so we set that variable, too.\n \n force_color () {\n-\tenv GIT_PAGER_IN_USE=true TERM=vt100 \"$@\"\n+\t# The first element of $@ may be a shell function, as a result POSIX\n+\t# does not guarantee that \"one-shot assignment\" will not persist after\n+\t# the function call. Thus, we prevent these variables from escaping\n+\t# this function's context with this subshell.\n+\t(\n+\t\tGIT_PAGER_IN_USE=true &&\n+\t\tTERM=vt100 &&\n+\t\texport GIT_PAGER_IN_USE TERM &&\n+\t\t\"$@\"\n+\t)\n }\n \n test_expect_success 'setup (initial)' '\n@@ -604,7 +613,7 @@ test_expect_success 'detect bogus diffFilter output' '\n \techo content >test &&\n \ttest_config interactive.diffFilter \"sed 1d\" &&\n \tprintf y >y &&\n-\ttest_must_fail force_color git add -p \n---\n t/t0000-basic.sh | 18 ++++++++++++++++\n t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 2ff176cd5d..90bf1dbc8d 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '\n \ttest $len = 4098\n '\n \n+test_expect_success 'test_must_fail on a failing git command' '\n+\ttest_must_fail git notacommand\n+'\n+\n+test_expect_success 'test_must_fail on a failing git command with env' '\n+\ttest_must_fail env var1=a var2=b git notacommand\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command' '\n+\t! test_must_fail grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n+test_expect_success 'test_must_fail rejects a non-git command with env' '\n+\t! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&\n+\tgrep -F \"test_must_fail: only '\"'\"'git'\"'\"' is allowed\" err\n+'\n+\n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 3103be8a32..b791933ffd 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -798,6 +798,37 @@ list_contains () {\n \treturn 1\n }\n \n+# Returns success if the arguments indicate that a command should be\n+# accepted by test_must_fail(). If the command is run with env, the env\n+# and its corresponding variable settings will be stripped before we\n+# test the command being run.\n+test_must_fail_acceptable () {\n+\tif test \"$1\" = \"env\"\n+\tthen\n+\t\tshift\n+\t\twhile test $# -gt 0\n+\t\tdo\n+\t\t\tcase \"$1\" in\n+\t\t\t*?=*)\n+\t\t\t\tshift\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tbreak\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n+\tfi\n+\n+\tcase \"$1\" in\n+\tgit|__git*|test-tool|test-svn-fe|test_terminal)\n+\t\treturn 0\n+\t\t;;\n+\t*)\n+\t\treturn 1\n+\t\t;;\n+\tesac\n+}\n+\n # This is not among top-level (test_expect_success | test_expect_failure)\n # but is a prefix that can be used in the test script, like:\n #\n@@ -817,6 +848,17 @@ list_contains () {\n # Multiple signals can be specified as a comma separated list.\n # Currently recognized signal names are: sigpipe, success.\n # (Don't use 'success', use 'test_might_fail' instead.)\n+#\n+# Do not use this to run anything but \"git\" and other specific testable\n+# commands (see test_must_fail_acceptable()). We are not in the\n+# business of vetting system supplied commands -- in other words, this\n+# is wrong:\n+#\n+# test_must_fail grep pattern output\n+#\n+# Instead use '!':\n+#\n+# ! grep pattern output\n \n test_must_fail () {\n \tcase \"$1\" in\n@@ -828,6 +870,11 @@ test_must_fail () {\n \t\t_test_ok=\n \t\t;;\n \tesac\n+\tif ! test_must_fail_acceptable \"$@\"\n+\tthen\n+\t\techo >&7 \"test_must_fail: only 'git' is allowed: $*\"\n+\t\treturn 1\n+\tfi\n \t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401073","messageId":"92d3b38428adaf0befc6eecb8a4669306d0d4276.1594101831.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1594101831.git.liu.denton@gmail.com","subject":"[RESEND PATCH v2 4/5] t9834: remove use of `test_might_fail p4`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T06:04:37Z","receivedAt":"2020-07-07T06:05:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The test_must_fail() family of functions (including test_might_fail())\nshould only be used on git commands. Replace test_might_fail() with\na compound command wrapping the old p4 invocation that always returns 0.\n\nSigned-off-by: Denton Liu \n---\n t/t9834-git-p4-file-dir-bug.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh\nindex 031e1f8668..dac67e89d7 100755\n--- a/t/t9834-git-p4-file-dir-bug.sh\n+++ b/t/t9834-git-p4-file-dir-bug.sh\n@@ -10,7 +10,7 @@ repository.'\n \n test_expect_success 'start p4d' '\n \tstart_p4d &&\n-\ttest_might_fail p4 configure set submit.collision.check=0\n+\t{ p4 configure set submit.collision.check=0 || :; }\n '\n \n test_expect_success 'init depot' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401075","messageId":"eb2052bf644cc10b5dd8975fd75542916c1749fa.1594101831.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1594101831.git.liu.denton@gmail.com","subject":"[RESEND PATCH v2 3/5] t7107: don't use test_must_fail()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T06:04:36Z","receivedAt":"2020-07-07T06:05:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We had a `test_must_fail verify_expect`. However, the git command in\nverify_expect() was not expected to fail; the test_cmp() was the failing\ncommand. Be more precise about testing failure by accepting an optional\nfirst argument of '!' which causes the result of the file comparison to\nbe negated.\n\nSigned-off-by: Denton Liu \n---\n t/t7107-reset-pathspec-file.sh | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh\nindex cad3a9de9e..15ccb14f7e 100755\n--- a/t/t7107-reset-pathspec-file.sh\n+++ b/t/t7107-reset-pathspec-file.sh\n@@ -22,7 +22,12 @@ restore_checkpoint () {\n \n verify_expect () {\n \tgit status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&\n-\ttest_cmp expect actual\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\t! test_cmp expect actual\n+\telse\n+\t\ttest_cmp expect actual\n+\tfi\n }\n \n test_expect_success '--pathspec-from-file from stdin' '\n@@ -131,7 +136,7 @@ test_expect_success 'quotes not compatible with --pathspec-file-nul' '\n \tcat >expect <<-\\EOF &&\n \t D fileA.t\n \tEOF\n-\ttest_must_fail verify_expect\n+\tverify_expect !\n '\n \n test_expect_success 'only touches what was listed' '\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401074","messageId":"9ba997f7c126ed0838ca7a72f33d87f40e173fd8.1594101831.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"cover.1594101831.git.liu.denton@gmail.com","subject":"[RESEND PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T06:04:35Z","receivedAt":"2020-07-07T06:05:13Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we plan on only allowing `test_might_fail` to work on a\nrestricted subset of commands, including `git`. Reorder the commands so\nthat `run_with_limited_open_files` comes before `test_might_fail`. This\nway, `test_might_fail` operates on a git command.\n\nSigned-off-by: Denton Liu \n---\n t/t5324-split-commit-graph.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 269d0964a3..9b850ea907 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -399,7 +399,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion'\n \t\tfor i in $(test_seq 64)\n \t\tdo\n \t\t\ttest_commit $i &&\n-\t\t\ttest_might_fail run_with_limited_open_files git commit-graph write \\\n+\t\t\trun_with_limited_open_files test_might_fail git commit-graph write \\\n \t\t\t\t--split=no-merge --reachable || return 1\n \t\tdone\n \t)\n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401152","messageId":"xmqqblkr3x6q.fsf@gitster.c.googlers.com","threadId":"53777","inReplyTo":"cover.1594101831.git.liu.denton@gmail.com","subject":"Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T20:08:29Z","receivedAt":"2020-07-07T20:08:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> The overall scope of these patches is to replace inappropriate uses of\n> test_must_fail. IOW, we should only allow test_must_fail to run on `git`\n> and `test-tool`. Ultimately, we will conclude by making test_must_fail\n> error out on non-git commands. An advance view of the final series can\n> be found here[1].\n>\n> This is the sixth and final(!) part. It cleans up instances of\n> `test_must_fail` that were introduced since the effort began. In\n> addition, it finally flips the switch and makes test_must_fail only\n> allow a whitelist of commands.\n>\n> This series is based on the merge of 'master' and\n> 'dl/test-must-fail-fixes-5'. In addition, this series was tested by\n> merging with 'seen~1' (to ignore the reftable failures) to ensure no\n> in-flight topics will require more changes.\n>\n> The first part can be found here[2]. The second part can be found\n> here[3]. The third part can be found here[4]. The fourth part can be\n> found here[5]. The fifth part can be found here[6].\n>\n> Changes since v1:\n>\n> * Add a code comment to force_color()\n>\n> * Do not allow nested env's in test_must_fail_acceptable()\n>\n> * Clean up the env-processing case code\n>\n> * Give an example on how to use `!`.\n\nThanks for a resend. Now part #5 is in 'master', I can queue these\ndirectly on top.\n"},{"id":"401154","messageId":"xmqq7dvf3uwc.fsf@gitster.c.googlers.com","threadId":"53777","inReplyTo":"xmqqblkr3x6q.fsf@gitster.c.googlers.com","subject":"Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T20:57:55Z","receivedAt":"2020-07-07T20:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Changes since v1:\n>>\n>> * Add a code comment to force_color()\n>>\n>> * Do not allow nested env's in test_must_fail_acceptable()\n>>\n>> * Clean up the env-processing case code\n>>\n>> * Give an example on how to use `!`.\n>\n> Thanks for a resend. Now part #5 is in 'master', I can queue these\n> directly on top.\n\nIt seems that the patch series lacks coverage for t9400 where we\nhave\n\ntest_expect_success 'cvs server does not run with vanilla git-shell' '\n\t(\n\t\tcd cvswork &&\n\t\tCVS_SERVER=$WORKDIR/remote-cvs &&\n\t\texport CVS_SERVER &&\n\t\ttest_must_fail cvs log merge\n\t)\n'\n\nwhich obviously needs to be converted before we declare that it is a\nhard error to feed a non-git command to test_must_fail.\n\n\n"},{"id":"401161","messageId":"4ca5e1f9c06ed509fc3165c550d0d665dd5b69c0.1594159495.git.liu.denton@gmail.com","threadId":"53777","inReplyTo":"xmqq7dvf3uwc.fsf@gitster.c.googlers.com","subject":"[PATCH] t9400: don't use test_must_fail with cvs","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T22:08:07Z","receivedAt":"2020-07-07T22:08:21Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We are using `test_must_fail cvs` to test that the cvs command fails as\nexpected. However, test_must_fail() is used to ensure that commands fail\nin an expected way, not due to something like a segv. Since we are not\nin the business of verifying the sanity of the external world, replace\n`test_must_fail cvs` with `! cvs` and assume that the cvs command does\nnot die unexpectedly.\n\nSigned-off-by: Denton Liu \n---\nHi Junio,\n\nThanks for pointing this out. It seems like the CI systems we use don't\ncover cvs. Is this something that we want to address or is it fine to\nleave it as a blindspot since no one really uses cvs anymore?\n\n t/t9400-git-cvsserver-server.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex a5e5dca753..4a46f31c41 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -603,7 +603,7 @@ test_expect_success 'cvs server does not run with vanilla git-shell' '\n \t\tcd cvswork &&\n \t\tCVS_SERVER=$WORKDIR/remote-cvs &&\n \t\texport CVS_SERVER &&\n-\t\ttest_must_fail cvs log merge\n+\t\t! cvs log merge\n \t)\n '\n \n-- \n2.27.0.383.g050319c2ae\n\n"},{"id":"401162","messageId":"xmqqwo3f2cx6.fsf@gitster.c.googlers.com","threadId":"53777","inReplyTo":"xmqq7dvf3uwc.fsf@gitster.c.googlers.com","subject":"Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T22:11:33Z","receivedAt":"2020-07-07T22:11:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>>> Changes since v1:\n>>>\n>>> * Add a code comment to force_color()\n>>>\n>>> * Do not allow nested env's in test_must_fail_acceptable()\n>>>\n>>> * Clean up the env-processing case code\n>>>\n>>> * Give an example on how to use `!`.\n>>\n>> Thanks for a resend. Now part #5 is in 'master', I can queue these\n>> directly on top.\n>\n> It seems that the patch series lacks coverage for t9400 where we\n> have\n>\n> test_expect_success 'cvs server does not run with vanilla git-shell' '\n> \t(\n> \t\tcd cvswork &&\n> \t\tCVS_SERVER=$WORKDIR/remote-cvs &&\n> \t\texport CVS_SERVER &&\n> \t\ttest_must_fail cvs log merge\n> \t)\n> '\n>\n> which obviously needs to be converted before we declare that it is a\n> hard error to feed a non-git command to test_must_fail.\n\nFor today's integration cycle, I added a fix-up at the tip of the topic\n\nhttps://github.com/git/git/commit/dde09ce2b7dd62eafda6339c1c31ccfeb0f39cee\n"},{"id":"401164","messageId":"20200707222155.GA27311@generichostname","threadId":"53777","inReplyTo":"xmqqwo3f2cx6.fsf@gitster.c.googlers.com","subject":"Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-07-07T22:21:55Z","receivedAt":"2020-07-07T22:22:00Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Tue, Jul 07, 2020 at 03:11:33PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> > It seems that the patch series lacks coverage for t9400 where we\n> > have\n> >\n> > test_expect_success 'cvs server does not run with vanilla git-shell' '\n> > \t(\n> > \t\tcd cvswork &&\n> > \t\tCVS_SERVER=$WORKDIR/remote-cvs &&\n> > \t\texport CVS_SERVER &&\n> > \t\ttest_must_fail cvs log merge\n> > \t)\n> > '\n> >\n> > which obviously needs to be converted before we declare that it is a\n> > hard error to feed a non-git command to test_must_fail.\n> \n> For today's integration cycle, I added a fix-up at the tip of the topic\n> \n> https://github.com/git/git/commit/dde09ce2b7dd62eafda6339c1c31ccfeb0f39cee\n\nThanks. For the final version of this series, we should either queue\nyour patch or the one I just sent[0] just after the patch for t7107 so\nthat it comes before the we flip the switch on test_must_fail and also\nso that the patches show up in increasing numerical order.\n\n[0]: https://lore.kernel.org/git/4ca5e1f9c06ed509fc3165c550d0d665dd5b69c0.1594159495.git.liu.denton@gmail.com/\n"},{"id":"401165","messageId":"xmqqo8or2c2z.fsf@gitster.c.googlers.com","threadId":"53777","inReplyTo":"20200707222155.GA27311@generichostname","subject":"Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-07T22:29:40Z","receivedAt":"2020-07-07T22:29:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Thanks. For the final version of this series, we should either queue\n> your patch or the one I just sent[0] just after the patch for t7107 so\n> that it comes before the we flip the switch on test_must_fail and also\n> so that the patches show up in increasing numerical order.\n\nAh, our mails crossed. Surely the fix to cvs test must come before\nthe final step that tightens the test_must_fail helper.\n\n"}]}