{"thread":{"id":"59065","subject":"[PATCH] t/interop: allow tests to run \"git env--helper\"","startedAt":"2023-01-10T13:40:33Z","lastAt":"2023-01-15T02:08:05Z","messageCount":6,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470070","messageId":"Y71qiCs+oAS2OegH@coredump.intra.peff.net","threadId":"59065","inReplyTo":null,"subject":"[PATCH] t/interop: allow tests to run \"git env--helper\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-10T13:39:20Z","receivedAt":"2023-01-10T13:40:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The interop test library sets up wrappers \"git.a\" and \"git.b\" to\nrepresent the two versions to be tested. It also wraps vanilla \"git\" to\nreport an error, with the goal of catching tests which accidentally fail\nto use one of the version-specific wrappers (which could invalidate the\ntests in a very subtle way).\n\nAs a result, t/interop/i5700 has refused to run since 3b072c577b (tests:\nreplace test_tristate with \"git env--helper\", 2019-06-21). The problem\nis that lib-git-daemon.sh uses \"git env--helper\" to decide whether to\nrun daemon tests, which triggers the vanilla wrapper. That produces an\nerror, and we think that the daemon tests are disabled.\n\nLet's make our wrapper a little smarter, and allow env--helper\nspecifically. It's not an interesting part of Git to test for interop,\nand it's used extensively in test setup. The matching is rudimentary\n(e.g., it would not catch \"git --some-arg env--helper\", but it's enough\nfor our small set of interop tests).\n\nLet's likewise improve the error message from the wrapper (when it does\ncomplain) to show the arguments to git. That makes debugging a situation\nlike this much easier, since otherwise you are clueless about who is\ncalling \"git\" and why.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously nobody has run these for a while. I just happened to do so\ntoday while investigating something else. Maybe they're not worth\nkeeping around, and we should consider it all a failed experiment.\nBut it's easy enough to fix in the meantime.\n\nArguably \"git env--helper\" should be \"test-tool\", which wouldn't run\ninto this problem, but it's probably not worth refactoring it for the\nsake of these tests.\n\nBy the way, if you do try to run the whole set of tests, note that Git\nv1.0.0 no longer builds with modern openssl (you can't declare a BIGNUM\nanymore; you have to get it from BN_new()), and thus i5500 fails. You\ncan work around it with:\n\n  # this gets baked into GIT-BUILD-OPTIONS\n  make GIT_INTEROP_MAKE_OPTS=NO_OPENSSL=Nope\n\n  # and now interop tests will use it for building other versions\n  cd t/interop\n  make\n\n t/interop/interop-lib.sh | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh\nindex 3e0a2911d4..f65baa607d 100644\n--- a/t/interop/interop-lib.sh\n+++ b/t/interop/interop-lib.sh\n@@ -67,9 +67,18 @@ generate_wrappers () {\n \tmkdir -p .bin &&\n \twrap_git .bin/git.a \"$DIR_A\" &&\n \twrap_git .bin/git.b \"$DIR_B\" &&\n+\tGENERIC_GIT=\"$TEST_DIRECTORY/../bin-wrappers/git\" &&\n+\texport GENERIC_GIT &&\n \twrite_script .bin/git <<-\\EOF &&\n-\techo >&2 fatal: test tried to run generic git\n-\texit 1\n+\tcase \"$1\" in\n+\tenv--helper)\n+\t\texec \"$GENERIC_GIT\" \"$@\"\n+\t\t;;\n+\t*)\n+\t\techo >&2 fatal: test tried to run generic git: $*\n+\t\texit 1\n+\t\t;;\n+\tesac\n \tEOF\n \tPATH=$(pwd)/.bin:$PATH\n }\n-- \n2.39.0.508.g93b13bde48\n"},{"id":"470204","messageId":"patch-1.1-e662c570f1d-20230112T155226Z-avarab@gmail.com","threadId":"59065","inReplyTo":"Y71qiCs+oAS2OegH@coredump.intra.peff.net","subject":"[PATCH] env-helper: move this built-in to to \"test-tool env-helper\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T16:03:21Z","receivedAt":"2023-01-12T16:11:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since [1] there has been no reason for keeping \"git env--helper\" a\nbuilt-in. The reason it was a built-in to begin with was to support\nthe GIT_TEST_GETTEXT_POISON mode removed in that commit. I.e. unlike\nthe rest of \"test-tool\" it would potentially be called by the\ninstalled git via \"git-sh-i18n.sh\".\n\nAs none of that applies since [1] we should stop carrying this\ntechnical debt, and move it to t/helper/*. As this mostly move-only\nchange shows this has the nice bonus that we'll stop wasting time\ntranslating the internal-only strings it emits.\n\nEven though this was a built-in, it was intentionally never\ndocumented, see its introduction in [2]. It never saw use outside of\nthe test suite, except for the \"GIT_TEST_GETTEXT_POISON\" use-case\nnoted above.\n\n1. d162b25f956 (tests: remove support for GIT_TEST_GETTEXT_POISON,\n   2021-01-20)\n2. b4f207f3394 (env--helper: new undocumented builtin wrapping\n   git_env_*(), 2019-06-21)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Tue, Jan 10 2023, Jeff King wrote:\n\n> The interop test library sets up wrappers \"git.a\" and \"git.b\" to\n> represent the two versions to be tested. It also wraps vanilla \"git\" to\n> report an error, with the goal of catching tests which accidentally fail\n> to use one of the version-specific wrappers (which could invalidate the\n> tests in a very subtle way).\n>\n> As a result, t/interop/i5700 has refused to run since 3b072c577b (tests:\n> replace test_tristate with \"git env--helper\", 2019-06-21). The problem\n> is that lib-git-daemon.sh uses \"git env--helper\" to decide whether to\n> run daemon tests, which triggers the vanilla wrapper. That produces an\n> error, and we think that the daemon tests are disabled.\n>\n> Let's make our wrapper a little smarter, and allow env--helper\n> specifically. It's not an interesting part of Git to test for interop,\n> and it's used extensively in test setup. The matching is rudimentary\n> (e.g., it would not catch \"git --some-arg env--helper\", but it's enough\n> for our small set of interop tests).\n>\n> Let's likewise improve the error message from the wrapper (when it does\n> complain) to show the arguments to git. That makes debugging a situation\n> like this much easier, since otherwise you are clueless about who is\n> calling \"git\" and why.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Obviously nobody has run these for a while. I just happened to do so\n> today while investigating something else. Maybe they're not worth\n> keeping around, and we should consider it all a failed experiment.\n> But it's easy enough to fix in the meantime.\n>\n> Arguably \"git env--helper\" should be \"test-tool\", which wouldn't run\n> into this problem, but it's probably not worth refactoring it for the\n> sake of these tests.\n\nI think it's worth doing that, i.e. to take this alternate\napproach. When I removed the GIT_TEST_GETTEXT_POISON facility I didn't\nwant to make that series any larger, and keeping it a built-in seemed\nharmless, as there wasn't any practical difference between an\nundocumented built-in and a test-tool.\n\nBut as your patch shows there is, I think we should just pay down that\ntechnical debt, rather than adding to it by accumulating workarounds\n(however small those are...).\n\n .gitignore                                    |  1 -\n Makefile                                      |  2 +-\n git.c                                         |  1 -\n .../helper/test-env-helper.c                  | 24 +++----\n t/helper/test-tool.c                          |  1 +\n t/helper/test-tool.h                          |  1 +\n t/t0017-env-helper.sh                         | 62 +++++++++----------\n t/test-lib-functions.sh                       |  2 +-\n t/test-lib.sh                                 |  6 +-\n 9 files changed, 50 insertions(+), 50 deletions(-)\n rename builtin/env--helper.c => t/helper/test-env-helper.c (71%)\n\ndiff --git a/.gitignore b/.gitignore\nindex e942a83b45b..6782f3cecac 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -59,7 +59,6 @@\n /git-difftool\n /git-difftool--helper\n /git-describe\n-/git-env--helper\n /git-fast-export\n /git-fast-import\n /git-fetch\ndiff --git a/Makefile b/Makefile\nindex db447d07383..f2f342683c1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -799,6 +799,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-example-decorate.o\n+TEST_BUILTINS_OBJS += test-env-helper.o\n TEST_BUILTINS_OBJS += test-fast-rebase.o\n TEST_BUILTINS_OBJS += test-fsmonitor-client.o\n TEST_BUILTINS_OBJS += test-genrandom.o\n@@ -1231,7 +1232,6 @@ BUILTIN_OBJS += builtin/diff-index.o\n BUILTIN_OBJS += builtin/diff-tree.o\n BUILTIN_OBJS += builtin/diff.o\n BUILTIN_OBJS += builtin/difftool.o\n-BUILTIN_OBJS += builtin/env--helper.o\n BUILTIN_OBJS += builtin/fast-export.o\n BUILTIN_OBJS += builtin/fast-import.o\n BUILTIN_OBJS += builtin/fetch-pack.o\ndiff --git a/git.c b/git.c\nindex 32a5be68a17..96b0a2837dc 100644\n--- a/git.c\n+++ b/git.c\n@@ -507,7 +507,6 @@ static struct cmd_struct commands[] = {\n \t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },\n \t{ \"diff-tree\", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },\n \t{ \"difftool\", cmd_difftool, RUN_SETUP_GENTLY },\n-\t{ \"env--helper\", cmd_env__helper },\n \t{ \"fast-export\", cmd_fast_export, RUN_SETUP },\n \t{ \"fast-import\", cmd_fast_import, RUN_SETUP | NO_PARSEOPT },\n \t{ \"fetch\", cmd_fetch, RUN_SETUP },\ndiff --git a/builtin/env--helper.c b/t/helper/test-env-helper.c\nsimilarity index 71%\nrename from builtin/env--helper.c\nrename to t/helper/test-env-helper.c\nindex ea04c166364..66c88b8ff3d 100644\n--- a/builtin/env--helper.c\n+++ b/t/helper/test-env-helper.c\n@@ -1,9 +1,9 @@\n-#include \"builtin.h\"\n+#include \"test-tool.h\"\n #include \"config.h\"\n #include \"parse-options.h\"\n \n static char const * const env__helper_usage[] = {\n-\tN_(\"git env--helper --type=[bool|ulong] <options> <env-var>\"),\n+\t\"test-tool env-helper --type=[bool|ulong] <options> <env-var>\",\n \tNULL\n };\n \n@@ -24,12 +24,12 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \telse if (!strcmp(arg, \"ulong\"))\n \t\t*cmdmode = ENV_HELPER_TYPE_ULONG;\n \telse\n-\t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n+\t\tdie(\"unrecognized --type argument, %s\", arg);\n \n \treturn 0;\n }\n \n-int cmd_env__helper(int argc, const char **argv, const char *prefix)\n+int cmd__env_helper(int argc, const char **argv)\n {\n \tint exit_code = 0;\n \tconst char *env_variable = NULL;\n@@ -39,17 +39,17 @@ int cmd_env__helper(int argc, const char **argv, const char *prefix)\n \tunsigned long ret_ulong, default_ulong;\n \tenum cmdmode cmdmode = 0;\n \tstruct option opts[] = {\n-\t\tOPT_CALLBACK_F(0, \"type\", &cmdmode, N_(\"type\"),\n-\t\t\t       N_(\"value is given this type\"), PARSE_OPT_NONEG,\n+\t\tOPT_CALLBACK_F(0, \"type\", &cmdmode, \"type\",\n+\t\t\t       \"value is given this type\", PARSE_OPT_NONEG,\n \t\t\t       option_parse_type),\n-\t\tOPT_STRING(0, \"default\", &env_default, N_(\"value\"),\n-\t\t\t   N_(\"default for git_env_*(...) to fall back on\")),\n+\t\tOPT_STRING(0, \"default\", &env_default, \"value\",\n+\t\t\t   \"default for git_env_*(...) to fall back on\"),\n \t\tOPT_BOOL(0, \"exit-code\", &exit_code,\n-\t\t\t N_(\"be quiet only use git_env_*() value as exit code\")),\n+\t\t\t \"be quiet only use git_env_*() value as exit code\"),\n \t\tOPT_END(),\n \t};\n \n-\targc = parse_options(argc, argv, prefix, opts, env__helper_usage,\n+\targc = parse_options(argc, argv, NULL, opts, env__helper_usage,\n \t\t\t     PARSE_OPT_KEEP_UNKNOWN_OPT);\n \tif (env_default && !*env_default)\n \t\tusage_with_options(env__helper_usage, opts);\n@@ -64,7 +64,7 @@ int cmd_env__helper(int argc, const char **argv, const char *prefix)\n \t\tif (env_default) {\n \t\t\tdefault_int = git_parse_maybe_bool(env_default);\n \t\t\tif (default_int == -1) {\n-\t\t\t\terror(_(\"option `--default' expects a boolean value with `--type=bool`, not `%s`\"),\n+\t\t\t\terror(\"option `--default' expects a boolean value with `--type=bool`, not `%s`\",\n \t\t\t\t      env_default);\n \t\t\t\tusage_with_options(env__helper_usage, opts);\n \t\t\t}\n@@ -79,7 +79,7 @@ int cmd_env__helper(int argc, const char **argv, const char *prefix)\n \tcase ENV_HELPER_TYPE_ULONG:\n \t\tif (env_default) {\n \t\t\tif (!git_parse_ulong(env_default, &default_ulong)) {\n-\t\t\t\terror(_(\"option `--default' expects an unsigned long value with `--type=ulong`, not `%s`\"),\n+\t\t\t\terror(\"option `--default' expects an unsigned long value with `--type=ulong`, not `%s`\",\n \t\t\t\t      env_default);\n \t\t\t\tusage_with_options(env__helper_usage, opts);\n \t\t\t}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 7eb1a26a305..abe8a785eb6 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -28,6 +28,7 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-fsmonitor\", cmd__dump_fsmonitor },\n \t{ \"dump-split-index\", cmd__dump_split_index },\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n+\t{ \"env-helper\", cmd__env_helper },\n \t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"fast-rebase\", cmd__fast_rebase },\n \t{ \"fsmonitor-client\", cmd__fsmonitor_client },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 2e20a16eb82..ea2672436c9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -22,6 +22,7 @@ int cmd__dump_fsmonitor(int argc, const char **argv);\n int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n int cmd__dump_reftable(int argc, const char **argv);\n+int cmd__env_helper(int argc, const char **argv);\n int cmd__example_decorate(int argc, const char **argv);\n int cmd__fast_rebase(int argc, const char **argv);\n int cmd__fsmonitor_client(int argc, const char **argv);\ndiff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh\nindex 2e42fba9567..fc14ba091cb 100755\n--- a/t/t0017-env-helper.sh\n+++ b/t/t0017-env-helper.sh\n@@ -1,87 +1,87 @@\n #!/bin/sh\n \n-test_description='test env--helper'\n+test_description='test test-tool env-helper'\n \n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n \n-test_expect_success 'env--helper usage' '\n-\ttest_must_fail git env--helper &&\n-\ttest_must_fail git env--helper --type=bool &&\n-\ttest_must_fail git env--helper --type=ulong &&\n-\ttest_must_fail git env--helper --type=bool &&\n-\ttest_must_fail git env--helper --type=bool --default &&\n-\ttest_must_fail git env--helper --type=bool --default= &&\n-\ttest_must_fail git env--helper --defaultxyz\n+test_expect_success 'test-tool env-helper usage' '\n+\ttest_must_fail test-tool env-helper &&\n+\ttest_must_fail test-tool env-helper --type=bool &&\n+\ttest_must_fail test-tool env-helper --type=ulong &&\n+\ttest_must_fail test-tool env-helper --type=bool &&\n+\ttest_must_fail test-tool env-helper --type=bool --default &&\n+\ttest_must_fail test-tool env-helper --type=bool --default= &&\n+\ttest_must_fail test-tool env-helper --defaultxyz\n '\n \n-test_expect_success 'env--helper bad default values' '\n-\ttest_must_fail git env--helper --type=bool --default=1xyz MISSING &&\n-\ttest_must_fail git env--helper --type=ulong --default=1xyz MISSING\n+test_expect_success 'test-tool env-helper bad default values' '\n+\ttest_must_fail test-tool env-helper --type=bool --default=1xyz MISSING &&\n+\ttest_must_fail test-tool env-helper --type=ulong --default=1xyz MISSING\n '\n \n-test_expect_success 'env--helper --type=bool' '\n+test_expect_success 'test-tool env-helper --type=bool' '\n \t# Test various --default bool values\n \techo true >expected &&\n-\tgit env--helper --type=bool --default=1 MISSING >actual &&\n+\ttest-tool env-helper --type=bool --default=1 MISSING >actual &&\n \ttest_cmp expected actual &&\n-\tgit env--helper --type=bool --default=yes MISSING >actual &&\n+\ttest-tool env-helper --type=bool --default=yes MISSING >actual &&\n \ttest_cmp expected actual &&\n-\tgit env--helper --type=bool --default=true MISSING >actual &&\n+\ttest-tool env-helper --type=bool --default=true MISSING >actual &&\n \ttest_cmp expected actual &&\n \techo false >expected &&\n-\ttest_must_fail git env--helper --type=bool --default=0 MISSING >actual &&\n+\ttest_must_fail test-tool env-helper --type=bool --default=0 MISSING >actual &&\n \ttest_cmp expected actual &&\n-\ttest_must_fail git env--helper --type=bool --default=no MISSING >actual &&\n+\ttest_must_fail test-tool env-helper --type=bool --default=no MISSING >actual &&\n \ttest_cmp expected actual &&\n-\ttest_must_fail git env--helper --type=bool --default=false MISSING >actual &&\n+\ttest_must_fail test-tool env-helper --type=bool --default=false MISSING >actual &&\n \ttest_cmp expected actual &&\n \n \t# No output with --exit-code\n-\tgit env--helper --type=bool --default=true --exit-code MISSING >actual.out 2>actual.err &&\n+\ttest-tool env-helper --type=bool --default=true --exit-code MISSING >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err &&\n-\ttest_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n+\ttest_must_fail test-tool env-helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err &&\n \n \t# Existing variable\n-\tEXISTS=true git env--helper --type=bool --default=false --exit-code EXISTS >actual.out 2>actual.err &&\n+\tEXISTS=true test-tool env-helper --type=bool --default=false --exit-code EXISTS >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err &&\n \ttest_must_fail \\\n \t\tenv EXISTS=false \\\n-\t\tgit env--helper --type=bool --default=true --exit-code EXISTS >actual.out 2>actual.err &&\n+\t\ttest-tool env-helper --type=bool --default=true --exit-code EXISTS >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err\n '\n \n-test_expect_success 'env--helper --type=ulong' '\n+test_expect_success 'test-tool env-helper --type=ulong' '\n \techo 1234567890 >expected &&\n-\tgit env--helper --type=ulong --default=1234567890 MISSING >actual.out 2>actual.err &&\n+\ttest-tool env-helper --type=ulong --default=1234567890 MISSING >actual.out 2>actual.err &&\n \ttest_cmp expected actual.out &&\n \ttest_must_be_empty actual.err &&\n \n \techo 0 >expected &&\n-\ttest_must_fail git env--helper --type=ulong --default=0 MISSING >actual &&\n+\ttest_must_fail test-tool env-helper --type=ulong --default=0 MISSING >actual &&\n \ttest_cmp expected actual &&\n \n-\tgit env--helper --type=ulong --default=1234567890 --exit-code MISSING >actual.out 2>actual.err &&\n+\ttest-tool env-helper --type=ulong --default=1234567890 --exit-code MISSING >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err &&\n \n-\tEXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS --exit-code >actual.out 2>actual.err &&\n+\tEXISTS=1234567890 test-tool env-helper --type=ulong --default=0 EXISTS --exit-code >actual.out 2>actual.err &&\n \ttest_must_be_empty actual.out &&\n \ttest_must_be_empty actual.err &&\n \n \techo 1234567890 >expected &&\n-\tEXISTS=1234567890 git env--helper --type=ulong --default=0 EXISTS >actual.out 2>actual.err &&\n+\tEXISTS=1234567890 test-tool env-helper --type=ulong --default=0 EXISTS >actual.out 2>actual.err &&\n \ttest_cmp expected actual.out &&\n \ttest_must_be_empty actual.err\n '\n \n-test_expect_success 'env--helper reads config thanks to trace2' '\n+test_expect_success 'test-tool env-helper reads config thanks to trace2' '\n \tmkdir home &&\n \tgit config -f home/.gitconfig include.path cycle &&\n \tgit config -f home/cycle include.path .gitconfig &&\n@@ -93,7 +93,7 @@ test_expect_success 'env--helper reads config thanks to trace2' '\n \n \ttest_must_fail \\\n \t\tenv HOME=\"$(pwd)/home\" GIT_TEST_ENV_HELPER=true \\\n-\t\tgit -C cycle env--helper --type=bool --default=0 --exit-code GIT_TEST_ENV_HELPER 2>err &&\n+\t\ttest-tool -C cycle env-helper --type=bool --default=0 --exit-code GIT_TEST_ENV_HELPER 2>err &&\n \tgrep \"exceeded maximum include depth\" err\n '\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 54e74d5301d..525055d44bc 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1422,7 +1422,7 @@ test_bool_env () {\n \t\tBUG \"test_bool_env requires two parameters (variable name and default value)\"\n \tfi\n \n-\tgit env--helper --type=bool --default=\"$2\" --exit-code \"$1\"\n+\ttest-tool env-helper --type=bool --default=\"$2\" --exit-code \"$1\"\n \tret=$?\n \tcase $ret in\n \t0|1)\t# unset or valid bool value\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 4fab1c1984c..01e88781dd2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1542,8 +1542,8 @@ then\n \t# Normalize with test_bool_env\n \tpasses_sanitize_leak=\n \n-\t# We need to see TEST_PASSES_SANITIZE_LEAK in \"git\n-\t# env--helper\" (via test_bool_env)\n+\t# We need to see TEST_PASSES_SANITIZE_LEAK in \"test-tool\n+\t# env-helper\" (via test_bool_env)\n \texport TEST_PASSES_SANITIZE_LEAK\n \tif test_bool_env TEST_PASSES_SANITIZE_LEAK false\n \tthen\n@@ -1682,7 +1682,7 @@ yes () {\n # The GIT_TEST_FAIL_PREREQS code hooks into test_set_prereq(), and\n # thus needs to be set up really early, and set an internal variable\n # for convenience so the hot test_set_prereq() codepath doesn't need\n-# to call \"git env--helper\" (via test_bool_env). Only do that work\n+# to call \"test-tool env-helper\" (via test_bool_env). Only do that work\n # if needed by seeing if GIT_TEST_FAIL_PREREQS is set at all.\n GIT_TEST_FAIL_PREREQS_INTERNAL=\n if test -n \"$GIT_TEST_FAIL_PREREQS\"\n-- \n2.39.0.1215.g1ba3f685d4f\n\n"},{"id":"470215","messageId":"Y8A3yGeJl0TCDNqe@coredump.intra.peff.net","threadId":"59065","inReplyTo":"patch-1.1-e662c570f1d-20230112T155226Z-avarab@gmail.com","subject":"Re: [PATCH] env-helper: move this built-in to to \"test-tool env-helper\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-12T16:39:36Z","receivedAt":"2023-01-12T16:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 12, 2023 at 05:03:21PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Arguably \"git env--helper\" should be \"test-tool\", which wouldn't run\n> > into this problem, but it's probably not worth refactoring it for the\n> > sake of these tests.\n> \n> I think it's worth doing that, i.e. to take this alternate\n> approach. When I removed the GIT_TEST_GETTEXT_POISON facility I didn't\n> want to make that series any larger, and keeping it a built-in seemed\n> harmless, as there wasn't any practical difference between an\n> undocumented built-in and a test-tool.\n> \n> But as your patch shows there is, I think we should just pay down that\n> technical debt, rather than adding to it by accumulating workarounds\n> (however small those are...).\n\nYeah, I am totally fine with this direction. My reservations were:\n\n  1. It was work somebody had to do. But now you've done it.\n\n  2. It's _possible_ that some script somewhere is depending on it. But\n     I think the \"--\" in the name plus the lack of documentation means\n     that it's unlikely, and that we're morally absolved if somebody's\n     script does break.\n\n>  .gitignore                                    |  1 -\n>  Makefile                                      |  2 +-\n>  git.c                                         |  1 -\n>  .../helper/test-env-helper.c                  | 24 +++----\n>  t/helper/test-tool.c                          |  1 +\n>  t/helper/test-tool.h                          |  1 +\n>  t/t0017-env-helper.sh                         | 62 +++++++++----------\n>  t/test-lib-functions.sh                       |  2 +-\n>  t/test-lib.sh                                 |  6 +-\n>  9 files changed, 50 insertions(+), 50 deletions(-)\n>  rename builtin/env--helper.c => t/helper/test-env-helper.c (71%)\n\nThe patch itself looks obviously correct to me.\n\n> -\ttest_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n> +\ttest_must_fail test-tool env-helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n\nLong lines like these made me wonder if it should simply be \"test-tool\nenv\", which is shorter. We do not need \"helper\" to avoid polluting the\nmain git-command namespace, and everything in test-tool is a helper\nanyway. But it probably doesn't matter much either way. It's not like\nthat line wasn't already overly long. :)\n\nIf we do take this, then my t/interop patch can be dropped, though we\nmight want to salvage the error message bit:\n\n-- >8 --\nSubject: [PATCH] t/interop: report which vanilla git command failed\n\nThe interop test library sets up wrappers \"git.a\" and \"git.b\" to\nrepresent the two versions to be tested. It also wraps vanilla \"git\" to\nreport an error, with the goal of catching tests which accidentally fail\nto use one of the version-specific wrappers (which could invalidate the\ntests in a very subtle way).\n\nBut when it catches an invocation of vanilla git, it doesn't give any\ndetails, which makes it very hard to debug exactly which invocation is\nresponsible (especially if it's buried in a function invocation, etc).\nLet's report the arguments passed to git, which helps narrow it down.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/interop/interop-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh\nindex 3e0a2911d4..62f4481b6e 100644\n--- a/t/interop/interop-lib.sh\n+++ b/t/interop/interop-lib.sh\n@@ -68,7 +68,7 @@ generate_wrappers () {\n \twrap_git .bin/git.a \"$DIR_A\" &&\n \twrap_git .bin/git.b \"$DIR_B\" &&\n \twrite_script .bin/git <<-\\EOF &&\n-\techo >&2 fatal: test tried to run generic git\n+\techo >&2 fatal: test tried to run generic git: $*\n \texit 1\n \tEOF\n \tPATH=$(pwd)/.bin:$PATH\n-- \n2.39.0.508.g93b13bde48\n"},{"id":"470306","messageId":"xmqqpmbi195g.fsf@gitster.g","threadId":"59065","inReplyTo":"Y8A3yGeJl0TCDNqe@coredump.intra.peff.net","subject":"Re: [PATCH] env-helper: move this built-in to to \"test-tool env-helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-13T20:17:47Z","receivedAt":"2023-01-13T20:17:56Z","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> Yeah, I am totally fine with this direction. My reservations were:\n>\n>   1. It was work somebody had to do. But now you've done it.\n>\n>   2. It's _possible_ that some script somewhere is depending on it. But\n>      I think the \"--\" in the name plus the lack of documentation means\n>      that it's unlikely, and that we're morally absolved if somebody's\n>      script does break.\n\nThat's how we burned some folks who depend on submodule--helper,\nisn't it? ;-)\n\n> The patch itself looks obviously correct to me.\n\n>> -\ttest_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n>> +\ttest_must_fail test-tool env-helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&\n>\n> Long lines like these made me wonder if it should simply be \"test-tool\n> env\", which is shorter. We do not need \"helper\" to avoid polluting the\n> main git-command namespace, and everything in test-tool is a helper\n> anyway. But it probably doesn't matter much either way. It's not like\n> that line wasn't already overly long. :)\n\nI agree that the \"-helper\" looks a bit irritating, not because the\nline is long, but because test-tool is by definition about helpers\nused in tests, so the suffix is redundant.\n\nLet's have a small update before queuing it.\n\n> If we do take this, then my t/interop patch can be dropped, though we\n> might want to salvage the error message bit:\n\nYup, that does make sense.  Will queue what is below the scissors\nline.\n\nThanks, both.\n\n>\n> -- >8 --\n> Subject: [PATCH] t/interop: report which vanilla git command failed\n>\n> The interop test library sets up wrappers \"git.a\" and \"git.b\" to\n> represent the two versions to be tested. It also wraps vanilla \"git\" to\n> report an error, with the goal of catching tests which accidentally fail\n> to use one of the version-specific wrappers (which could invalidate the\n> tests in a very subtle way).\n>\n> But when it catches an invocation of vanilla git, it doesn't give any\n> details, which makes it very hard to debug exactly which invocation is\n> responsible (especially if it's buried in a function invocation, etc).\n> Let's report the arguments passed to git, which helps narrow it down.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/interop/interop-lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh\n> index 3e0a2911d4..62f4481b6e 100644\n> --- a/t/interop/interop-lib.sh\n> +++ b/t/interop/interop-lib.sh\n> @@ -68,7 +68,7 @@ generate_wrappers () {\n>  \twrap_git .bin/git.a \"$DIR_A\" &&\n>  \twrap_git .bin/git.b \"$DIR_B\" &&\n>  \twrite_script .bin/git <<-\\EOF &&\n> -\techo >&2 fatal: test tried to run generic git\n> +\techo >&2 fatal: test tried to run generic git: $*\n>  \texit 1\n>  \tEOF\n>  \tPATH=$(pwd)/.bin:$PATH\n"},{"id":"470358","messageId":"744c8abe-6065-872d-8c9f-a1b1ab682d52@gmail.com","threadId":"59065","inReplyTo":"patch-1.1-e662c570f1d-20230112T155226Z-avarab@gmail.com","subject":"Re: [PATCH] env-helper: move this built-in to to \"test-tool env-helper\"","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-01-14T17:43:45Z","receivedAt":"2023-01-14T17:43:55Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Hi Ævar,\n\nHere's a drive-by nitpick:\n\nOn 12/01/2023 17:03, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/Makefile b/Makefile\n> index db447d07383..f2f342683c1 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -799,6 +799,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n>   TEST_BUILTINS_OBJS += test-dump-split-index.o\n>   TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n>   TEST_BUILTINS_OBJS += test-example-decorate.o\n> +TEST_BUILTINS_OBJS += test-env-helper.o\n\nThe .o files are sorted alphabetically in TEST_BUILTINS_OBJS, so\ntest-env-helper.o should be above test-example-decorate.o\n\nCompare it to changes in t/helper/test-tool.[ch]:\n\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 7eb1a26a305..abe8a785eb6 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -28,6 +28,7 @@ static struct test_cmd cmds[] = {\n>   \t{ \"dump-fsmonitor\", cmd__dump_fsmonitor },\n>   \t{ \"dump-split-index\", cmd__dump_split_index },\n>   \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n> +\t{ \"env-helper\", cmd__env_helper },\n>   \t{ \"example-decorate\", cmd__example_decorate },\n>   \t{ \"fast-rebase\", cmd__fast_rebase },\n>   \t{ \"fsmonitor-client\", cmd__fsmonitor_client },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 2e20a16eb82..ea2672436c9 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -22,6 +22,7 @@ int cmd__dump_fsmonitor(int argc, const char **argv);\n>   int cmd__dump_split_index(int argc, const char **argv);\n>   int cmd__dump_untracked_cache(int argc, const char **argv);\n>   int cmd__dump_reftable(int argc, const char **argv);\n> +int cmd__env_helper(int argc, const char **argv);\n>   int cmd__example_decorate(int argc, const char **argv);\n>   int cmd__fast_rebase(int argc, const char **argv);\n>   int cmd__fsmonitor_client(int argc, const char **argv);\n"},{"id":"470370","messageId":"xmqq358clffc.fsf@gitster.g","threadId":"59065","inReplyTo":"744c8abe-6065-872d-8c9f-a1b1ab682d52@gmail.com","subject":"Re: [PATCH] env-helper: move this built-in to to \"test-tool env-helper\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-15T02:06:31Z","receivedAt":"2023-01-15T02:08:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -799,6 +799,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o\n>>   TEST_BUILTINS_OBJS += test-dump-split-index.o\n>>   TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n>>   TEST_BUILTINS_OBJS += test-example-decorate.o\n>> +TEST_BUILTINS_OBJS += test-env-helper.o\n>\n> The .o files are sorted alphabetically in TEST_BUILTINS_OBJS, so\n> test-env-helper.o should be above test-example-decorate.o\n\nI'll locally tweak to correct this, to prevent this patch from\nmaking it worse, but the ordering of other entries are already\nbroken at other places (e.g. around test-rot13-filter.o).  Somebody\nmay want to send in a clean-up patch, or alternatively we may not\ncare too deeply and doing nothing about it ;-).\n\nThanks.\n"}]}