{"thread":{"id":"61987","subject":"[PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","startedAt":"2024-08-21T11:02:36Z","lastAt":"2024-08-22T16:25:18Z","messageCount":15,"participants":["Derrick Stolee via GitGitGadget","Jeff King","Junio C Hamano","Gabor Gombas","Patrick Steinhardt","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"501413","messageId":"pull.1776.git.1724238152.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":null,"subject":"[PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:25Z","receivedAt":"2024-08-21T11:02:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Advice is supposed to be for humans, not machines. Why do we output it when\nstderr is not a terminal? Let's stop doing that.\n\nI'm labeling this as an RFC because I believe there is some risk with this\nchange. In particular, this does change behavior to reduce the output that\nsome scripts may depend upon. But this output is not intended to be locked\nin and we add or edit advice messages without considering this impact, so\nthere is risk in the existing system already.\n\nThis series is motivated by an internal tool breaking due to the advice\nmessage added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index\nexpands, 2024-07-08). This tool is assuming that any output to stderr is an\nerror, and in this case is attempting to parse it to determine what kind of\nerror (warning, error, or failure).\n\nI've recommended that the tool author remove the advice message for now, but\nI'd like to help other tool authors avoid this surprise.\n\nI read the thread for the --no-advice option [1] looking to see if this was\npresented as an option, but did not see it as part of that review. I hope\nthat this is not considered a breaking change for users, but I could see the\nargument for that.\n\n[1]\nhttps://lore.kernel.org/git/20240424035857.84583-1-james@jamesliu.io/t/#u\n\n * Patches 1-5 are preparation patches to make the test library work to test\n the advice system after the final patch. These are split by test file\n name to reduce the size of the patches, but could be squashed into a\n megapatch if necessary. This is usually a simple addition of the\n GIT_ADVICE=1 environment variable, but there were some changes made to\n those lines to be more correct as necessary.\n * Patch 6 highlights the fact that 'git status' uses advice_enabled() to\n determine if it should print certain parenthetical results. See\n format_tracking_info() in remote.c for an example. This output doesn't\n use the advise() method, but instead appends to a string buffer that is\n later sent to stdout. (If we think this part of the change is too risky,\n then we could move the isatty() out of advice_enabled() and into\n advise(), but that would not match the existing behavior of what is\n blocked by --no-advice.)\n * Patch 7 modifies advice_enabled() to disable when isatty(2) is false and\n GIT_ADVICE is unset.\n\nThanks, - Stolee\n\nDerrick Stolee (7):\n t1000-2000: add GIT_ADVICE=1 for advice tests\n t3000-4000: add GIT_ADVICE=1 to advice tests\n t5000: add GIT_ADVICE=1 to advice tests\n t6000: add GIT_ADVICE=1 to advice tests\n t7000: add GIT_ADVICE=1 to advice tests\n t7508/12: set GIT_ADVICE=1 across all tests\n advice: refuse to output if stderr not TTY\n\n Documentation/config/advice.txt | 9 ++-\n advice.c | 4 +-\n t/lib-httpd.sh | 2 +-\n t/t0018-advice.sh | 18 +++--\n t/t1092-sparse-checkout-compatibility.sh | 18 ++---\n t/t2020-checkout-detach.sh | 25 ++++---\n t/t2024-checkout-dwim.sh | 5 +-\n t/t2060-switch.sh | 4 +-\n t/t2204-add-ignored.sh | 8 +--\n t/t2400-worktree-add.sh | 12 ++--\n t/t3200-branch.sh | 4 +-\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3501-revert-cherry-pick.sh | 2 +-\n t/t3507-cherry-pick-conflict.sh | 4 +-\n t/t3510-cherry-pick-sequence.sh | 6 +-\n t/t3600-rm.sh | 12 ++--\n t/t3602-rm-sparse-checkout.sh | 18 ++---\n t/t3700-add.sh | 6 +-\n t/t3705-add-sparse-checkout.sh | 32 ++++-----\n t/t4150-am.sh | 14 ++--\n t/t5505-remote.sh | 5 +-\n t/t5520-pull.sh | 4 +-\n t/t5541-http-push-smart.sh | 6 +-\n t/t6001-rev-list-graft.sh | 4 +-\n t/t6050-replace.sh | 6 +-\n t/t6436-merge-overwrite.sh | 6 +-\n t/t6437-submodule-merge.sh | 16 ++---\n t/t6439-merge-co-error-msgs.sh | 12 ++--\n t/t7002-mv-sparse-checkout.sh | 85 ++++++++++++-----------\n t/t7004-tag.sh | 2 +-\n t/t7060-wtstatus.sh | 11 +--\n t/t7201-co.sh | 2 +-\n t/t7400-submodule-basic.sh | 2 +-\n t/t7402-submodule-rebase.sh | 3 +-\n t/t7406-submodule-update.sh | 2 +-\n t/t7500-commit-template-squash-signoff.sh | 3 +-\n t/t7508-status.sh | 4 ++\n t/t7512-status-help.sh | 8 ++-\n t/t7520-ignored-hook-warning.sh | 8 +--\n 39 files changed, 214 insertions(+), 180 deletions(-)\n\n\nbase-commit: bb9c16bd4f1a9a00799e10c81ee6506cf468c0c7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1776%2Fderrickstolee%2Fadvice-tty-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1776/derrickstolee/advice-tty-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1776\n-- \ngitgitgadget\n"},{"id":"501414","messageId":"37eaab2f76341d6a4dd253b67b1567c807c2e219.1724238152.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 1/7] t1000-2000: add GIT_ADVICE=1 for advice tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:26Z","receivedAt":"2024-08-21T11:02:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSeveral tests validate the exact output of stderr, including when the stderr\nfile should be empty. In advance of modifying the advice system to only\noutput when stderr is a terminal, force the advice system to output in these\ncases.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++--------\n t/t2020-checkout-detach.sh | 25 ++++++++++++++---------\n t/t2024-checkout-dwim.sh | 5 +++--\n t/t2060-switch.sh | 4 ++--\n t/t2204-add-ignored.sh | 8 ++++----\n t/t2400-worktree-add.sh | 12 +++++------\n t/t7500-commit-template-squash-signoff.sh | 3 ++-\n 7 files changed, 41 insertions(+), 34 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a2c0e1b4dcc..b5183ea7c83 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -411,10 +411,10 @@ test_expect_success 'add outside sparse cone' '\n \trun_on_sparse mkdir folder1 &&\n \trun_on_sparse ../edit-contents folder1/a &&\n \trun_on_sparse ../edit-contents folder1/newfile &&\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/a &&\n-\ttest_sparse_match test_must_fail git add folder1/newfile &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/newfile &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/newfile\n '\n@@ -466,13 +466,13 @@ test_expect_success 'status/add: outside sparse cone' '\n \ttest_sparse_match git status --porcelain=v2 &&\n \n \t# Adding the path outside of the sparse-checkout cone should fail.\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/a &&\n \ttest_all_match git add --refresh folder1/a &&\n \ttest_must_be_empty sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/a &&\n-\ttest_sparse_match test_must_fail git add folder1/new &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/new &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/new &&\n \ttest_sparse_match git add --sparse folder1/a &&\n@@ -1018,7 +1018,7 @@ test_expect_success 'merge with conflict outside cone' '\n \ttest_all_match git status --porcelain=v2 &&\n \n \t# 2. Add the file with conflict markers\n-\ttest_sparse_match test_must_fail git add folder1/a &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder1/a &&\n \ttest_all_match git add --sparse folder1/a &&\n@@ -1027,7 +1027,7 @@ test_expect_success 'merge with conflict outside cone' '\n \t# 3. Rename the file to another sparse filename and\n \t# accept conflict markers as resolved content.\n \trun_on_all mv folder2/a folder2/z &&\n-\ttest_sparse_match test_must_fail git add folder2 &&\n+\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder2 &&\n \tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \ttest_sparse_unstaged folder2/z &&\n \ttest_all_match git add --sparse folder2 &&\n@@ -1058,7 +1058,7 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' '\n \t\t# NEEDSWORK: Even though the merge conflict removed the\n \t\t# SKIP_WORKTREE bit from the index entry for folder1/a, we should\n \t\t# warn that this is a problematic add.\n-\t\ttest_sparse_match test_must_fail git add folder1/a &&\n+\t\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&\n \t\tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \t\ttest_sparse_unstaged folder1/a &&\n \t\ttest_all_match git add --sparse folder1/a &&\n@@ -1070,7 +1070,7 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' '\n \t\t# outside of the sparse-checkout cone and does not match an\n \t\t# existing index entry with the SKIP_WORKTREE bit cleared.\n \t\trun_on_all mv folder2/a folder2/z &&\n-\t\ttest_sparse_match test_must_fail git add folder2 &&\n+\t\ttest_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder2 &&\n \t\tgrep \"Disable or modify the sparsity rules\" sparse-checkout-err &&\n \t\ttest_sparse_unstaged folder2/z &&\n \t\ttest_all_match git add --sparse folder2 &&\n@@ -2341,7 +2341,7 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tgit -C sparse-index sparse-checkout set deep/deeper1 &&\n \tmkdir -p sparse-index/deep/deeper2/deepest &&\n \ttouch sparse-index/deep/deeper2/deepest/bogus &&\n-\tgit -C sparse-index status 2>err &&\n+\tGIT_ADVICE=1 git -C sparse-index status 2>err &&\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n \ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 8d90d028504..43ee72b19bd 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -175,7 +175,7 @@ test_expect_success 'tracking count is accurate after orphan check' '\n \tgit config branch.child.remote . &&\n \tgit config branch.child.merge refs/heads/main &&\n \tgit checkout child^ &&\n-\tgit checkout child >stdout &&\n+\tGIT_ADVICE=1 git checkout child >stdout &&\n \ttest_cmp expect stdout &&\n \n \tgit checkout --detach child >stdout &&\n@@ -251,15 +251,17 @@ test_expect_success 'describe_detached_head prints no SHA-1 ellipsis when not as\n \t# Various ways of *not* asking for ellipses\n \n \tsane_unset GIT_PRINT_SHA1_ELLIPSIS &&\n-\tgit -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 1st_detach actual &&\n \n-\tGIT_PRINT_SHA1_ELLIPSIS=\"no\" git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=\"no\" git -c 'core.abbrev=12' \\\n+\t\tcheckout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 2nd_detach actual &&\n \n-\tGIT_PRINT_SHA1_ELLIPSIS= git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS= git -c 'core.abbrev=12' \\\n+\t\tcheckout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 3rd_detach actual &&\n \n@@ -270,17 +272,17 @@ test_expect_success 'describe_detached_head prints no SHA-1 ellipsis when not as\n \tcheck_not_detached &&\n \n \t# Make no mention of the env var at all\n-\tgit -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 1st_detach actual &&\n \n \tGIT_PRINT_SHA1_ELLIPSIS='nope' &&\n-\tgit -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 2nd_detach actual &&\n \n \tGIT_PRINT_SHA1_ELLIPSIS=nein &&\n-\tgit -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 3rd_detach actual &&\n \n@@ -333,15 +335,18 @@ test_expect_success 'describe_detached_head does print SHA-1 ellipsis when asked\n \t# Various ways of asking for ellipses...\n \t# The user can just use any kind of quoting (including none).\n \n-\tGIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' \\\n+\t\tcheckout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 1st_detach actual &&\n \n-\tGIT_PRINT_SHA1_ELLIPSIS=Yes git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=Yes git -c 'core.abbrev=12' \\\n+\t\tcheckout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 2nd_detach actual &&\n \n-\tGIT_PRINT_SHA1_ELLIPSIS=YES git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&\n+\tGIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=YES git -c 'core.abbrev=12' \\\n+\t\tcheckout HEAD^ >actual 2>&1 &&\n \tcheck_detached &&\n \ttest_cmp 3rd_detach actual &&\n \ndiff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh\nindex 2caada3d834..56be88b1620 100755\n--- a/t/t2024-checkout-dwim.sh\n+++ b/t/t2024-checkout-dwim.sh\n@@ -103,11 +103,12 @@ test_expect_success 'when arg matches multiple remotes, do not fallback to inter\n test_expect_success 'checkout of branch from multiple remotes fails with advice' '\n \tgit checkout -B main &&\n \ttest_might_fail git branch -D foo &&\n-\ttest_must_fail git checkout foo 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git checkout foo 2>stderr &&\n \ttest_branch main &&\n \tstatus_uno_is_clean &&\n \ttest_grep \"^hint: \" stderr &&\n-\ttest_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \\\n+\ttest_env GIT_ADVICE=1 test_must_fail git \\\n+\t\t-c advice.checkoutAmbiguousRemoteBranchName=false \\\n \t\tcheckout foo 2>stderr &&\n \ttest_branch main &&\n \tstatus_uno_is_clean &&\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex 77b2346291b..d84b3accf0e 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -34,13 +34,13 @@ test_expect_success 'switch and detach' '\n '\n \n test_expect_success 'suggestion to detach' '\n-\ttest_must_fail git switch main^{commit} 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git switch main^{commit} 2>stderr &&\n \tgrep \"try again with the --detach option\" stderr\n '\n \n test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '\n \ttest_config advice.suggestDetachingHead false &&\n-\ttest_must_fail git switch main^{commit} 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git switch main^{commit} 2>stderr &&\n \t! grep \"try again with the --detach option\" stderr\n '\n \ndiff --git a/t/t2204-add-ignored.sh b/t/t2204-add-ignored.sh\nindex b7cf1e492c1..ca46bbd22c7 100755\n--- a/t/t2204-add-ignored.sh\n+++ b/t/t2204-add-ignored.sh\n@@ -30,7 +30,7 @@ for i in ign dir/ign dir/sub dir/sub/*ign sub/file sub sub/*\n do\n \ttest_expect_success \"complaints for ignored $i\" '\n \t\trm -f .git/index &&\n-\t\ttest_must_fail git add \"$i\" 2>err &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git add \"$i\" 2>err &&\n \t\tgit ls-files \"$i\" >out &&\n \t\ttest_must_be_empty out\n \t'\n@@ -41,7 +41,7 @@ do\n \n \ttest_expect_success \"complaints for ignored $i with unignored file\" '\n \t\trm -f .git/index &&\n-\t\ttest_must_fail git add \"$i\" file 2>err &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git add \"$i\" file 2>err &&\n \t\tgit ls-files \"$i\" >out &&\n \t\ttest_must_be_empty out\n \t'\n@@ -56,7 +56,7 @@ do\n \t\trm -f .git/index &&\n \t\t(\n \t\t\tcd dir &&\n-\t\t\ttest_must_fail git add \"$i\" 2>err &&\n+\t\t\ttest_env GIT_ADVICE=1 test_must_fail git add \"$i\" 2>err &&\n \t\t\tgit ls-files \"$i\" >out &&\n \t\t\ttest_must_be_empty out\n \t\t)\n@@ -76,7 +76,7 @@ do\n \t\trm -f .git/index &&\n \t\t(\n \t\t\tcd sub &&\n-\t\t\ttest_must_fail git add \"$i\" 2>err &&\n+\t\t\ttest_env GIT_ADVICE=1 test_must_fail git add \"$i\" 2>err &&\n \t\t\tgit ls-files \"$i\" >out &&\n \t\t\ttest_must_be_empty out\n \t\t)\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex cfc4aeb1798..742002ff41e 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -436,7 +436,7 @@ test_wt_add_orphan_hint () {\n \t\tgit init repo &&\n \t\t(cd repo && test_commit commit) &&\n \t\tgit -C repo switch --orphan noref &&\n-\t\ttest_must_fail git -C repo worktree add $opts foobar/ 2>actual &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git -C repo worktree add $opts foobar/ 2>actual &&\n \t\t! grep \"error: unknown switch\" actual &&\n \t\tgrep \"hint: If you meant to create a worktree containing a new unborn branch\" actual &&\n \t\tif [ $use_branch -eq 1 ]\n@@ -983,7 +983,7 @@ test_dwim_orphan () {\n \t\t\tfi &&\n \t\t\tif [ \"$outcome\" = \"infer\" ]\n \t\t\tthen\n-\t\t\t\tgit $dashc_args worktree add $args 2>actual &&\n+\t\t\t\tGIT_ADVICE=1 git $dashc_args worktree add $args 2>actual &&\n \t\t\t\tif [ $use_quiet -eq 1 ]\n \t\t\t\tthen\n \t\t\t\t\ttest_must_be_empty actual\n@@ -992,7 +992,7 @@ test_dwim_orphan () {\n \t\t\t\tfi\n \t\t\telif [ \"$outcome\" = \"no_infer\" ]\n \t\t\tthen\n-\t\t\t\tgit $dashc_args worktree add $args 2>actual &&\n+\t\t\t\tGIT_ADVICE=1 git $dashc_args worktree add $args 2>actual &&\n \t\t\t\tif [ $use_quiet -eq 1 ]\n \t\t\t\tthen\n \t\t\t\t\ttest_must_be_empty actual\n@@ -1001,11 +1001,11 @@ test_dwim_orphan () {\n \t\t\t\tfi\n \t\t\telif [ \"$outcome\" = \"fetch_error\" ]\n \t\t\tthen\n-\t\t\t\ttest_must_fail git $dashc_args worktree add $args 2>actual &&\n+\t\t\t\ttest_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&\n \t\t\t\tgrep \"$fetch_error_text\" actual\n \t\t\telif [ \"$outcome\" = \"fatal_orphan_bad_combo\" ]\n \t\t\tthen\n-\t\t\t\ttest_must_fail git $dashc_args worktree add $args 2>actual &&\n+\t\t\t\ttest_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&\n \t\t\t\tif [ $use_quiet -eq 1 ]\n \t\t\t\tthen\n \t\t\t\t\t! grep \"$info_text\" actual\n@@ -1015,7 +1015,7 @@ test_dwim_orphan () {\n \t\t\t\tgrep \"$bad_combo_regex\" actual\n \t\t\telif [ \"$outcome\" = \"warn_bad_head\" ]\n \t\t\tthen\n-\t\t\t\ttest_must_fail git $dashc_args worktree add $args 2>actual &&\n+\t\t\t\ttest_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&\n \t\t\t\tif [ $use_quiet -eq 1 ]\n \t\t\t\tthen\n \t\t\t\t\tgrep \"$invalid_ref_regex\" actual &&\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 4dca8d97a77..546b6f2f373 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -554,7 +554,8 @@ test_expect_success 'commit without staging files fails and displays hints' '\n \tgit add file &&\n \tgit commit -m initial &&\n \techo \"changes\" >>file &&\n-\ttest_must_fail git commit -m update >actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail \\\n+\t\tgit commit -m update >actual &&\n \ttest_grep \"no changes added to commit (use \\\"git add\\\" and/or \\\"git commit -a\\\")\" actual\n '\n \n-- \ngitgitgadget\n\n"},{"id":"501415","messageId":"483fcc94355e69e21f76ed4a6d8cd8b885bd7f75.1724238152.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 2/7] t3000-4000: add GIT_ADVICE=1 to advice tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:27Z","receivedAt":"2024-08-21T11:02:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSeveral tests validate the exact output of stderr, including when the stderr\nfile should be empty. In advance of modifying the advice system to only\noutput when stderr is a terminal, force the advice system to output in these\ncases.\n\nSigned-off-by: Derrick Stolee \n---\n t/t3200-branch.sh | 4 ++--\n t/t3404-rebase-interactive.sh | 2 +-\n t/t3501-revert-cherry-pick.sh | 2 +-\n t/t3507-cherry-pick-conflict.sh | 4 ++--\n t/t3510-cherry-pick-sequence.sh | 6 +++---\n t/t3600-rm.sh | 12 ++++++------\n t/t3602-rm-sparse-checkout.sh | 18 +++++++++---------\n t/t3700-add.sh | 6 +++---\n t/t3705-add-sparse-checkout.sh | 32 ++++++++++++++++----------------\n t/t4150-am.sh | 14 +++++++-------\n 10 files changed, 50 insertions(+), 50 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex ccfa6a720d0..9ff64fe4f1a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1161,7 +1161,7 @@ test_expect_success 'avoid ambiguous track and advise' '\n \thint: different remotes'\\'' fetch refspecs map into different\n \thint: tracking namespaces.\n \tEOF\n-\ttest_must_fail git branch all1 main 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git branch all1 main 2>actual &&\n \ttest_cmp expected actual &&\n \ttest -z \"$(git config branch.all1.merge)\"\n '\n@@ -1699,7 +1699,7 @@ test_expect_success 'errors if given a bad branch name' '\n \thint: See `man git check-ref-format`\n \thint: Disable this message with \"git config advice.refSyntax false\"\n \tEOF\n-\ttest_must_fail git branch foo..bar >actual 2>&1 &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git branch foo..bar >actual 2>&1 &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex f92baad1381..c31ca807f7b 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -2229,7 +2229,7 @@ test_expect_success 'non-merge commands reject merge commits' '\n \tEOF\n \t(\n \t\tset_replace_editor todo &&\n-\t\ttest_must_fail git rebase -i HEAD 2>actual\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git rebase -i HEAD 2>actual\n \t) &&\n \tcat >expect <<-EOF &&\n \terror: ${SQ}pick${SQ} does not accept merge commits\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 411027fb58c..3478a8a588f 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -181,7 +181,7 @@ test_expect_success 'advice from failed revert' '\n \thint: Disable this message with \"git config advice.mergeConflict false\"\n \tEOF\n \ttest_commit --append --no-tag \"double-add dream\" dream dream &&\n-\ttest_must_fail git revert HEAD^ 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git revert HEAD^ 2>actual &&\n \ttest_cmp expected actual\n '\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex f3947b400a3..5633a10659d 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -62,7 +62,7 @@ test_expect_success 'advice from failed cherry-pick' '\n \thint: run \"git cherry-pick --abort\".\n \thint: Disable this message with \"git config advice.mergeConflict false\"\n \tEOF\n-\ttest_must_fail git cherry-pick picked 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git cherry-pick picked 2>actual &&\n \n \ttest_cmp expected actual\n '\n@@ -77,7 +77,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' \"\n \thint: with 'git add ' or 'git rm '\n \thint: Disable this message with \\\"git config advice.mergeConflict false\\\"\n \tEOF\n-\ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git cherry-pick --no-commit picked 2>actual &&\n \n \ttest_cmp expected actual\n \"\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 7eb52b12edc..291c5de4f7d 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -231,7 +231,7 @@ test_expect_success 'check advice when we move HEAD by committing' '\n \techo c >foo &&\n \tgit commit -a &&\n \ttest_path_is_missing .git/CHERRY_PICK_HEAD &&\n-\ttest_must_fail git cherry-pick --skip 2>advice &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git cherry-pick --skip 2>advice &&\n \ttest_cmp expect advice\n '\n \n@@ -243,7 +243,7 @@ test_expect_success 'selectively advise --skip while launching another sequence'\n \tfatal: cherry-pick failed\n \tEOF\n \ttest_must_fail git cherry-pick picked..yetanotherpick &&\n-\ttest_must_fail git cherry-pick picked..yetanotherpick 2>advice &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&\n \ttest_cmp expect advice &&\n \tcat >expect <<-EOF &&\n \terror: cherry-pick is already in progress\n@@ -251,7 +251,7 @@ test_expect_success 'selectively advise --skip while launching another sequence'\n \tfatal: cherry-pick failed\n \tEOF\n \tgit reset --merge &&\n-\ttest_must_fail git cherry-pick picked..yetanotherpick 2>advice &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&\n \ttest_cmp expect advice\n '\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 31ac31d4bcd..90a30a3a002 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -822,7 +822,7 @@ test_expect_success 'rm files with different staged content' '\n \tEOF\n \techo content1 >foo.txt &&\n \techo content1 >bar.txt &&\n-\ttest_must_fail git rm foo.txt bar.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm foo.txt bar.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -847,7 +847,7 @@ test_expect_success 'rm file with local modification' '\n \tEOF\n \tgit commit -m \"testing rm 3\" &&\n \techo content3 >foo.txt &&\n-\ttest_must_fail git rm foo.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm foo.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -857,7 +857,7 @@ test_expect_success 'rm file with local modification without hints' '\n \t bar.txt\n \tEOF\n \techo content4 >bar.txt &&\n-\ttest_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -870,7 +870,7 @@ test_expect_success 'rm file with changes in the index' '\n \tgit reset --hard &&\n \techo content5 >foo.txt &&\n \tgit add foo.txt &&\n-\ttest_must_fail git rm foo.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm foo.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -879,7 +879,7 @@ test_expect_success 'rm file with changes in the index without hints' '\n \terror: the following file has changes staged in the index:\n \t foo.txt\n \tEOF\n-\ttest_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -898,7 +898,7 @@ test_expect_success 'rm files with two different errors' '\n \techo content6 >foo1.txt &&\n \techo content6 >bar1.txt &&\n \tgit add bar1.txt &&\n-\ttest_must_fail git rm bar1.txt foo1.txt 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm bar1.txt foo1.txt 2>actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh\nindex fcdefba48cc..c2b197046d4 100755\n--- a/t/t3602-rm-sparse-checkout.sh\n+++ b/t/t3602-rm-sparse-checkout.sh\n@@ -32,7 +32,7 @@ for opt in \"\" -f --dry-run\n do\n \ttest_expect_success \"rm${opt:+ $opt} does not remove sparse entries\" '\n \t\tgit sparse-checkout set --no-cone a &&\n-\t\ttest_must_fail git rm $opt b 2>stderr &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git rm $opt b 2>stderr &&\n \t\ttest_cmp b_error_and_hint stderr &&\n \t\tgit ls-files --error-unmatch b\n \t'\n@@ -72,14 +72,14 @@ test_expect_success 'recursive rm --sparse removes sparse entries' '\n test_expect_success 'rm obeys advice.updateSparsePath' '\n \tgit reset --hard &&\n \tgit sparse-checkout set a &&\n-\ttest_must_fail git -c advice.updateSparsePath=false rm b 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git -c advice.updateSparsePath=false rm b 2>stderr &&\n \ttest_cmp sparse_entry_b_error stderr\n '\n \n test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '\n \tgit reset --hard &&\n \tgit sparse-checkout set a &&\n-\ttest_must_fail git rm nonexistent 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm nonexistent 2>stderr &&\n \tgrep \"fatal: pathspec .nonexistent. did not match any files\" stderr &&\n \t! grep -F -f sparse_error_header stderr\n '\n@@ -87,7 +87,7 @@ test_expect_success 'do not advice about sparse entries when they do not match t\n test_expect_success 'do not warn about sparse entries when pathspec matches dense entries' '\n \tgit reset --hard &&\n \tgit sparse-checkout set a &&\n-\tgit rm \"[ba]\" 2>stderr &&\n+\tGIT_ADVICE=1 git rm \"[ba]\" 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit ls-files --error-unmatch b &&\n \ttest_must_fail git ls-files --error-unmatch a\n@@ -96,7 +96,7 @@ test_expect_success 'do not warn about sparse entries when pathspec matches dens\n test_expect_success 'do not warn about sparse entries with --ignore-unmatch' '\n \tgit reset --hard &&\n \tgit sparse-checkout set a &&\n-\tgit rm --ignore-unmatch b 2>stderr &&\n+\tGIT_ADVICE=1 git rm --ignore-unmatch b 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit ls-files --error-unmatch b\n '\n@@ -105,9 +105,9 @@ test_expect_success 'refuse to rm a non-skip-worktree path outside sparse cone'\n \tgit reset --hard &&\n \tgit sparse-checkout set a &&\n \tgit update-index --no-skip-worktree b &&\n-\ttest_must_fail git rm b 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm b 2>stderr &&\n \ttest_cmp b_error_and_hint stderr &&\n-\tgit rm --sparse b 2>stderr &&\n+\tGIT_ADVICE=1 git rm --sparse b 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_path_is_missing b\n '\n@@ -120,7 +120,7 @@ test_expect_success 'can remove files from non-sparse dir' '\n \ttest_commit x/y/f &&\n \n \tgit sparse-checkout set --no-cone w !/x y/ &&\n-\tgit rm w/f.t x/y/f.t 2>stderr &&\n+\tGIT_ADVICE=1 git rm w/f.t x/y/f.t 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -132,7 +132,7 @@ test_expect_success 'refuse to remove non-skip-worktree file from sparse dir' '\n \tgit sparse-checkout set --no-cone !/x y/ !x/y/z &&\n \n \tgit update-index --no-skip-worktree x/y/z/f.t &&\n-\ttest_must_fail git rm x/y/z/f.t 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rm x/y/z/f.t 2>stderr &&\n \techo x/y/z/f.t | cat sparse_error_header - sparse_hint >expect &&\n \ttest_cmp expect stderr\n '\ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 839c904745a..8042c3bc34a 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -34,7 +34,7 @@ test_expect_success 'Test with no pathspecs' '\n \thint: Maybe you wanted to say ${SQ}git add .${SQ}?\n \thint: Disable this message with \"git config advice.addEmptyPathspec false\"\n \tEOF\n-\tgit add 2>actual &&\n+\tGIT_ADVICE=1 git add 2>actual &&\n \ttest_cmp expect actual\n '\n \n@@ -360,7 +360,7 @@ test_expect_success '\"git add\" a embedded repository' '\n \t\t\tgit -C $name commit --allow-empty -m $name ||\n \t\t\t\treturn 1\n \t\tdone &&\n-\t\tgit add . 2>actual &&\n+\t\tGIT_ADVICE=1 git add . 2>actual &&\n \t\tcat >expect <<-EOF &&\n \t\twarning: adding embedded git repository: inner1\n \t\thint: You${SQ}ve added another git repository inside your current repository.\n@@ -421,7 +421,7 @@ add 'track-this'\n EOF\n \n test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '\n-\ttest_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual.out 2>actual.err\n+\ttest_env GIT_ADVICE=1 test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual.out 2>actual.err\n '\n \n test_expect_success 'git add --dry-run --ignore-missing of non-existing file output' '\ndiff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh\nindex 2bade9e804f..c06e803c0e9 100755\n--- a/t/t3705-add-sparse-checkout.sh\n+++ b/t/t3705-add-sparse-checkout.sh\n@@ -64,7 +64,7 @@ test_expect_success 'setup' \"\n test_expect_success 'git add does not remove sparse entries' '\n \tsetup_sparse_entry &&\n \trm sparse_entry &&\n-\ttest_must_fail git add sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp error_and_hint stderr &&\n \ttest_sparse_entry_unchanged\n@@ -74,7 +74,7 @@ test_expect_success 'git add -A does not remove sparse entries' '\n \tsetup_sparse_entry &&\n \trm sparse_entry &&\n \tsetup_gitignore &&\n-\tgit add -A 2>stderr &&\n+\tGIT_ADVICE=1 git add -A 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_sparse_entry_unchanged\n '\n@@ -83,7 +83,7 @@ test_expect_success 'git add . does not remove sparse entries' '\n \tsetup_sparse_entry &&\n \trm sparse_entry &&\n \tsetup_gitignore &&\n-\ttest_must_fail git add . 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add . 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \n \tcat sparse_error_header >expect &&\n@@ -99,7 +99,7 @@ do\n \ttest_expect_success \"git add${opt:+ $opt} does not update sparse entries\" '\n \t\tsetup_sparse_entry &&\n \t\techo modified >sparse_entry &&\n-\t\ttest_must_fail git add $opt sparse_entry 2>stderr &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git add $opt sparse_entry 2>stderr &&\n \t\ttest_sparse_entry_unstaged &&\n \t\ttest_cmp error_and_hint stderr &&\n \t\ttest_sparse_entry_unchanged\n@@ -110,7 +110,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '\n \tsetup_sparse_entry &&\n \tgit ls-files --debug sparse_entry | grep mtime >before &&\n \ttest-tool chmtime -60 sparse_entry &&\n-\ttest_must_fail git add --refresh sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add --refresh sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp error_and_hint stderr &&\n \tgit ls-files --debug sparse_entry | grep mtime >after &&\n@@ -119,7 +119,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '\n \n test_expect_success 'git add --chmod does not update sparse entries' '\n \tsetup_sparse_entry &&\n-\ttest_must_fail git add --chmod=+x sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add --chmod=+x sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp error_and_hint stderr &&\n \ttest_sparse_entry_unchanged &&\n@@ -131,7 +131,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' '\n \ttest_config core.autocrlf false &&\n \tsetup_sparse_entry \"LINEONE\\r\\nLINETWO\\r\\n\" &&\n \techo \"sparse_entry text=auto\" >.gitattributes &&\n-\ttest_must_fail git add --renormalize sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add --renormalize sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp error_and_hint stderr &&\n \ttest_sparse_entry_unchanged\n@@ -140,7 +140,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' '\n test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '\n \tsetup_sparse_entry &&\n \trm sparse_entry &&\n-\ttest_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp error_and_hint stderr &&\n \ttest_sparse_entry_unchanged\n@@ -148,7 +148,7 @@ test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '\n \n test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '\n \tsetup_sparse_entry &&\n-\ttest_must_fail git add nonexistent 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add nonexistent 2>stderr &&\n \tgrep \"fatal: pathspec .nonexistent. did not match any files\" stderr &&\n \t! grep -F -f sparse_error_header stderr\n '\n@@ -157,7 +157,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' '\n \tsetup_sparse_entry &&\n \techo modified >sparse_entry &&\n \t>dense_entry &&\n-\tgit add \"*_entry\" 2>stderr &&\n+\tGIT_ADVICE=1 git add \"*_entry\" 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_sparse_entry_unchanged &&\n \tgit ls-files --error-unmatch dense_entry\n@@ -181,12 +181,12 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '\n \ttest_sparse_entry_unstaged &&\n \n \t# Avoid munging CRLFs to avoid an error message\n-\tgit -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&\n+\tGIT_ADVICE=1 git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit ls-files --stage >actual &&\n \tgrep \"^100644 .*sparse_entry\\$\" actual &&\n \n-\tgit add --sparse --chmod=+x sparse_entry 2>stderr &&\n+\tGIT_ADVICE=1 git add --sparse --chmod=+x sparse_entry 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit ls-files --stage >actual &&\n \tgrep \"^100755 .*sparse_entry\\$\" actual &&\n@@ -201,7 +201,7 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '\n \n test_expect_success 'add obeys advice.updateSparsePath' '\n \tsetup_sparse_entry &&\n-\ttest_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&\n \ttest_sparse_entry_unstaged &&\n \ttest_cmp sparse_entry_error stderr\n \n@@ -212,7 +212,7 @@ test_expect_success 'add allows sparse entries with --sparse' '\n \techo modified >sparse_entry &&\n \ttest_must_fail git add sparse_entry &&\n \ttest_sparse_entry_unchanged &&\n-\tgit add --sparse sparse_entry 2>stderr &&\n+\tGIT_ADVICE=1 git add --sparse sparse_entry 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -220,7 +220,7 @@ test_expect_success 'can add files from non-sparse dir' '\n \tgit sparse-checkout set w !/x y/ &&\n \tmkdir -p w x/y &&\n \ttouch w/f x/y/f &&\n-\tgit add w/f x/y/f 2>stderr &&\n+\tGIT_ADVICE=1 git add w/f x/y/f 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -228,7 +228,7 @@ test_expect_success 'refuse to add non-skip-worktree file from sparse dir' '\n \tgit sparse-checkout set !/x y/ !x/y/z &&\n \tmkdir -p x/y/z &&\n \ttouch x/y/z/f &&\n-\ttest_must_fail git add x/y/z/f 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git add x/y/z/f 2>stderr &&\n \techo x/y/z/f | cat sparse_error_header - sparse_hint >expect &&\n \ttest_cmp expect stderr\n '\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 5e2b6c80eae..68a62ff330e 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -678,7 +678,7 @@ test_expect_success 'am -3 -q is quiet' '\n \trm -fr .git/rebase-apply &&\n \tgit checkout -f lorem2 &&\n \tgit reset base3way --hard &&\n-\tgit am -3 -q lorem-move.patch >output.out 2>&1 &&\n+\tGIT_ADVICE=1 git am -3 -q lorem-move.patch >output.out 2>&1 &&\n \ttest_must_be_empty output.out\n '\n \n@@ -921,7 +921,7 @@ test_expect_success 'am -q is quiet' '\n \tgit reset --hard &&\n \tgit checkout first &&\n \ttest_tick &&\n-\tgit am -q output.out 2>&1 &&\n+\tGIT_ADVICE=1 git am -q output.out 2>&1 &&\n \ttest_must_be_empty output.out\n '\n \n@@ -930,7 +930,7 @@ test_expect_success 'am empty-file does not infloop' '\n \tgit reset --hard &&\n \ttouch empty-file &&\n \ttest_tick &&\n-\ttest_must_fail git am empty-file 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git am empty-file 2>actual &&\n \techo Patch format detection failed. >expected &&\n \ttest_cmp expected actual\n '\n@@ -1180,7 +1180,7 @@ test_expect_success 'apply binary blob in partial clone' '\n \n test_expect_success 'an empty input file is error regardless of --empty option' '\n \ttest_when_finished \"git am --abort || :\" &&\n-\ttest_must_fail git am --empty=drop empty.patch 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git am --empty=drop empty.patch 2>actual &&\n \techo \"Patch format detection failed.\" >expected &&\n \ttest_cmp expected actual\n '\n@@ -1188,7 +1188,7 @@ test_expect_success 'an empty input file is error regardless of --empty option'\n test_expect_success 'invalid when passing the --empty option alone' '\n \ttest_when_finished \"git am --abort || :\" &&\n \tgit checkout empty-commit^ &&\n-\ttest_must_fail git am --empty empty-commit.patch 2>err &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git am --empty empty-commit.patch 2>err &&\n \techo \"error: invalid value for '\\''--empty'\\'': '\\''empty-commit.patch'\\''\" >expected &&\n \ttest_cmp expected err\n '\n@@ -1224,7 +1224,7 @@ test_expect_success 'record as an empty commit when meeting e-mail message that\n \n test_expect_success 'skip an empty patch in the middle of an am session' '\n \tgit checkout empty-commit^ &&\n-\ttest_must_fail git am empty-commit.patch >out 2>err &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git am empty-commit.patch >out 2>err &&\n \tgrep \"Patch is empty.\" out &&\n \tgrep \"To record the empty patch as an empty commit, run \\\"git am --allow-empty\\\".\" err &&\n \tgit am --skip &&\n@@ -1236,7 +1236,7 @@ test_expect_success 'skip an empty patch in the middle of an am session' '\n \n test_expect_success 'record an empty patch as an empty commit in the middle of an am session' '\n \tgit checkout empty-commit^ &&\n-\ttest_must_fail git am empty-commit.patch >out 2>err &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git am empty-commit.patch >out 2>err &&\n \tgrep \"Patch is empty.\" out &&\n \tgrep \"To record the empty patch as an empty commit, run \\\"git am --allow-empty\\\".\" err &&\n \tgit am --allow-empty >output &&\n-- \ngitgitgadget\n\n"},{"id":"501416","messageId":"970964550ab519c9a8070fada116951bfe04f75d.1724238153.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 3/7] t5000: add GIT_ADVICE=1 to advice tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:28Z","receivedAt":"2024-08-21T11:02:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSeveral tests validate the exact output of stderr, including when the stderr\nfile should be empty. In advance of modifying the advice system to only\noutput when stderr is a terminal, force the advice system to output in these\ncases.\n\nIn particular, lib-https.sh must be updated in order for t5541 to succeed as\nit calls test_http_push_nonff.\n\nSigned-off-by: Derrick Stolee \n---\n t/lib-httpd.sh | 2 +-\n t/t5505-remote.sh | 5 +++--\n t/t5520-pull.sh | 4 ++--\n t/t5541-http-push-smart.sh | 6 ++++--\n 4 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex d83bafeab32..b85ce907f05 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -265,7 +265,7 @@ test_http_push_nonff () {\n \t\techo \"changed\" > path2 &&\n \t\tgit commit -a -m path2 --amend &&\n \n-\t\ttest_must_fail git push -v origin >output 2>&1 &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git push -v origin >output 2>&1 &&\n \t\t(\n \t\t\tcd \"$REMOTE_REPO\" &&\n \t\t\techo \"$HEAD\" >expect &&\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 08424e878e1..3e5215add31 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1452,10 +1452,11 @@ test_expect_success 'unqualified refspec DWIM and advice' '\n \t\t\telse\n \t\t\t\toid=$(git rev-parse some-tag^{$type})\n \t\t\tfi &&\n-\t\t\ttest_must_fail git push origin $oid:dst 2>err &&\n+\t\t\ttest_env GIT_ADVICE=1 test_must_fail git push origin $oid:dst 2>err &&\n \t\t\ttest_grep \"error: The destination you\" err &&\n \t\t\ttest_grep \"hint: Did you mean\" err &&\n-\t\t\ttest_must_fail git -c advice.pushUnqualifiedRefName=false \\\n+\t\t\ttest_env GIT_ADVICE=1 test_must_fail git \\\n+\t\t\t\t-c advice.pushUnqualifiedRefName=false \\\n \t\t\t\tpush origin $oid:dst 2>err &&\n \t\t\ttest_grep \"error: The destination you\" err &&\n \t\t\ttest_grep ! \"hint: Did you mean\" err ||\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 1098cbd0a19..c4a309ce4ae 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -375,7 +375,7 @@ test_expect_success '--rebase with conflicts shows advice' '\n \techo conflicting >>seq.txt &&\n \ttest_tick &&\n \tgit commit -m \"Create conflict\" seq.txt &&\n-\ttest_must_fail git pull --rebase . seq 2>err >out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git pull --rebase . seq 2>err >out &&\n \ttest_grep \"Resolve all conflicts manually\" err\n '\n \n@@ -389,7 +389,7 @@ test_expect_success 'failed --rebase shows advice' '\n \t# force checkout because `git reset --hard` will not leave clean `file`\n \tgit checkout -f -b fails-to-rebase HEAD^ &&\n \ttest_commit v2-without-cr file \"2\" file2-lf &&\n-\ttest_must_fail git pull --rebase . diverging 2>err >out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git pull --rebase . diverging 2>err >out &&\n \ttest_grep \"Resolve all conflicts manually\" err\n '\n \ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 71428f3d5c7..dfd4c21808f 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -145,7 +145,7 @@ test_expect_success 'push fails for non-fast-forward refs unmatched by remote he\n \n \t# push main too; this ensures there is at least one '\"'push'\"' command to\n \t# the remote helper and triggers interaction with the helper.\n-\ttest_must_fail git push -v origin +main main:niam >output 2>&1'\n+\ttest_env GIT_ADVICE=1 test_must_fail git push -v origin +main main:niam >output 2>&1'\n \n test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper: remote output' '\n \tgrep \"^ + [a-f0-9]*\\.\\.\\.[a-f0-9]* *main -> main (forced update)$\" output &&\n@@ -477,7 +477,9 @@ test_expect_success 'Non-ASCII branch name can be used with --force-with-lease'\n \n test_expect_success 'colorize errors/hints' '\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n-\ttest_must_fail git -c color.transport=always -c color.advice=always \\\n+\ttest_env GIT_ADVICE=1 test_must_fail git \\\n+\t\t-c color.transport=always \\\n+\t\t-c color.advice=always \\\n \t\t-c color.push=always \\\n \t\tpush origin origin/main^:main 2>act &&\n \ttest_decode_color decoded &&\n-- \ngitgitgadget\n\n"},{"id":"501417","messageId":"1ce8a8050e13fa5f24dace3ae03ff0e6a5a71aa9.1724238153.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 4/7] t6000: add GIT_ADVICE=1 to advice tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:29Z","receivedAt":"2024-08-21T11:02:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSeveral tests validate the exact output of stderr, including when the stderr\nfile should be empty. In advance of modifying the advice system to only\noutput when stderr is a terminal, force the advice system to output in these\ncases.\n\nSigned-off-by: Derrick Stolee \n---\n t/t6001-rev-list-graft.sh | 4 ++--\n t/t6050-replace.sh | 6 +++---\n t/t6436-merge-overwrite.sh | 6 +++---\n t/t6437-submodule-merge.sh | 16 ++++++++--------\n t/t6439-merge-co-error-msgs.sh | 12 ++++++------\n 5 files changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh\nindex 3553bbbfe73..e3f19621727 100755\n--- a/t/t6001-rev-list-graft.sh\n+++ b/t/t6001-rev-list-graft.sh\n@@ -118,10 +118,10 @@ do\n done\n \n test_expect_success 'show advice that grafts are deprecated' '\n-\tgit show HEAD 2>err &&\n+\tGIT_ADVICE=1 git show HEAD 2>err &&\n \ttest_grep \"git replace\" err &&\n \ttest_config advice.graftFileDeprecated false &&\n-\tgit show HEAD 2>err &&\n+\tGIT_ADVICE=1 git show HEAD 2>err &&\n \ttest_grep ! \"git replace\" err\n '\n \ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex c6e9b33e44e..fc48cb4b0ad 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -489,9 +489,9 @@ test_expect_success '--convert-graft-file' '\n \tprintf \"%s\\n%s %s\\n\\n# comment\\n%s\\n\" \\\n \t\t$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \\\n \t\t>.git/info/grafts &&\n-\tgit status 2>stderr &&\n+\tGIT_ADVICE=1 git status 2>stderr &&\n \ttest_grep \"hint:.*grafts is deprecated\" stderr &&\n-\tgit replace --convert-graft-file 2>stderr &&\n+\tGIT_ADVICE=1 git replace --convert-graft-file 2>stderr &&\n \ttest_grep ! \"hint:.*grafts is deprecated\" stderr &&\n \ttest_path_is_missing .git/info/grafts &&\n \n@@ -502,7 +502,7 @@ test_expect_success '--convert-graft-file' '\n \t: create invalid graft file and verify that it is not deleted &&\n \ttest_when_finished \"rm -f .git/info/grafts\" &&\n \techo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&\n-\ttest_must_fail git replace --convert-graft-file 2>err &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git replace --convert-graft-file 2>err &&\n \ttest_grep \"$EMPTY_BLOB $EMPTY_TREE\" err &&\n \ttest_grep \"$EMPTY_BLOB $EMPTY_TREE\" .git/info/grafts\n '\ndiff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh\nindex ccc620477d4..7c9f5b623f1 100755\n--- a/t/t6436-merge-overwrite.sh\n+++ b/t/t6436-merge-overwrite.sh\n@@ -104,7 +104,7 @@ test_expect_success 'will not overwrite unstaged changes in renamed file' '\n \tcp important other.c &&\n \tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n \tthen\n-\t\ttest_must_fail git merge c1a >out 2>err &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c1a >out 2>err &&\n \t\ttest_grep \"would be overwritten by merge\" err &&\n \t\ttest_cmp important other.c &&\n \t\ttest_path_is_missing .git/MERGE_HEAD\n@@ -140,7 +140,7 @@ test_expect_success 'will not overwrite untracked file in leading path' '\n \trm -rf sub &&\n \tcp important sub &&\n \tcp important sub2 &&\n-\ttest_must_fail git merge sub 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge sub 2>out &&\n \ttest_cmp out expect &&\n \ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest_cmp important sub &&\n@@ -175,7 +175,7 @@ test_expect_success 'will not overwrite untracked file on unborn branch' '\n \tgit rm -fr . &&\n \tgit checkout --orphan new &&\n \tcp important c0.c &&\n-\ttest_must_fail git merge c0 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge c0 2>out &&\n \ttest_cmp out expect\n '\n \ndiff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh\nindex 7a3f1cb27c1..9265cebca75 100755\n--- a/t/t6437-submodule-merge.sh\n+++ b/t/t6437-submodule-merge.sh\n@@ -113,11 +113,11 @@ test_expect_success 'merging should conflict for non fast-forward' '\n \t git checkout -b test-nonforward-a b &&\n \t if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n \t then\n-\t\ttest_must_fail git merge c 2>actual &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c 2>actual &&\n \t\tsub_expect=\"go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)\" &&\n \t\tgrep \"$sub_expect\" actual\n \t else\n-\t\ttest_must_fail git merge c 2> actual\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c 2> actual\n \t fi)\n '\n \n@@ -154,11 +154,11 @@ test_expect_success 'merging should conflict for non fast-forward (resolution ex\n \t git rev-parse --short sub-d > ../expect) &&\n \t if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n \t then\n-\t\ttest_must_fail git merge c >actual 2>sub-actual &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c >actual 2>sub-actual &&\n \t\tsub_expect=\"go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)\" &&\n \t\tgrep \"$sub_expect\" sub-actual\n \t else\n-\t\ttest_must_fail git merge c 2> actual\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c 2> actual\n \t fi &&\n \t grep $(cat expect) actual > /dev/null &&\n \t git reset --hard)\n@@ -181,11 +181,11 @@ test_expect_success 'merging should fail for ambiguous common parent' '\n \t ) &&\n \t if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n \t then\n-\t\ttest_must_fail git merge c >actual 2>sub-actual &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c >actual 2>sub-actual &&\n \t\tsub_expect=\"go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)\" &&\n \t\tgrep \"$sub_expect\" sub-actual\n \t else\n-\t\ttest_must_fail git merge c 2> actual\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge c 2> actual\n \t fi &&\n \tgrep $(cat expect1) actual > /dev/null &&\n \tgrep $(cat expect2) actual > /dev/null &&\n@@ -227,7 +227,7 @@ test_expect_success 'merging should fail for changes that are backwards' '\n \tgit commit -a -m \"f\" &&\n \n \tgit checkout -b test-backward e &&\n-\ttest_must_fail git merge f 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge f 2>actual &&\n \tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n then\n \t\tsub_expect=\"go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)\" &&\n@@ -535,7 +535,7 @@ test_expect_success 'merging should fail with no merge base' '\n \tgit checkout -b b init &&\n \tgit add sub &&\n \tgit commit -m \"b\" &&\n-\ttest_must_fail git merge a 2>actual &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge a 2>actual &&\n \tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n then\n \t\tsub_expect=\"go to submodule (sub), and either merge commit $(git -C sub rev-parse --short HEAD^1)\" &&\ndiff --git a/t/t6439-merge-co-error-msgs.sh b/t/t6439-merge-co-error-msgs.sh\nindex 0cbec57cdab..dcc7d45ac75 100755\n--- a/t/t6439-merge-co-error-msgs.sh\n+++ b/t/t6439-merge-co-error-msgs.sh\n@@ -40,13 +40,13 @@ Aborting\n EOF\n \n test_expect_success 'untracked files overwritten by merge (fast and non-fast forward)' '\n-\ttest_must_fail git merge branch 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge branch 2>out &&\n \ttest_cmp out expect &&\n \tgit commit --allow-empty -m empty &&\n \t(\n \t\tGIT_MERGE_VERBOSITY=0 &&\n \t\texport GIT_MERGE_VERBOSITY &&\n-\t\ttest_must_fail git merge branch 2>out2\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git merge branch 2>out2\n \t) &&\n \techo \"Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed.\" >>expect &&\n \ttest_cmp out2 expect &&\n@@ -69,7 +69,7 @@ test_expect_success 'untracked files or local changes ovewritten by merge' '\n \tgit add two &&\n \tgit add three &&\n \tgit add four &&\n-\ttest_must_fail git merge branch 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git merge branch 2>out &&\n \ttest_cmp out expect\n '\n \n@@ -91,7 +91,7 @@ test_expect_success 'cannot switch branches because of local changes' '\n \tgit checkout main &&\n \techo uno >rep/one &&\n \techo dos >rep/two &&\n-\ttest_must_fail git checkout branch 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&\n \ttest_cmp out expect\n '\n \n@@ -105,7 +105,7 @@ EOF\n \n test_expect_success 'not uptodate file porcelain checkout error' '\n \tgit add rep/one rep/two &&\n-\ttest_must_fail git checkout branch 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&\n \ttest_cmp out expect\n '\n \n@@ -136,7 +136,7 @@ test_expect_success 'not_uptodate_dir porcelain checkout error' '\n \tgit checkout main &&\n \t>rep/untracked-file &&\n \t>rep2/untracked-file &&\n-\ttest_must_fail git checkout branch 2>out &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&\n \ttest_cmp out ../expect\n '\n \n-- \ngitgitgadget\n\n"},{"id":"501418","messageId":"ce725bb8991e8f7e61e85e30967f147a6c00a823.1724238153.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 5/7] t7000: add GIT_ADVICE=1 to advice tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:30Z","receivedAt":"2024-08-21T11:02:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSeveral tests validate the exact output of stderr, including when the stderr\nfile should be empty. In advance of modifying the advice system to only\noutput when stderr is a terminal, force the advice system to output in these\ncases.\n\nIn addition, two more edits were made while in the neighborhood:\n\n 1. In t7002, a redirected stderr was ignored and is now checked as empty.\n\n 2. In t7060 and 7500, the output of \"git status\" has paranthetical messages\n that appear only when advice is enabled, even though it is sent to stdout.\n\n 3. In t7400, a command was checked for failure with \"!\" but is now checked\n via test_must_fail.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7002-mv-sparse-checkout.sh | 85 +++++++++++++++++----------------\n t/t7004-tag.sh | 2 +-\n t/t7060-wtstatus.sh | 11 +++--\n t/t7201-co.sh | 2 +-\n t/t7400-submodule-basic.sh | 2 +-\n t/t7402-submodule-rebase.sh | 3 +-\n t/t7406-submodule-update.sh | 2 +-\n t/t7512-status-help.sh | 4 +-\n t/t7520-ignored-hook-warning.sh | 8 ++--\n 9 files changed, 61 insertions(+), 58 deletions(-)\n\ndiff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh\nindex 57969ce805a..3b194bfa2f7 100755\n--- a/t/t7002-mv-sparse-checkout.sh\n+++ b/t/t7002-mv-sparse-checkout.sh\n@@ -55,13 +55,13 @@ test_expect_success 'mv refuses to move sparse-to-sparse' '\n \tgit reset --hard &&\n \tgit sparse-checkout set --no-cone a &&\n \ttouch b &&\n-\ttest_must_fail git mv b e 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv b e 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo b >>expect &&\n \techo e >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse b e 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse b e 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -72,7 +72,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '\n \n \t# tracked-to-untracked\n \ttouch b &&\n-\tgit mv -k b e 2>stderr &&\n+\tGIT_ADVICE=1 git mv -k b e 2>stderr &&\n \ttest_path_exists b &&\n \ttest_path_is_missing e &&\n \tcat sparse_error_header >expect &&\n@@ -81,7 +81,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse b e 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse b e 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_path_is_missing b &&\n \ttest_path_exists e &&\n@@ -89,7 +89,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '\n \t# tracked-to-tracked\n \tgit reset --hard &&\n \ttouch b &&\n-\tgit mv -k b c 2>stderr &&\n+\tGIT_ADVICE=1 git mv -k b c 2>stderr &&\n \ttest_path_exists b &&\n \ttest_path_is_missing c &&\n \tcat sparse_error_header >expect &&\n@@ -98,7 +98,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse b c 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse b c 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_path_is_missing b &&\n \ttest_path_exists c\n@@ -110,14 +110,14 @@ test_expect_success 'mv refuses to move non-sparse-to-sparse' '\n \tgit sparse-checkout set a &&\n \n \t# tracked-to-untracked\n-\ttest_must_fail git mv a e 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv a e 2>stderr &&\n \ttest_path_exists a &&\n \ttest_path_is_missing e &&\n \tcat sparse_error_header >expect &&\n \techo e >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse a e 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse a e 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_path_is_missing a &&\n \ttest_path_exists e &&\n@@ -125,14 +125,14 @@ test_expect_success 'mv refuses to move non-sparse-to-sparse' '\n \t# tracked-to-tracked\n \trm e &&\n \tgit reset --hard &&\n-\ttest_must_fail git mv a c 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv a c 2>stderr &&\n \ttest_path_exists a &&\n \ttest_path_is_missing c &&\n \tcat sparse_error_header >expect &&\n \techo c >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse a c 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse a c 2>stderr &&\n \ttest_must_be_empty stderr &&\n \ttest_path_is_missing a &&\n \ttest_path_exists c\n@@ -145,12 +145,12 @@ test_expect_success 'mv refuses to move sparse-to-non-sparse' '\n \n \t# tracked-to-untracked\n \ttouch b &&\n-\ttest_must_fail git mv b e 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv b e 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo b >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse b e 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse b e 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -164,7 +164,7 @@ test_expect_success 'recursive mv refuses to move (possible) sparse' '\n \tmkdir sub/dir2 &&\n \ttouch sub/d sub/dir2/e &&\n \n-\ttest_must_fail git mv sub sub2 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub sub2 2>stderr &&\n \tcat sparse_error_header >expect &&\n \tcat >>expect <<-\\EOF &&\n \tsub/d\n@@ -174,7 +174,7 @@ test_expect_success 'recursive mv refuses to move (possible) sparse' '\n \tEOF\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse sub sub2 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub sub2 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit commit -m \"moved sub to sub2\" &&\n \tgit rev-parse HEAD~1:sub >expect &&\n@@ -193,7 +193,7 @@ test_expect_success 'recursive mv refuses to move sparse' '\n \tmkdir sub/dir2 &&\n \ttouch sub/dir2/e &&\n \n-\ttest_must_fail git mv sub sub2 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub sub2 2>stderr &&\n \tcat sparse_error_header >expect &&\n \tcat >>expect <<-\\EOF &&\n \tsub/dir2/e\n@@ -201,7 +201,7 @@ test_expect_success 'recursive mv refuses to move sparse' '\n \tEOF\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n-\tgit mv --sparse sub sub2 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub sub2 2>stderr &&\n \ttest_must_be_empty stderr &&\n \tgit commit -m \"moved sub to sub2\" &&\n \tgit rev-parse HEAD~1:sub >expect &&\n@@ -216,8 +216,9 @@ test_expect_success 'can move files to non-sparse dir' '\n \tgit sparse-checkout set a b c w !/x y/ &&\n \tmkdir -p w x/y &&\n \n-\tgit mv a w/new-a 2>stderr &&\n-\tgit mv b x/y/new-b 2>stderr &&\n+\tGIT_ADVICE=1 git mv a w/new-a 2>stderr &&\n+\ttest_must_be_empty stderr &&\n+\tGIT_ADVICE=1 git mv b x/y/new-b 2>stderr &&\n \ttest_must_be_empty stderr\n '\n \n@@ -228,7 +229,7 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '\n \tgit sparse-checkout set a !/x y/ !x/y/z &&\n \tmkdir -p x/y/z &&\n \n-\ttest_must_fail git mv a x/y/z/new-a 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv a x/y/z/new-a 2>stderr &&\n \techo x/y/z/new-a | cat sparse_error_header - sparse_hint >expect &&\n \ttest_cmp expect stderr\n '\n@@ -237,7 +238,7 @@ test_expect_success 'refuse to move out-of-cone directory without --sparse' '\n \ttest_when_finished \"cleanup_sparse_checkout\" &&\n \tsetup_sparse_checkout &&\n \n-\ttest_must_fail git mv folder1 sub 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv folder1 sub 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo folder1/file1 >>expect &&\n \tcat sparse_hint >>expect &&\n@@ -248,7 +249,7 @@ test_expect_success 'can move out-of-cone directory with --sparse' '\n \ttest_when_finished \"cleanup_sparse_checkout\" &&\n \tsetup_sparse_checkout &&\n \n-\tgit mv --sparse folder1 sub 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse folder1 sub 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_dir sub/folder1 &&\n@@ -259,7 +260,7 @@ test_expect_success 'refuse to move out-of-cone file without --sparse' '\n \ttest_when_finished \"cleanup_sparse_checkout\" &&\n \tsetup_sparse_checkout &&\n \n-\ttest_must_fail git mv folder1/file1 sub 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv folder1/file1 sub 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo folder1/file1 >>expect &&\n \tcat sparse_hint >>expect &&\n@@ -270,7 +271,7 @@ test_expect_success 'can move out-of-cone file with --sparse' '\n \ttest_when_finished \"cleanup_sparse_checkout\" &&\n \tsetup_sparse_checkout &&\n \n-\tgit mv --sparse folder1/file1 sub 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse folder1/file1 sub 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_file sub/file1\n@@ -284,7 +285,7 @@ test_expect_success 'refuse to move sparse file to existing destination' '\n \tgit add folder1 sub/file1 &&\n \tgit sparse-checkout set --cone sub &&\n \n-\ttest_must_fail git mv --sparse folder1/file1 sub 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&\n \techo \"fatal: destination exists, source=folder1/file1, destination=sub/file1\" >expect &&\n \ttest_cmp expect stderr\n '\n@@ -298,7 +299,7 @@ test_expect_success 'move sparse file to existing destination with --force and -\n \tgit add folder1 sub/file1 &&\n \tgit sparse-checkout set --cone sub &&\n \n-\tgit mv --sparse --force folder1/file1 sub 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse --force folder1/file1 sub 2>stderr &&\n \ttest_must_be_empty stderr &&\n \techo \"overwrite\" >expect &&\n \ttest_cmp expect sub/file1\n@@ -308,13 +309,13 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '\n \ttest_when_finished \"cleanup_sparse_checkout\" &&\n \tsetup_sparse_checkout &&\n \n-\ttest_must_fail git mv sub/d folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/d folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/d\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse sub/d folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub/d folder1 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_missing sub/d &&\n@@ -330,18 +331,18 @@ test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '\n \techo \"sub/file1 overwrite\" >sub/file1 &&\n \tgit add sub/file1 &&\n \n-\ttest_must_fail git mv sub/file1 folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/file1 folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/file1\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\ttest_must_fail git mv --sparse sub/file1 folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&\n \techo \"fatal: destination exists in the index, source=sub/file1, destination=folder1/file1\" \\\n \t>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse -f sub/file1 folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse -f sub/file1 folder1 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_missing sub/file1 &&\n@@ -366,18 +367,18 @@ test_expect_success 'move clean path from in-cone to out-of-cone file overwrite'\n \techo \"sub/file1 overwrite\" >sub/file1 &&\n \tgit add sub/file1 &&\n \n-\ttest_must_fail git mv sub/file1 folder1/file1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/file1 folder1/file1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/file1\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\ttest_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&\n \techo \"fatal: destination exists in the index, source=sub/file1, destination=folder1/file1\" \\\n \t>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse -f sub/file1 folder1/file1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse -f sub/file1 folder1/file1 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_missing sub/file1 &&\n@@ -403,19 +404,19 @@ test_expect_success 'move directory with one of the files overwrite' '\n \techo test >sub/dir/file1 &&\n \tgit add sub/dir/file1 &&\n \n-\ttest_must_fail git mv sub/dir folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/dir/e\" >>expect &&\n \techo \"folder1/dir/file1\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\ttest_must_fail git mv --sparse sub/dir folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/dir folder1 2>stderr &&\n \techo \"fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1\" \\\n \t>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse -f sub/dir folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse -f sub/dir folder1 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_missing sub/dir/file1 &&\n@@ -438,13 +439,13 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '\n \tsetup_sparse_checkout &&\n \techo \"modified\" >>sub/d &&\n \n-\ttest_must_fail git mv sub/d folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/d folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/d\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse sub/d folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub/d folder1 2>stderr &&\n \tcat dirty_error_header >expect &&\n \techo \"folder1/d\" >>expect &&\n \tcat dirty_hint >>expect &&\n@@ -462,13 +463,13 @@ test_expect_success 'move dir from in-cone to out-of-cone' '\n \tsetup_sparse_checkout &&\n \tmkdir sub/dir/deep &&\n \n-\ttest_must_fail git mv sub/dir folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/dir/e\" >>expect &&\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse sub/dir folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub/dir folder1 2>stderr &&\n \ttest_must_be_empty stderr &&\n \n \ttest_path_is_missing sub/dir &&\n@@ -487,7 +488,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '\n \techo \"modified\" >>sub/dir/e2 &&\n \techo \"modified\" >>sub/dir/e3 &&\n \n-\ttest_must_fail git mv sub/dir folder1 2>stderr &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&\n \tcat sparse_error_header >expect &&\n \techo \"folder1/dir/e\" >>expect &&\n \techo \"folder1/dir/e2\" >>expect &&\n@@ -495,7 +496,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '\n \tcat sparse_hint >>expect &&\n \ttest_cmp expect stderr &&\n \n-\tgit mv --sparse sub/dir folder1 2>stderr &&\n+\tGIT_ADVICE=1 git mv --sparse sub/dir folder1 2>stderr &&\n \tcat dirty_error_header >expect &&\n \techo \"folder1/dir/e2\" >>expect &&\n \techo \"folder1/dir/e3\" >>expect &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex b1316e62f46..bc216d012cb 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1852,7 +1852,7 @@ test_expect_success 'recursive tagging should give advice' '\n \thint: \tgit tag -f nested annotated-v4.0^{}\n \thint: Disable this message with \"git config advice.nestedTag false\"\n \tEOF\n-\tgit tag -m nested nested annotated-v4.0 2>actual &&\n+\tGIT_ADVICE=1 git tag -m nested nested annotated-v4.0 2>actual &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh\nindex aaeb4a53344..8dfb6885156 100755\n--- a/t/t7060-wtstatus.sh\n+++ b/t/t7060-wtstatus.sh\n@@ -56,9 +56,10 @@ EOF\n \t\tgit rm foo &&\n \t\tgit commit -m delete &&\n \t\ttest_must_fail git merge main &&\n-\t\ttest_must_fail git commit --dry-run >../actual &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail \\\n+\t\t\tgit commit --dry-run >../actual &&\n \t\ttest_cmp ../expect ../actual &&\n-\t\tgit status >../actual &&\n+\t\ttest_env GIT_ADVICE=1 git status >../actual &&\n \t\ttest_cmp ../expect ../actual\n \t)\n '\n@@ -151,7 +152,7 @@ Unmerged paths:\n \n no changes added to commit (use \"git add\" and/or \"git commit -a\")\n EOF\n-\tgit status --untracked-files=no >actual &&\n+\tGIT_ADVICE=1 git status --untracked-files=no >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -185,7 +186,7 @@ Unmerged paths:\n \n no changes added to commit (use \"git add\" and/or \"git commit -a\")\n EOF\n-\tgit status --untracked-files=no >actual &&\n+\tGIT_ADVICE=1 git status --untracked-files=no >actual &&\n \ttest_cmp expected actual\n '\n \n@@ -210,7 +211,7 @@ Unmerged paths:\n \n Untracked files not listed (use -u option to show untracked files)\n EOF\n-\tgit status --untracked-files=no >actual &&\n+\tGIT_ADVICE=1 git status --untracked-files=no >actual &&\n \ttest_cmp expected actual &&\n \tgit reset --hard &&\n \tgit checkout main\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 2d984eb4c6a..9ee2374e3d2 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD' '\n \trev=$(git rev-parse --short renamer^) &&\n \tgit checkout -f renamer &&\n \tgit clean -f &&\n-\tgit checkout renamer^ 2>messages &&\n+\tGIT_ADVICE=1 git checkout renamer^ 2>messages &&\n \tgrep \"HEAD is now at $rev\" messages &&\n \ttest_line_count -gt 1 messages &&\n \tH=$(git rev-parse --verify HEAD) &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 098d8833b65..95e4bacd19e 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -219,7 +219,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\techo \"*\" > .gitignore &&\n \t\tgit add --force .gitignore &&\n \t\tgit commit -m\"Ignore everything\" &&\n-\t\t! git submodule add \"$submodurl\" submod >actual 2>&1 &&\n+\t\ttest_env GIT_ADVICE=1 test_must_fail git submodule add \"$submodurl\" submod >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\ndiff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh\nindex aa2fdc31d1a..b155bd6e1c3 100755\n--- a/t/t7402-submodule-rebase.sh\n+++ b/t/t7402-submodule-rebase.sh\n@@ -116,7 +116,8 @@ test_expect_success 'rebasing submodule that should conflict' '\n \ttest_tick &&\n \tgit commit -m fourth &&\n \n-\ttest_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&\n+\ttest_env GIT_ADVICE=1 test_must_fail git rebase \\\n+\t\t--onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&\n \tgit ls-files -s submodule >actual &&\n \t(\n \t\tcd submodule &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 297c6c3b5cc..560eeea9c99 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -206,7 +206,7 @@ test_expect_success 'submodule update should fail due to local changes' '\n \t (cd submodule &&\n \t compare_head\n \t ) &&\n-\t test_must_fail git submodule update submodule 2>../actual.raw\n+\t test_env GIT_ADVICE=1 test_must_fail git submodule update submodule 2>../actual.raw\n \t) &&\n \tsed \"s/^> //\" >expect <<-\\EOF &&\n \t> error: Your local changes to the following files would be overwritten by checkout:\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex cdd5f2c6979..de277257d50 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -847,7 +847,7 @@ EOF\n test_expect_success 'status shows cherry-pick with invalid oid' '\n \tmkdir .git/sequencer &&\n \ttest_write_lines \"pick invalid-oid\" >.git/sequencer/todo &&\n-\tgit status --untracked-files=no >actual 2>err &&\n+\tGIT_ADVICE=1 git status --untracked-files=no >actual 2>err &&\n \tgit cherry-pick --quit &&\n \ttest_must_be_empty err &&\n \ttest_cmp expected actual\n@@ -856,7 +856,7 @@ test_expect_success 'status shows cherry-pick with invalid oid' '\n test_expect_success 'status does not show error if .git/sequencer is a file' '\n \ttest_when_finished \"rm .git/sequencer\" &&\n \ttest_write_lines hello >.git/sequencer &&\n-\tgit status --untracked-files=no 2>err &&\n+\tGIT_ADVICE=1 git status --untracked-files=no 2>err &&\n \ttest_must_be_empty err\n '\n \ndiff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh\nindex 3b63c34a309..21e088894c3 100755\n--- a/t/t7520-ignored-hook-warning.sh\n+++ b/t/t7520-ignored-hook-warning.sh\n@@ -12,27 +12,27 @@ test_expect_success setup '\n '\n \n test_expect_success 'no warning if hook is not ignored' '\n-\tgit commit --allow-empty -m \"more\" 2>message &&\n+\tGIT_ADVICE=1 git commit --allow-empty -m \"more\" 2>message &&\n \ttest_grep ! -e \"hook was ignored\" message\n '\n \n test_expect_success POSIXPERM 'warning if hook is ignored' '\n \ttest_hook --disable pre-commit &&\n-\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\tGIT_ADVICE=1 git commit --allow-empty -m \"even more\" 2>message &&\n \ttest_grep -e \"hook was ignored\" message\n '\n \n test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '\n \ttest_config advice.ignoredHook false &&\n \ttest_hook --disable pre-commit &&\n-\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\tGIT_ADVICE=1 git commit --allow-empty -m \"even more\" 2>message &&\n \ttest_grep ! -e \"hook was ignored\" message\n '\n \n test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '\n \ttest_hook --remove pre-commit &&\n \ttest_unconfig advice.ignoredHook &&\n-\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\tGIT_ADVICE=1 git commit --allow-empty -m \"even more\" 2>message &&\n \ttest_grep ! -e \"hook was ignored\" message\n '\n \n-- \ngitgitgadget\n\n"},{"id":"501419","messageId":"960d1ec11ece90a29da8a909243aeeca0fdc04fb.1724238153.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 6/7] t7508/12: set GIT_ADVICE=1 across all tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:31Z","receivedAt":"2024-08-21T11:02:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe output of 'git status' changes depending on the availability of advice,\neven though the messages are to stdout. Since this test script is all about\ntesting the output of 'git status' including the existence (or lack of)\nthese messages, set the GIT_ADVICE environment globally across the script.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7508-status.sh | 4 ++++\n t/t7512-status-help.sh | 4 ++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 773383fefb5..7158ee57f37 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -9,6 +9,10 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n+# 'git status' output changes depending on the availability of advice,\n+# so force its output to enable advice, even though it goes to stdout.\n+GIT_ADVICE=1 && export GIT_ADVICE\n+\n test_expect_success 'status -h in broken repository' '\n \tgit config --global advice.statusuoption false &&\n \tmkdir broken &&\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex de277257d50..1d9676bb3e2 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -17,6 +17,10 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n set_fake_editor\n \n+# 'git status' output changes depending on the availability of advice,\n+# so force its output to enable advice, even though it goes to stdout.\n+GIT_ADVICE=1 && export GIT_ADVICE\n+\n test_expect_success 'prepare for conflicts' '\n \tgit config --global advice.statusuoption false &&\n \ttest_commit init main.txt init &&\n-- \ngitgitgadget\n\n"},{"id":"501420","messageId":"25d769903b2ab4a4c454929bf6378751bd366a37.1724238153.git.gitgitgadget@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"[PATCH 7/7] advice: refuse to output if stderr not TTY","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-21T11:02:32Z","receivedAt":"2024-08-21T11:02:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe advice system is intended to help end users around corner cases or other\ndifficult spots when using the Git tool. As such, they are added without\nconsidering the possibility that they could break scripts or external tools\nthat execute Git processes and then parse the output.\n\nI will not debate the merit of tools parsing stderr, but instead attempt to\nbe helpful to tool authors by avoiding these behavior changes across Git\nversions.\n\nIn b79deeb5544 (advice: add --no-advice global option, 2024-05-03), the\n--no-advice option was presented as a way to help tool authors specify that\nthey do not want any advice messages. As part of this implementation, the\nGIT_ADVICE environment variable is given as a way to communicate the desire\nfor advice (=1) or no advice (=0) and pass that along to all child\nprocesses.\n\nHowever, both the --no-advice option and the GIT_ADVICE environment variable\nrequire the tool author to change how they interact with Git to gain this\nprotection.\n\nIf Git instead disables the advice system when stderr is not a terminal,\nthen tool authors benefit immediately.\n\nIt is important, though, to let interested users force advice to be enabled,\neven when redirecting stderr to a non-terminal file. Be sure to test this by\nensuring GIT_ADVICE=1 forces advice to be written to non-terminals.\n\nThe changes leading up to this already set GIT_ADVICE=1 in all other test\nscripts that care about the advice being output (or not).\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/advice.txt | 9 ++++++---\n advice.c | 4 +++-\n t/t0018-advice.sh | 18 +++++++++++++-----\n 3 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 0ba89898207..4946a8aff8d 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -1,8 +1,11 @@\n advice.*::\n \tThese variables control various optional help messages designed to\n-\taid new users. When left unconfigured, Git will give the message\n-\talongside instructions on how to squelch it. You can tell Git\n-\tthat you do not need the help message by setting these to `false`:\n+\taid new users. These are only output to `stderr` when it is a\n+\tterminal.\n++\n+When left unconfigured, Git will give the message alongside instructions\n+on how to squelch it. You can tell Git that you do not need the help\n+message by setting these to `false`:\n +\n --\n \taddEmbeddedRepo::\ndiff --git a/advice.c b/advice.c\nindex 6b879d805c0..05cf467b680 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -133,7 +133,9 @@ int advice_enabled(enum advice_type type)\n \tstatic int globally_enabled = -1;\n \n \tif (globally_enabled < 0)\n-\t\tglobally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1);\n+\t\tglobally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, -1);\n+\tif (globally_enabled < 0)\n+\t\tglobally_enabled = isatty(2);\n \tif (!globally_enabled)\n \t\treturn 0;\n \ndiff --git a/t/t0018-advice.sh b/t/t0018-advice.sh\nindex fac52322a7f..c63ef070a76 100755\n--- a/t/t0018-advice.sh\n+++ b/t/t0018-advice.sh\n@@ -8,7 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-test_expect_success 'advice should be printed when config variable is unset' '\n+test_expect_success TTY 'advice should be printed when config variable is unset' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n \thint: Disable this message with \"git config advice.nestedTag false\"\n@@ -17,7 +17,7 @@ test_expect_success 'advice should be printed when config variable is unset' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'advice should be printed when config variable is set to true' '\n+test_expect_success TTY 'advice should be printed when config variable is set to true' '\n \tcat >expect <<-\\EOF &&\n \thint: This is a piece of advice\n \tEOF\n@@ -26,13 +26,13 @@ test_expect_success 'advice should be printed when config variable is set to tru\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'advice should not be printed when config variable is set to false' '\n+test_expect_success TTY 'advice should not be printed when config variable is set to false' '\n \ttest_config advice.nestedTag false &&\n \ttest-tool advise \"This is a piece of advice\" 2>actual &&\n \ttest_must_be_empty actual\n '\n \n-test_expect_success 'advice should not be printed when --no-advice is used' '\n+test_expect_success TTY 'advice should not be printed when --no-advice is used' '\n \tq_to_tab >expect <<-\\EOF &&\n \tOn branch trunk\n \n@@ -54,7 +54,7 @@ test_expect_success 'advice should not be printed when --no-advice is used' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '\n+test_expect_success TTY 'advice should not be printed when GIT_ADVICE is set to false' '\n \tq_to_tab >expect <<-\\EOF &&\n \tOn branch trunk\n \n@@ -76,6 +76,8 @@ test_expect_success 'advice should not be printed when GIT_ADVICE is set to fals\n \ttest_cmp expect actual\n '\n \n+# This test also verifies that GIT_ADVICE=1 ignores the requirement\n+# that stderr is a terminal.\n test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '\n \tq_to_tab >expect <<-\\EOF &&\n \tOn branch trunk\n@@ -99,4 +101,10 @@ test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'advice should not be printed when stderr is not a terminal' '\n+\ttest_config advice.nestedTag true &&\n+\ttest-tool advise \"This is a piece of advice\" 2>actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"501436","messageId":"20240821154001.GA506216@coredump.intra.peff.net","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-21T15:40:01Z","receivedAt":"2024-08-21T15:40:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 21, 2024 at 11:02:25AM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> Advice is supposed to be for humans, not machines. Why do we output it when\n> stderr is not a terminal? Let's stop doing that.\n> \n> I'm labeling this as an RFC because I believe there is some risk with this\n> change. In particular, this does change behavior to reduce the output that\n> some scripts may depend upon. But this output is not intended to be locked\n> in and we add or edit advice messages without considering this impact, so\n> there is risk in the existing system already.\n\nPlaying devil's advocate for a moment: what about programs that read\nstderr but intend to relay the output to the user?\n\nFor example, programs running on the server side of a push are spawned\nby receive-pack with their stderr fed into a muxer that ships it to the\nclient, who then dumps it to the user's terminal. Would we ever want to\nsee their advice?\n\nMy guess is \"conceivably yes\", though I don't know of a specific example\n(and in fact, I've seen the \"your hook was ignored because it's not\nexecutable\" advice coming from a server, which was actually more of an\nannoyance on the client side).\n\nDitto for upload-pack. Another possible place where it matters:\ninterfaces that wrap Git and collect the output to show to the user. I\ndon't use git-gui, but I'd imagine it does this in some places.\n\nLooking over patch 7, I think the escape hatch for all of these cases\nwould be setting GIT_ADVICE=1. Which isn't too bad, but it does require\nsome action. I'm not sure if it is worth it (but then, I am not all that\nsympathetic to the script you mentioned that was trying to be too clever\nabout parsing stderr).\n\n-Peff\n"},{"id":"501443","messageId":"xmqqbk1l25p3.fsf@gitster.g","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-21T16:36:56Z","receivedAt":"2024-08-21T16:37:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> Advice is supposed to be for humans, not machines. Why do we output it when\n> stderr is not a terminal? Let's stop doing that.\n\nLast night while skimming the series on my phone (read: not a real\nreview at all), I found it very annoying that GIT_ADVICE=1 had to be\nsprinkled all over the place. I wonder if we want to instead set\nand export it in t/test-lib.sh and turn it off as needed?\n\nThe end-to-end tests we have are primarily to guarantee the\ncontinuity of the end-user experience by humans, and ensuring that\nan advice message is given when appropriate and it does not get\nshown otherwise is very much inherent part of them. An alternative\nworkaround to counteract the breakage this series causes of course\nis to run everything under test_terminal and it probably is much\nmore kosher philosophically ;-), but compared to that, globally\ndisabling the \"if (!isatty(2))\" while running the tests, and\ntemporarily lifting that disabling during tests of the new feature\nadded by this series would be easier to reason about, I would\nsuspect.\n\n> This series is motivated by an internal tool breaking due to the advice\n> message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index\n> expands, 2024-07-08). This tool is assuming that any output to stderr is an\n> error, and in this case is attempting to parse it to determine what kind of\n> error (warning, error, or failure).\n\nThe \"anything on stderr is an error\" attitude needs to be fixed\nregardless of where it comes from (tcl/tk scripts have, or at least\nused to have, the tendency, which I found annoying), but regardless,\nI thought we added a mechanism to squelch all advice messages for\nthis exact purpose at f0e21837 (Merge branch 'jl/git-no-advice',\n2024-05-16). Why isn't the tool using the mechanism that already\nexists?\n\nI would have supported the behaviour proposed by this series 100% if\nit were on the table when we were introducing the advise mechanism,\nbut unfortunately nobody seemed have suggested it back then. I am\nwilling to go with an \"experiment\" to change the behaviour,\ndeliberately breaking \"backward compatibility\", if we have a wide\nsupport here during the review period. FWIW, I think any scripts\nthat scrape the advice messages are already broken.\n\n\n"},{"id":"501444","messageId":"xmqq7cc925l3.fsf@gitster.g","threadId":"61987","inReplyTo":"20240821154001.GA506216@coredump.intra.peff.net","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-21T16:39:20Z","receivedAt":"2024-08-21T16:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Playing devil's advocate for a moment: what about programs that read\n> stderr but intend to relay the output to the user?\n>\n> For example, programs running on the server side of a push are spawned\n> by receive-pack with their stderr fed into a muxer that ships it to the\n> client, who then dumps it to the user's terminal. Would we ever want to\n> see their advice?\n>\n> My guess is \"conceivably yes\", though I don't know of a specific example\n> (and in fact, I've seen the \"your hook was ignored because it's not\n> executable\" advice coming from a server, which was actually more of an\n> annoyance on the client side).\n\nAh, I should have waited to think about the topic before reading\nwhat you wrote. Yes, this is a huge downside.\n\n> Looking over patch 7, I think the escape hatch for all of these cases\n> would be setting GIT_ADVICE=1. Which isn't too bad, but it does require\n> some action. I'm not sure if it is worth it (but then, I am not all that\n> sympathetic to the script you mentioned that was trying to be too clever\n> about parsing stderr).\n\nThis too.\n"},{"id":"501470","messageId":"ZsbUwZM0ZPuWIlS7@lan","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Gabor Gombas","fromEmail":"gombasgg@gmail.com","sentAt":"2024-08-22T06:03:45Z","receivedAt":"2024-08-22T06:03:49Z","isPatch":true,"sender":{"key":"gombasgg@gmail.com","avatar":null},"body":"Hi,\n\nOn Wed, Aug 21, 2024 at 11:02:25AM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> Advice is supposed to be for humans, not machines. Why do we output it when\n> stderr is not a terminal? Let's stop doing that.\n\nReally bad idea. \"/some/script 2>&1 | tee /some/where | less\" is a\ncommon, generic debug construct (with countless variations of the exact\ncommands in the pipe - this is Unix, after all). If /some/script happens\nto run git, then I _do_ want to see all the diagnostic messages it might\nproduce, both recorded at /some/where, and displayed by \"less\".\n\nRegards,\nGabor\n"},{"id":"501471","messageId":"ZsbYYo3pLUAmBU0e@tanuki","threadId":"61987","inReplyTo":"xmqqbk1l25p3.fsf@gitster.g","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-22T06:19:14Z","receivedAt":"2024-08-22T06:19:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 21, 2024 at 09:36:56AM -0700, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n> > Advice is supposed to be for humans, not machines. Why do we output it when\n> > stderr is not a terminal? Let's stop doing that.\n> \n> Last night while skimming the series on my phone (read: not a real\n> review at all), I found it very annoying that GIT_ADVICE=1 had to be\n> sprinkled all over the place. I wonder if we want to instead set\n> and export it in t/test-lib.sh and turn it off as needed?\n> \n> The end-to-end tests we have are primarily to guarantee the\n> continuity of the end-user experience by humans, and ensuring that\n> an advice message is given when appropriate and it does not get\n> shown otherwise is very much inherent part of them. An alternative\n> workaround to counteract the breakage this series causes of course\n> is to run everything under test_terminal and it probably is much\n> more kosher philosophically ;-), but compared to that, globally\n> disabling the \"if (!isatty(2))\" while running the tests, and\n> temporarily lifting that disabling during tests of the new feature\n> added by this series would be easier to reason about, I would\n> suspect.\n> \n> > This series is motivated by an internal tool breaking due to the advice\n> > message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index\n> > expands, 2024-07-08). This tool is assuming that any output to stderr is an\n> > error, and in this case is attempting to parse it to determine what kind of\n> > error (warning, error, or failure).\n> \n> The \"anything on stderr is an error\" attitude needs to be fixed\n> regardless of where it comes from (tcl/tk scripts have, or at least\n> used to have, the tendency, which I found annoying), but regardless,\n> I thought we added a mechanism to squelch all advice messages for\n> this exact purpose at f0e21837 (Merge branch 'jl/git-no-advice',\n> 2024-05-16). Why isn't the tool using the mechanism that already\n> exists?\n> \n> I would have supported the behaviour proposed by this series 100% if\n> it were on the table when we were introducing the advise mechanism,\n> but unfortunately nobody seemed have suggested it back then. I am\n> willing to go with an \"experiment\" to change the behaviour,\n> deliberately breaking \"backward compatibility\", if we have a wide\n> support here during the review period. FWIW, I think any scripts\n> that scrape the advice messages are already broken.\n\nI continue to believe that the biggest issue in this context is that\nthere is no proper interface between Git and its caller that would allow\nthe caller to learn about errors in a machine-parseable way. Matching\nerror messages against regular expressions is bad, and can easily be\nbroken by the output changing in whatever way. This may be because the\nerror message itself was changed, or it may be because we have started\nto show advice messages. It's extremely fragile, and from my point of\nview there is no good way to classify errors right now.\n\nI won't argue that checking whether stderr is empty or not is good -- it\nalmost certainly feels wrong to me. But that's only one small part of a\nmore widespread issue. Having structured error handling in Git, e.g. via\na new structure that represents errors as discussed a couple of months\nago [1] would go a long way. I didn't quite like the approach chosen by\nthat patch series, but think that the idea certainly has merit.\n\nThe other question is why advice is being shown in the first place. In\ntheory, all one should ever use in scripted usecases are plumbing tools.\nAnd as plumbing tools are explicitly not designed for users, they should\nnever show advice in the first place. I guess chances are high though\nthat the scripts in question used porcelain. That is also understandable\nthough: our plumbing tools are often not as powerful as the porcelain\nones, which has been lamented on the mailing list several times.\n\nSo I certainly get the sentiment of this patch series, but feel like we\ncontinue to work around the underlying problems. Those are rooted rather\ndeep though, so fixing them is nothing we can do in a release or two,\nbut rather on the order of years. Meanwhile I guess we have to find\nshort-term solutions.\n\nPatrick\n\n[1]: https://lore.kernel.org/git/pull.1666.git.git.1708241612.gitgitgadget@gmail.com/\n"},{"id":"501534","messageId":"e90949ed-8065-4498-9ddb-3d5c6afa7b35@gmail.com","threadId":"61987","inReplyTo":"pull.1776.git.1724238152.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-08-22T13:15:42Z","receivedAt":"2024-08-22T13:15:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/24 7:02 AM, Derrick Stolee via GitGitGadget wrote:\n> Advice is supposed to be for humans, not machines. Why do we output it when\n> stderr is not a terminal? Let's stop doing that.\n> \n> I'm labeling this as an RFC because I believe there is some risk with this\n> change. \n\nThanks, all, for the feedback about the risk of making such a change. I\nagree that we should not pursue this direction.\n\nThe main issues are:\n\n 1. Some tools create a wrapper around Git and may want to supply the\n advice to the user by parsing stderr.\n\n 2. The advice system has been on for a long time and we cannot know\n where other dependencies could be for it.\n\nI'll abandon this RFC, but plan on the following action items:\n\n * Document GIT_ADVICE in Documentation/git.exe.\n\n * Modify Documentation/config/advice.txt to mention GIT_ADVICE and\n recommend that automated tools calling Git commands set it to zero.\n\n * If we have a place to recommend best practices for automation\n executing Git commands, then I would add GIT_ADVICE=0 as a\n recommendation there. I couldn't find one myself. Do we have one?\n\nThanks!\n-Stolee\n\n"},{"id":"501548","messageId":"xmqqcym0sexi.fsf@gitster.g","threadId":"61987","inReplyTo":"e90949ed-8065-4498-9ddb-3d5c6afa7b35@gmail.com","subject":"Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-22T16:25:13Z","receivedAt":"2024-08-22T16:25:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/21/24 7:02 AM, Derrick Stolee via GitGitGadget wrote:\n>> Advice is supposed to be for humans, not machines. Why do we output it when\n>> stderr is not a terminal? Let's stop doing that.\n>> I'm labeling this as an RFC because I believe there is some risk\n>> with this\n>> change. \n>\n> Thanks, all, for the feedback about the risk of making such a change. I\n> agree that we should not pursue this direction.\n>\n> The main issues are:\n>\n> 1. Some tools create a wrapper around Git and may want to supply the\n> advice to the user by parsing stderr.\n\nOr they may just pass it through to the user without even parsing.\n\n> 2. The advice system has been on for a long time and we cannot know\n> where other dependencies could be for it.\n>\n> I'll abandon this RFC, but plan on the following action items:\n>\n> * Document GIT_ADVICE in Documentation/git.exe.\n>\n> * Modify Documentation/config/advice.txt to mention GIT_ADVICE and\n> recommend that automated tools calling Git commands set it to zero.\n\nFWIW, not documenting it was very much deliberate to discourage\nfolks placing it in their ~/.login file. I am OK with the above as\nlong as \"this is for tools\" is stressed well enough.\n"}]}