threads / rfc / 61987

patch, 7 parts[RFC] advice: refuse to output if stderr not TTY

Subject: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

## tl;dr

15 messages between Aug 21, 2024 and Aug 22, 2024. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · lore

Advice is supposed to be for humans, not machines. Why do we output it when stderr is not a terminal? Let's stop doing that.

I'm labeling this as an RFC because I believe there is some risk with this change. In particular, this does change behavior to reduce the output that some scripts may depend upon. But this output is not intended to be locked in and we add or edit advice messages without considering this impact, so there is risk in the existing system already.

This series is motivated by an internal tool breaking due to the advice message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index expands, 2024-07-08). This tool is assuming that any output to stderr is an error, and in this case is attempting to parse it to determine what kind of error (warning, error, or failure).

I've recommended that the tool author remove the advice message for now, but I'd like to help other tool authors avoid this surprise.

I read the thread for the --no-advice option [1] looking to see if this was presented as an option, but did not see it as part of that review. I hope that this is not considered a breaking change for users, but I could see the argument for that.

[1] https://lore.kernel.org/git/20240424035857.84583-1-james@jamesliu.io/t/#u

 * Patches 1-5 are preparation patches to make the test library work to test
   the advice system after the final patch. These are split by test file
   name to reduce the size of the patches, but could be squashed into a
   megapatch if necessary. This is usually a simple addition of the
   GIT_ADVICE=1 environment variable, but there were some changes made to
   those lines to be more correct as necessary.
 * Patch 6 highlights the fact that 'git status' uses advice_enabled() to
   determine if it should print certain parenthetical results. See
   format_tracking_info() in remote.c for an example. This output doesn't
   use the advise() method, but instead appends to a string buffer that is
   later sent to stdout. (If we think this part of the change is too risky,
   then we could move the isatty() out of advice_enabled() and into
   advise(), but that would not match the existing behavior of what is
   blocked by --no-advice.)
 * Patch 7 modifies advice_enabled() to disable when isatty(2) is false and
   GIT_ADVICE is unset.

Thanks, - Stolee

Derrick Stolee (7):
  t1000-2000: add GIT_ADVICE=1 for advice tests
  t3000-4000: add GIT_ADVICE=1 to advice tests
  t5000: add GIT_ADVICE=1 to advice tests
  t6000: add GIT_ADVICE=1 to advice tests
  t7000: add GIT_ADVICE=1 to advice tests
  t7508/12: set GIT_ADVICE=1 across all tests
  advice: refuse to output if stderr not TTY

 Documentation/config/advice.txt           |  9 ++-
 advice.c                                  |  4 +-
 t/lib-httpd.sh                            |  2 +-
 t/t0018-advice.sh                         | 18 +++--
 t/t1092-sparse-checkout-compatibility.sh  | 18 ++---
 t/t2020-checkout-detach.sh                | 25 ++++---
 t/t2024-checkout-dwim.sh                  |  5 +-
 t/t2060-switch.sh                         |  4 +-
 t/t2204-add-ignored.sh                    |  8 +--
 t/t2400-worktree-add.sh                   | 12 ++--
 t/t3200-branch.sh                         |  4 +-
 t/t3404-rebase-interactive.sh             |  2 +-
 t/t3501-revert-cherry-pick.sh             |  2 +-
 t/t3507-cherry-pick-conflict.sh           |  4 +-
 t/t3510-cherry-pick-sequence.sh           |  6 +-
 t/t3600-rm.sh                             | 12 ++--
 t/t3602-rm-sparse-checkout.sh             | 18 ++---
 t/t3700-add.sh                            |  6 +-
 t/t3705-add-sparse-checkout.sh            | 32 ++++-----
 t/t4150-am.sh                             | 14 ++--
 t/t5505-remote.sh                         |  5 +-
 t/t5520-pull.sh                           |  4 +-
 t/t5541-http-push-smart.sh                |  6 +-
 t/t6001-rev-list-graft.sh                 |  4 +-
 t/t6050-replace.sh                        |  6 +-
 t/t6436-merge-overwrite.sh                |  6 +-
 t/t6437-submodule-merge.sh                | 16 ++---
 t/t6439-merge-co-error-msgs.sh            | 12 ++--
 t/t7002-mv-sparse-checkout.sh             | 85 ++++++++++++-----------
 t/t7004-tag.sh                            |  2 +-
 t/t7060-wtstatus.sh                       | 11 +--
 t/t7201-co.sh                             |  2 +-
 t/t7400-submodule-basic.sh                |  2 +-
 t/t7402-submodule-rebase.sh               |  3 +-
 t/t7406-submodule-update.sh               |  2 +-
 t/t7500-commit-template-squash-signoff.sh |  3 +-
 t/t7508-status.sh                         |  4 ++
 t/t7512-status-help.sh                    |  8 ++-
 t/t7520-ignored-hook-warning.sh           |  8 +--
 39 files changed, 214 insertions(+), 180 deletions(-)

base-commit: bb9c16bd4f1a9a00799e10c81ee6506cf468c0c7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1776%2Fderrickstolee%2Fadvice-tty-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1776/derrickstolee/advice-tty-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1776

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/7] t1000-2000: add GIT_ADVICE=1 for advice tests

From: Derrick Stolee <derrickstolee@github.com>

Several tests validate the exact output of stderr, including when the stderr file should be empty. In advance of modifying the advice system to only output when stderr is a terminal, force the advice system to output in these cases.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t1092-sparse-checkout-compatibility.sh  | 18 ++++++++--------
 t/t2020-checkout-detach.sh                | 25 ++++++++++++++---------
 t/t2024-checkout-dwim.sh                  |  5 +++--
 t/t2060-switch.sh                         |  4 ++--
 t/t2204-add-ignored.sh                    |  8 ++++----
 t/t2400-worktree-add.sh                   | 12 +++++------
 t/t7500-commit-template-squash-signoff.sh |  3 ++-
 7 files changed, 41 insertions(+), 34 deletions(-)

Show changes to 7 files +41 −34

t/t1092-sparse-checkout-compatibility.sh, t/t2020-checkout-detach.sh, t/t2024-checkout-dwim.sh, t/t2060-switch.sh, t/t2204-add-ignored.sh, t/t2400-worktree-add.sh, t/t7500-commit-template-squash-signoff.sh

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index a2c0e1b4dcc..b5183ea7c83 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -411,10 +411,10 @@ test_expect_success 'add outside sparse cone' '
 	run_on_sparse mkdir folder1 &&
 	run_on_sparse ../edit-contents folder1/a &&
 	run_on_sparse ../edit-contents folder1/newfile &&
-	test_sparse_match test_must_fail git add folder1/a &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder1/a &&
-	test_sparse_match test_must_fail git add folder1/newfile &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/newfile &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder1/newfile
 '
@@ -466,13 +466,13 @@ test_expect_success 'status/add: outside sparse cone' '
 	test_sparse_match git status --porcelain=v2 &&
 
 	# Adding the path outside of the sparse-checkout cone should fail.
-	test_sparse_match test_must_fail git add folder1/a &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder1/a &&
 	test_all_match git add --refresh folder1/a &&
 	test_must_be_empty sparse-checkout-err &&
 	test_sparse_unstaged folder1/a &&
-	test_sparse_match test_must_fail git add folder1/new &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/new &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder1/new &&
 	test_sparse_match git add --sparse folder1/a &&
@@ -1018,7 +1018,7 @@ test_expect_success 'merge with conflict outside cone' '
 	test_all_match git status --porcelain=v2 &&
 
 	# 2. Add the file with conflict markers
-	test_sparse_match test_must_fail git add folder1/a &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder1/a &&
 	test_all_match git add --sparse folder1/a &&
@@ -1027,7 +1027,7 @@ test_expect_success 'merge with conflict outside cone' '
 	# 3. Rename the file to another sparse filename and
 	#    accept conflict markers as resolved content.
 	run_on_all mv folder2/a folder2/z &&
-	test_sparse_match test_must_fail git add folder2 &&
+	test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder2 &&
 	grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_sparse_unstaged folder2/z &&
 	test_all_match git add --sparse folder2 &&
@@ -1058,7 +1058,7 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' '
 		# NEEDSWORK: Even though the merge conflict removed the
 		# SKIP_WORKTREE bit from the index entry for folder1/a, we should
 		# warn that this is a problematic add.
-		test_sparse_match test_must_fail git add folder1/a &&
+		test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder1/a &&
 		grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 		test_sparse_unstaged folder1/a &&
 		test_all_match git add --sparse folder1/a &&
@@ -1070,7 +1070,7 @@ test_expect_success 'cherry-pick/rebase with conflict outside cone' '
 		# outside of the sparse-checkout cone and does not match an
 		# existing index entry with the SKIP_WORKTREE bit cleared.
 		run_on_all mv folder2/a folder2/z &&
-		test_sparse_match test_must_fail git add folder2 &&
+		test_env GIT_ADVICE=1 test_sparse_match test_must_fail git add folder2 &&
 		grep "Disable or modify the sparsity rules" sparse-checkout-err &&
 		test_sparse_unstaged folder2/z &&
 		test_all_match git add --sparse folder2 &&
@@ -2341,7 +2341,7 @@ test_expect_success 'advice.sparseIndexExpanded' '
 	git -C sparse-index sparse-checkout set deep/deeper1 &&
 	mkdir -p sparse-index/deep/deeper2/deepest &&
 	touch sparse-index/deep/deeper2/deepest/bogus &&
-	git -C sparse-index status 2>err &&
+	GIT_ADVICE=1 git -C sparse-index status 2>err &&
 	grep "The sparse index is expanding to a full index" err
 '
 
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 8d90d028504..43ee72b19bd 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -175,7 +175,7 @@ test_expect_success 'tracking count is accurate after orphan check' '
 	git config branch.child.remote . &&
 	git config branch.child.merge refs/heads/main &&
 	git checkout child^ &&
-	git checkout child >stdout &&
+	GIT_ADVICE=1 git checkout child >stdout &&
 	test_cmp expect stdout &&
 
 	git checkout --detach child >stdout &&
@@ -251,15 +251,17 @@ test_expect_success 'describe_detached_head prints no SHA-1 ellipsis when not as
 	# Various ways of *not* asking for ellipses
 
 	sane_unset GIT_PRINT_SHA1_ELLIPSIS &&
-	git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 1st_detach actual &&
 
-	GIT_PRINT_SHA1_ELLIPSIS="no" git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1  GIT_PRINT_SHA1_ELLIPSIS="no" git -c 'core.abbrev=12' \
+		checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 2nd_detach actual &&
 
-	GIT_PRINT_SHA1_ELLIPSIS= git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS= git -c 'core.abbrev=12' \
+		checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 3rd_detach actual &&
 
@@ -270,17 +272,17 @@ test_expect_success 'describe_detached_head prints no SHA-1 ellipsis when not as
 	check_not_detached &&
 
 	# Make no mention of the env var at all
-	git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 1st_detach actual &&
 
 	GIT_PRINT_SHA1_ELLIPSIS='nope' &&
-	git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 2nd_detach actual &&
 
 	GIT_PRINT_SHA1_ELLIPSIS=nein &&
-	git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 3rd_detach actual &&
 
@@ -333,15 +335,18 @@ test_expect_success 'describe_detached_head does print SHA-1 ellipsis when asked
 	# Various ways of asking for ellipses...
 	# The user can just use any kind of quoting (including none).
 
-	GIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=yes git -c 'core.abbrev=12' \
+		checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 1st_detach actual &&
 
-	GIT_PRINT_SHA1_ELLIPSIS=Yes git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=Yes git -c 'core.abbrev=12' \
+		checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 2nd_detach actual &&
 
-	GIT_PRINT_SHA1_ELLIPSIS=YES git -c 'core.abbrev=12' checkout HEAD^ >actual 2>&1 &&
+	GIT_ADVICE=1 GIT_PRINT_SHA1_ELLIPSIS=YES git -c 'core.abbrev=12' \
+		checkout HEAD^ >actual 2>&1 &&
 	check_detached &&
 	test_cmp 3rd_detach actual &&
 
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index 2caada3d834..56be88b1620 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -103,11 +103,12 @@ test_expect_success 'when arg matches multiple remotes, do not fallback to inter
 test_expect_success 'checkout of branch from multiple remotes fails with advice' '
 	git checkout -B main &&
 	test_might_fail git branch -D foo &&
-	test_must_fail git checkout foo 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git checkout foo 2>stderr &&
 	test_branch main &&
 	status_uno_is_clean &&
 	test_grep "^hint: " stderr &&
-	test_must_fail git -c advice.checkoutAmbiguousRemoteBranchName=false \
+	test_env GIT_ADVICE=1 test_must_fail git \
+		-c advice.checkoutAmbiguousRemoteBranchName=false \
 		checkout foo 2>stderr &&
 	test_branch main &&
 	status_uno_is_clean &&
diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
index 77b2346291b..d84b3accf0e 100755
--- a/t/t2060-switch.sh
+++ b/t/t2060-switch.sh
@@ -34,13 +34,13 @@ test_expect_success 'switch and detach' '
 '
 
 test_expect_success 'suggestion to detach' '
-	test_must_fail git switch main^{commit} 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git switch main^{commit} 2>stderr &&
 	grep "try again with the --detach option" stderr
 '
 
 test_expect_success 'suggestion to detach is suppressed with advice.suggestDetachingHead=false' '
 	test_config advice.suggestDetachingHead false &&
-	test_must_fail git switch main^{commit} 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git switch main^{commit} 2>stderr &&
 	! grep "try again with the --detach option" stderr
 '
 
diff --git a/t/t2204-add-ignored.sh b/t/t2204-add-ignored.sh
index b7cf1e492c1..ca46bbd22c7 100755
--- a/t/t2204-add-ignored.sh
+++ b/t/t2204-add-ignored.sh
@@ -30,7 +30,7 @@ for i in ign dir/ign dir/sub dir/sub/*ign sub/file sub sub/*
 do
 	test_expect_success "complaints for ignored $i" '
 		rm -f .git/index &&
-		test_must_fail git add "$i" 2>err &&
+		test_env GIT_ADVICE=1 test_must_fail git add "$i" 2>err &&
 		git ls-files "$i" >out &&
 		test_must_be_empty out
 	'
@@ -41,7 +41,7 @@ do
 
 	test_expect_success "complaints for ignored $i with unignored file" '
 		rm -f .git/index &&
-		test_must_fail git add "$i" file 2>err &&
+		test_env GIT_ADVICE=1 test_must_fail git add "$i" file 2>err &&
 		git ls-files "$i" >out &&
 		test_must_be_empty out
 	'
@@ -56,7 +56,7 @@ do
 		rm -f .git/index &&
 		(
 			cd dir &&
-			test_must_fail git add "$i" 2>err &&
+			test_env GIT_ADVICE=1 test_must_fail git add "$i" 2>err &&
 			git ls-files "$i" >out &&
 			test_must_be_empty out
 		)
@@ -76,7 +76,7 @@ do
 		rm -f .git/index &&
 		(
 			cd sub &&
-			test_must_fail git add "$i" 2>err &&
+			test_env GIT_ADVICE=1 test_must_fail git add "$i" 2>err &&
 			git ls-files "$i" >out &&
 			test_must_be_empty out
 		)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index cfc4aeb1798..742002ff41e 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -436,7 +436,7 @@ test_wt_add_orphan_hint () {
 		git init repo &&
 		(cd repo && test_commit commit) &&
 		git -C repo switch --orphan noref &&
-		test_must_fail git -C repo worktree add $opts foobar/ 2>actual &&
+		test_env GIT_ADVICE=1 test_must_fail git -C repo worktree add $opts foobar/ 2>actual &&
 		! grep "error: unknown switch" actual &&
 		grep "hint: If you meant to create a worktree containing a new unborn branch" actual &&
 		if [ $use_branch -eq 1 ]
@@ -983,7 +983,7 @@ test_dwim_orphan () {
 			fi &&
 			if [ "$outcome" = "infer" ]
 			then
-				git $dashc_args worktree add $args 2>actual &&
+				GIT_ADVICE=1 git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
 					test_must_be_empty actual
@@ -992,7 +992,7 @@ test_dwim_orphan () {
 				fi
 			elif [ "$outcome" = "no_infer" ]
 			then
-				git $dashc_args worktree add $args 2>actual &&
+				GIT_ADVICE=1 git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
 					test_must_be_empty actual
@@ -1001,11 +1001,11 @@ test_dwim_orphan () {
 				fi
 			elif [ "$outcome" = "fetch_error" ]
 			then
-				test_must_fail git $dashc_args worktree add $args 2>actual &&
+				test_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&
 				grep "$fetch_error_text" actual
 			elif [ "$outcome" = "fatal_orphan_bad_combo" ]
 			then
-				test_must_fail git $dashc_args worktree add $args 2>actual &&
+				test_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
 					! grep "$info_text" actual
@@ -1015,7 +1015,7 @@ test_dwim_orphan () {
 				grep "$bad_combo_regex" actual
 			elif [ "$outcome" = "warn_bad_head" ]
 			then
-				test_must_fail git $dashc_args worktree add $args 2>actual &&
+				test_env GIT_ADVICE=1 test_must_fail git $dashc_args worktree add $args 2>actual &&
 				if [ $use_quiet -eq 1 ]
 				then
 					grep "$invalid_ref_regex" actual &&
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 4dca8d97a77..546b6f2f373 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -554,7 +554,8 @@ test_expect_success 'commit without staging files fails and displays hints' '
 	git add file &&
 	git commit -m initial &&
 	echo "changes" >>file &&
-	test_must_fail git commit -m update >actual &&
+	test_env GIT_ADVICE=1 test_must_fail \
+		git commit -m update >actual &&
 	test_grep "no changes added to commit (use \"git add\" and/or \"git commit -a\")" actual
 '

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 2/7] t3000-4000: add GIT_ADVICE=1 to advice tests

From: Derrick Stolee <derrickstolee@github.com>

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t3200-branch.sh               |  4 ++--
 t/t3404-rebase-interactive.sh   |  2 +-
 t/t3501-revert-cherry-pick.sh   |  2 +-
 t/t3507-cherry-pick-conflict.sh |  4 ++--
 t/t3510-cherry-pick-sequence.sh |  6 +++---
 t/t3600-rm.sh                   | 12 ++++++------
 t/t3602-rm-sparse-checkout.sh   | 18 +++++++++---------
 t/t3700-add.sh                  |  6 +++---
 t/t3705-add-sparse-checkout.sh  | 32 ++++++++++++++++----------------
 t/t4150-am.sh                   | 14 +++++++-------
 10 files changed, 50 insertions(+), 50 deletions(-)

Show changes to 10 files +50 −50

t/t3200-branch.sh, t/t3404-rebase-interactive.sh, t/t3501-revert-cherry-pick.sh, t/t3507-cherry-pick-conflict.sh, t/t3510-cherry-pick-sequence.sh, t/t3600-rm.sh, t/t3602-rm-sparse-checkout.sh, t/t3700-add.sh, t/t3705-add-sparse-checkout.sh, t/t4150-am.sh

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ccfa6a720d0..9ff64fe4f1a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1161,7 +1161,7 @@ test_expect_success 'avoid ambiguous track and advise' '
 	hint: different remotes'\'' fetch refspecs map into different
 	hint: tracking namespaces.
 	EOF
-	test_must_fail git branch all1 main 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git branch all1 main 2>actual &&
 	test_cmp expected actual &&
 	test -z "$(git config branch.all1.merge)"
 '
@@ -1699,7 +1699,7 @@ test_expect_success 'errors if given a bad branch name' '
 	hint: See `man git check-ref-format`
 	hint: Disable this message with "git config advice.refSyntax false"
 	EOF
-	test_must_fail git branch foo..bar >actual 2>&1 &&
+	test_env GIT_ADVICE=1 test_must_fail git branch foo..bar >actual 2>&1 &&
 	test_cmp expect actual
 '
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f92baad1381..c31ca807f7b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2229,7 +2229,7 @@ test_expect_success 'non-merge commands reject merge commits' '
 	EOF
 	(
 		set_replace_editor todo &&
-		test_must_fail git rebase -i HEAD 2>actual
+		test_env GIT_ADVICE=1 test_must_fail git rebase -i HEAD 2>actual
 	) &&
 	cat >expect <<-EOF &&
 	error: ${SQ}pick${SQ} does not accept merge commits
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 411027fb58c..3478a8a588f 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -181,7 +181,7 @@ test_expect_success 'advice from failed revert' '
 	hint: Disable this message with "git config advice.mergeConflict false"
 	EOF
 	test_commit --append --no-tag "double-add dream" dream dream &&
-	test_must_fail git revert HEAD^ 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git revert HEAD^ 2>actual &&
 	test_cmp expected actual
 '
 
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index f3947b400a3..5633a10659d 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -62,7 +62,7 @@ test_expect_success 'advice from failed cherry-pick' '
 	hint: run "git cherry-pick --abort".
 	hint: Disable this message with "git config advice.mergeConflict false"
 	EOF
-	test_must_fail git cherry-pick picked 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
 '
@@ -77,7 +77,7 @@ test_expect_success 'advice from failed cherry-pick --no-commit' "
 	hint: with 'git add <paths>' or 'git rm <paths>'
 	hint: Disable this message with \"git config advice.mergeConflict false\"
 	EOF
-	test_must_fail git cherry-pick --no-commit picked 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git cherry-pick --no-commit picked 2>actual &&
 
 	test_cmp expected actual
 "
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7eb52b12edc..291c5de4f7d 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -231,7 +231,7 @@ test_expect_success 'check advice when we move HEAD by committing' '
 	echo c >foo &&
 	git commit -a &&
 	test_path_is_missing .git/CHERRY_PICK_HEAD &&
-	test_must_fail git cherry-pick --skip 2>advice &&
+	test_env GIT_ADVICE=1 test_must_fail git cherry-pick --skip 2>advice &&
 	test_cmp expect advice
 '
 
@@ -243,7 +243,7 @@ test_expect_success 'selectively advise --skip while launching another sequence'
 	fatal: cherry-pick failed
 	EOF
 	test_must_fail git cherry-pick picked..yetanotherpick &&
-	test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
+	test_env GIT_ADVICE=1 test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
 	test_cmp expect advice &&
 	cat >expect <<-EOF &&
 	error: cherry-pick is already in progress
@@ -251,7 +251,7 @@ test_expect_success 'selectively advise --skip while launching another sequence'
 	fatal: cherry-pick failed
 	EOF
 	git reset --merge &&
-	test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
+	test_env GIT_ADVICE=1 test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
 	test_cmp expect advice
 '
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 31ac31d4bcd..90a30a3a002 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -822,7 +822,7 @@ test_expect_success 'rm files with different staged content' '
 	EOF
 	echo content1 >foo.txt &&
 	echo content1 >bar.txt &&
-	test_must_fail git rm foo.txt bar.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git rm foo.txt bar.txt 2>actual &&
 	test_cmp expect actual
 '
 
@@ -847,7 +847,7 @@ test_expect_success 'rm file with local modification' '
 	EOF
 	git commit -m "testing rm 3" &&
 	echo content3 >foo.txt &&
-	test_must_fail git rm foo.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git rm foo.txt 2>actual &&
 	test_cmp expect actual
 '
 
@@ -857,7 +857,7 @@ test_expect_success 'rm file with local modification without hints' '
 	    bar.txt
 	EOF
 	echo content4 >bar.txt &&
-	test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&
 	test_cmp expect actual
 '
 
@@ -870,7 +870,7 @@ test_expect_success 'rm file with changes in the index' '
 	git reset --hard &&
 	echo content5 >foo.txt &&
 	git add foo.txt &&
-	test_must_fail git rm foo.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git rm foo.txt 2>actual &&
 	test_cmp expect actual
 '
 
@@ -879,7 +879,7 @@ test_expect_success 'rm file with changes in the index without hints' '
 	error: the following file has changes staged in the index:
 	    foo.txt
 	EOF
-	test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&
 	test_cmp expect actual
 '
 
@@ -898,7 +898,7 @@ test_expect_success 'rm files with two different errors' '
 	echo content6 >foo1.txt &&
 	echo content6 >bar1.txt &&
 	git add bar1.txt &&
-	test_must_fail git rm bar1.txt foo1.txt 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git rm bar1.txt foo1.txt 2>actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
index fcdefba48cc..c2b197046d4 100755
--- a/t/t3602-rm-sparse-checkout.sh
+++ b/t/t3602-rm-sparse-checkout.sh
@@ -32,7 +32,7 @@ for opt in "" -f --dry-run
 do
 	test_expect_success "rm${opt:+ $opt} does not remove sparse entries" '
 		git sparse-checkout set --no-cone a &&
-		test_must_fail git rm $opt b 2>stderr &&
+		test_env GIT_ADVICE=1 test_must_fail git rm $opt b 2>stderr &&
 		test_cmp b_error_and_hint stderr &&
 		git ls-files --error-unmatch b
 	'
@@ -72,14 +72,14 @@ test_expect_success 'recursive rm --sparse removes sparse entries' '
 test_expect_success 'rm obeys advice.updateSparsePath' '
 	git reset --hard &&
 	git sparse-checkout set a &&
-	test_must_fail git -c advice.updateSparsePath=false rm b 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git -c advice.updateSparsePath=false rm b 2>stderr &&
 	test_cmp sparse_entry_b_error stderr
 '
 
 test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '
 	git reset --hard &&
 	git sparse-checkout set a &&
-	test_must_fail git rm nonexistent 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git rm nonexistent 2>stderr &&
 	grep "fatal: pathspec .nonexistent. did not match any files" stderr &&
 	! grep -F -f sparse_error_header stderr
 '
@@ -87,7 +87,7 @@ test_expect_success 'do not advice about sparse entries when they do not match t
 test_expect_success 'do not warn about sparse entries when pathspec matches dense entries' '
 	git reset --hard &&
 	git sparse-checkout set a &&
-	git rm "[ba]" 2>stderr &&
+	GIT_ADVICE=1 git rm "[ba]" 2>stderr &&
 	test_must_be_empty stderr &&
 	git ls-files --error-unmatch b &&
 	test_must_fail git ls-files --error-unmatch a
@@ -96,7 +96,7 @@ test_expect_success 'do not warn about sparse entries when pathspec matches dens
 test_expect_success 'do not warn about sparse entries with --ignore-unmatch' '
 	git reset --hard &&
 	git sparse-checkout set a &&
-	git rm --ignore-unmatch b 2>stderr &&
+	GIT_ADVICE=1 git rm --ignore-unmatch b 2>stderr &&
 	test_must_be_empty stderr &&
 	git ls-files --error-unmatch b
 '
@@ -105,9 +105,9 @@ test_expect_success 'refuse to rm a non-skip-worktree path outside sparse cone'
 	git reset --hard &&
 	git sparse-checkout set a &&
 	git update-index --no-skip-worktree b &&
-	test_must_fail git rm b 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git rm b 2>stderr &&
 	test_cmp b_error_and_hint stderr &&
-	git rm --sparse b 2>stderr &&
+	GIT_ADVICE=1 git rm --sparse b 2>stderr &&
 	test_must_be_empty stderr &&
 	test_path_is_missing b
 '
@@ -120,7 +120,7 @@ test_expect_success 'can remove files from non-sparse dir' '
 	test_commit x/y/f &&
 
 	git sparse-checkout set --no-cone w !/x y/ &&
-	git rm w/f.t x/y/f.t 2>stderr &&
+	GIT_ADVICE=1 git rm w/f.t x/y/f.t 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -132,7 +132,7 @@ test_expect_success 'refuse to remove non-skip-worktree file from sparse dir' '
 	git sparse-checkout set --no-cone !/x y/ !x/y/z &&
 
 	git update-index --no-skip-worktree x/y/z/f.t &&
-	test_must_fail git rm x/y/z/f.t 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git rm x/y/z/f.t 2>stderr &&
 	echo x/y/z/f.t | cat sparse_error_header - sparse_hint >expect &&
 	test_cmp expect stderr
 '
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 839c904745a..8042c3bc34a 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -34,7 +34,7 @@ test_expect_success 'Test with no pathspecs' '
 	hint: Maybe you wanted to say ${SQ}git add .${SQ}?
 	hint: Disable this message with "git config advice.addEmptyPathspec false"
 	EOF
-	git add 2>actual &&
+	GIT_ADVICE=1 git add 2>actual &&
 	test_cmp expect actual
 '
 
@@ -360,7 +360,7 @@ test_expect_success '"git add" a embedded repository' '
 			git -C $name commit --allow-empty -m $name ||
 				return 1
 		done &&
-		git add . 2>actual &&
+		GIT_ADVICE=1 git add . 2>actual &&
 		cat >expect <<-EOF &&
 		warning: adding embedded git repository: inner1
 		hint: You${SQ}ve added another git repository inside your current repository.
@@ -421,7 +421,7 @@ add 'track-this'
 EOF
 
 test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
-	test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual.out 2>actual.err
+	test_env GIT_ADVICE=1 test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual.out 2>actual.err
 '
 
 test_expect_success 'git add --dry-run --ignore-missing of non-existing file output' '
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
index 2bade9e804f..c06e803c0e9 100755
--- a/t/t3705-add-sparse-checkout.sh
+++ b/t/t3705-add-sparse-checkout.sh
@@ -64,7 +64,7 @@ test_expect_success 'setup' "
 test_expect_success 'git add does not remove sparse entries' '
 	setup_sparse_entry &&
 	rm sparse_entry &&
-	test_must_fail git add sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp error_and_hint stderr &&
 	test_sparse_entry_unchanged
@@ -74,7 +74,7 @@ test_expect_success 'git add -A does not remove sparse entries' '
 	setup_sparse_entry &&
 	rm sparse_entry &&
 	setup_gitignore &&
-	git add -A 2>stderr &&
+	GIT_ADVICE=1 git add -A 2>stderr &&
 	test_must_be_empty stderr &&
 	test_sparse_entry_unchanged
 '
@@ -83,7 +83,7 @@ test_expect_success 'git add . does not remove sparse entries' '
 	setup_sparse_entry &&
 	rm sparse_entry &&
 	setup_gitignore &&
-	test_must_fail git add . 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add . 2>stderr &&
 	test_sparse_entry_unstaged &&
 
 	cat sparse_error_header >expect &&
@@ -99,7 +99,7 @@ do
 	test_expect_success "git add${opt:+ $opt} does not update sparse entries" '
 		setup_sparse_entry &&
 		echo modified >sparse_entry &&
-		test_must_fail git add $opt sparse_entry 2>stderr &&
+		test_env GIT_ADVICE=1 test_must_fail git add $opt sparse_entry 2>stderr &&
 		test_sparse_entry_unstaged &&
 		test_cmp error_and_hint stderr &&
 		test_sparse_entry_unchanged
@@ -110,7 +110,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
 	setup_sparse_entry &&
 	git ls-files --debug sparse_entry | grep mtime >before &&
 	test-tool chmtime -60 sparse_entry &&
-	test_must_fail git add --refresh sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add --refresh sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp error_and_hint stderr &&
 	git ls-files --debug sparse_entry | grep mtime >after &&
@@ -119,7 +119,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
 
 test_expect_success 'git add --chmod does not update sparse entries' '
 	setup_sparse_entry &&
-	test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp error_and_hint stderr &&
 	test_sparse_entry_unchanged &&
@@ -131,7 +131,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' '
 	test_config core.autocrlf false &&
 	setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
 	echo "sparse_entry text=auto" >.gitattributes &&
-	test_must_fail git add --renormalize sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add --renormalize sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp error_and_hint stderr &&
 	test_sparse_entry_unchanged
@@ -140,7 +140,7 @@ test_expect_success 'git add --renormalize does not update sparse entries' '
 test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '
 	setup_sparse_entry &&
 	rm sparse_entry &&
-	test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add --dry-run --ignore-missing sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp error_and_hint stderr &&
 	test_sparse_entry_unchanged
@@ -148,7 +148,7 @@ test_expect_success 'git add --dry-run --ignore-missing warn on sparse path' '
 
 test_expect_success 'do not advice about sparse entries when they do not match the pathspec' '
 	setup_sparse_entry &&
-	test_must_fail git add nonexistent 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add nonexistent 2>stderr &&
 	grep "fatal: pathspec .nonexistent. did not match any files" stderr &&
 	! grep -F -f sparse_error_header stderr
 '
@@ -157,7 +157,7 @@ test_expect_success 'do not warn when pathspec matches dense entries' '
 	setup_sparse_entry &&
 	echo modified >sparse_entry &&
 	>dense_entry &&
-	git add "*_entry" 2>stderr &&
+	GIT_ADVICE=1 git add "*_entry" 2>stderr &&
 	test_must_be_empty stderr &&
 	test_sparse_entry_unchanged &&
 	git ls-files --error-unmatch dense_entry
@@ -181,12 +181,12 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
 	test_sparse_entry_unstaged &&
 
 	# Avoid munging CRLFs to avoid an error message
-	git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
+	GIT_ADVICE=1 git -c core.autocrlf=input add --sparse sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
 	git ls-files --stage >actual &&
 	grep "^100644 .*sparse_entry\$" actual &&
 
-	git add --sparse --chmod=+x sparse_entry 2>stderr &&
+	GIT_ADVICE=1 git add --sparse --chmod=+x sparse_entry 2>stderr &&
 	test_must_be_empty stderr &&
 	git ls-files --stage >actual &&
 	grep "^100755 .*sparse_entry\$" actual &&
@@ -201,7 +201,7 @@ test_expect_success 'git add fails outside of sparse-checkout definition' '
 
 test_expect_success 'add obeys advice.updateSparsePath' '
 	setup_sparse_entry &&
-	test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git -c advice.updateSparsePath=false add sparse_entry 2>stderr &&
 	test_sparse_entry_unstaged &&
 	test_cmp sparse_entry_error stderr
 
@@ -212,7 +212,7 @@ test_expect_success 'add allows sparse entries with --sparse' '
 	echo modified >sparse_entry &&
 	test_must_fail git add sparse_entry &&
 	test_sparse_entry_unchanged &&
-	git add --sparse sparse_entry 2>stderr &&
+	GIT_ADVICE=1 git add --sparse sparse_entry 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -220,7 +220,7 @@ test_expect_success 'can add files from non-sparse dir' '
 	git sparse-checkout set w !/x y/ &&
 	mkdir -p w x/y &&
 	touch w/f x/y/f &&
-	git add w/f x/y/f 2>stderr &&
+	GIT_ADVICE=1 git add w/f x/y/f 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -228,7 +228,7 @@ test_expect_success 'refuse to add non-skip-worktree file from sparse dir' '
 	git sparse-checkout set !/x y/ !x/y/z &&
 	mkdir -p x/y/z &&
 	touch x/y/z/f &&
-	test_must_fail git add x/y/z/f 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git add x/y/z/f 2>stderr &&
 	echo x/y/z/f | cat sparse_error_header - sparse_hint >expect &&
 	test_cmp expect stderr
 '
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 5e2b6c80eae..68a62ff330e 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -678,7 +678,7 @@ test_expect_success 'am -3 -q is quiet' '
 	rm -fr .git/rebase-apply &&
 	git checkout -f lorem2 &&
 	git reset base3way --hard &&
-	git am -3 -q lorem-move.patch >output.out 2>&1 &&
+	GIT_ADVICE=1 git am -3 -q lorem-move.patch >output.out 2>&1 &&
 	test_must_be_empty output.out
 '
 
@@ -921,7 +921,7 @@ test_expect_success 'am -q is quiet' '
 	git reset --hard &&
 	git checkout first &&
 	test_tick &&
-	git am -q <patch1 >output.out 2>&1 &&
+	GIT_ADVICE=1 git am -q <patch1 >output.out 2>&1 &&
 	test_must_be_empty output.out
 '
 
@@ -930,7 +930,7 @@ test_expect_success 'am empty-file does not infloop' '
 	git reset --hard &&
 	touch empty-file &&
 	test_tick &&
-	test_must_fail git am empty-file 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git am empty-file 2>actual &&
 	echo Patch format detection failed. >expected &&
 	test_cmp expected actual
 '
@@ -1180,7 +1180,7 @@ test_expect_success 'apply binary blob in partial clone' '
 
 test_expect_success 'an empty input file is error regardless of --empty option' '
 	test_when_finished "git am --abort || :" &&
-	test_must_fail git am --empty=drop empty.patch 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git am --empty=drop empty.patch 2>actual &&
 	echo "Patch format detection failed." >expected &&
 	test_cmp expected actual
 '
@@ -1188,7 +1188,7 @@ test_expect_success 'an empty input file is error regardless of --empty option'
 test_expect_success 'invalid when passing the --empty option alone' '
 	test_when_finished "git am --abort || :" &&
 	git checkout empty-commit^ &&
-	test_must_fail git am --empty empty-commit.patch 2>err &&
+	test_env GIT_ADVICE=1 test_must_fail git am --empty empty-commit.patch 2>err &&
 	echo "error: invalid value for '\''--empty'\'': '\''empty-commit.patch'\''" >expected &&
 	test_cmp expected err
 '
@@ -1224,7 +1224,7 @@ test_expect_success 'record as an empty commit when meeting e-mail message that
 
 test_expect_success 'skip an empty patch in the middle of an am session' '
 	git checkout empty-commit^ &&
-	test_must_fail git am empty-commit.patch >out 2>err &&
+	test_env GIT_ADVICE=1 test_must_fail git am empty-commit.patch >out 2>err &&
 	grep "Patch is empty." out &&
 	grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err &&
 	git am --skip &&
@@ -1236,7 +1236,7 @@ test_expect_success 'skip an empty patch in the middle of an am session' '
 
 test_expect_success 'record an empty patch as an empty commit in the middle of an am session' '
 	git checkout empty-commit^ &&
-	test_must_fail git am empty-commit.patch >out 2>err &&
+	test_env GIT_ADVICE=1 test_must_fail git am empty-commit.patch >out 2>err &&
 	grep "Patch is empty." out &&
 	grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err &&
 	git am --allow-empty >output &&

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 3/7] t5000: add GIT_ADVICE=1 to advice tests

From: Derrick Stolee <derrickstolee@github.com>

In particular, lib-https.sh must be updated in order for t5541 to succeed as it calls test_http_push_nonff.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/lib-httpd.sh             | 2 +-
 t/t5505-remote.sh          | 5 +++--
 t/t5520-pull.sh            | 4 ++--
 t/t5541-http-push-smart.sh | 6 ++++--
 4 files changed, 10 insertions(+), 7 deletions(-)

Show changes to 4 files +10 −7

t/lib-httpd.sh, t/t5505-remote.sh, t/t5520-pull.sh, t/t5541-http-push-smart.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index d83bafeab32..b85ce907f05 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -265,7 +265,7 @@ test_http_push_nonff () {
 		echo "changed" > path2 &&
 		git commit -a -m path2 --amend &&
 
-		test_must_fail git push -v origin >output 2>&1 &&
+		test_env GIT_ADVICE=1 test_must_fail git push -v origin >output 2>&1 &&
 		(
 			cd "$REMOTE_REPO" &&
 			echo "$HEAD" >expect &&
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 08424e878e1..3e5215add31 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1452,10 +1452,11 @@ test_expect_success 'unqualified <dst> refspec DWIM and advice' '
 			else
 				oid=$(git rev-parse some-tag^{$type})
 			fi &&
-			test_must_fail git push origin $oid:dst 2>err &&
+			test_env GIT_ADVICE=1 test_must_fail git push origin $oid:dst 2>err &&
 			test_grep "error: The destination you" err &&
 			test_grep "hint: Did you mean" err &&
-			test_must_fail git -c advice.pushUnqualifiedRefName=false \
+			test_env GIT_ADVICE=1 test_must_fail git \
+				-c advice.pushUnqualifiedRefName=false \
 				push origin $oid:dst 2>err &&
 			test_grep "error: The destination you" err &&
 			test_grep ! "hint: Did you mean" err ||
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1098cbd0a19..c4a309ce4ae 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -375,7 +375,7 @@ test_expect_success '--rebase with conflicts shows advice' '
 	echo conflicting >>seq.txt &&
 	test_tick &&
 	git commit -m "Create conflict" seq.txt &&
-	test_must_fail git pull --rebase . seq 2>err >out &&
+	test_env GIT_ADVICE=1 test_must_fail git pull --rebase . seq 2>err >out &&
 	test_grep "Resolve all conflicts manually" err
 '
 
@@ -389,7 +389,7 @@ test_expect_success 'failed --rebase shows advice' '
 	# force checkout because `git reset --hard` will not leave clean `file`
 	git checkout -f -b fails-to-rebase HEAD^ &&
 	test_commit v2-without-cr file "2" file2-lf &&
-	test_must_fail git pull --rebase . diverging 2>err >out &&
+	test_env GIT_ADVICE=1 test_must_fail git pull --rebase . diverging 2>err >out &&
 	test_grep "Resolve all conflicts manually" err
 '
 
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 71428f3d5c7..dfd4c21808f 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -145,7 +145,7 @@ test_expect_success 'push fails for non-fast-forward refs unmatched by remote he
 
 	# push main too; this ensures there is at least one '"'push'"' command to
 	# the remote helper and triggers interaction with the helper.
-	test_must_fail git push -v origin +main main:niam >output 2>&1'
+	test_env GIT_ADVICE=1 test_must_fail git push -v origin +main main:niam >output 2>&1'
 
 test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper: remote output' '
 	grep "^ + [a-f0-9]*\.\.\.[a-f0-9]* *main -> main (forced update)$" output &&
@@ -477,7 +477,9 @@ test_expect_success 'Non-ASCII branch name can be used with --force-with-lease'
 
 test_expect_success 'colorize errors/hints' '
 	cd "$ROOT_PATH"/test_repo_clone &&
-	test_must_fail git -c color.transport=always -c color.advice=always \
+	test_env GIT_ADVICE=1 test_must_fail git \
+		-c color.transport=always \
+		-c color.advice=always \
 		-c color.push=always \
 		push origin origin/main^:main 2>act &&
 	test_decode_color <act >decoded &&

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 4/7] t6000: add GIT_ADVICE=1 to advice tests

From: Derrick Stolee <derrickstolee@github.com>

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t6001-rev-list-graft.sh      |  4 ++--
 t/t6050-replace.sh             |  6 +++---
 t/t6436-merge-overwrite.sh     |  6 +++---
 t/t6437-submodule-merge.sh     | 16 ++++++++--------
 t/t6439-merge-co-error-msgs.sh | 12 ++++++------
 5 files changed, 22 insertions(+), 22 deletions(-)

Show changes to 5 files +22 −22

t/t6001-rev-list-graft.sh, t/t6050-replace.sh, t/t6436-merge-overwrite.sh, t/t6437-submodule-merge.sh, t/t6439-merge-co-error-msgs.sh

diff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh
index 3553bbbfe73..e3f19621727 100755
--- a/t/t6001-rev-list-graft.sh
+++ b/t/t6001-rev-list-graft.sh
@@ -118,10 +118,10 @@ do
 done
 
 test_expect_success 'show advice that grafts are deprecated' '
-	git show HEAD 2>err &&
+	GIT_ADVICE=1 git show HEAD 2>err &&
 	test_grep "git replace" err &&
 	test_config advice.graftFileDeprecated false &&
-	git show HEAD 2>err &&
+	GIT_ADVICE=1 git show HEAD 2>err &&
 	test_grep ! "git replace" err
 '
 
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c6e9b33e44e..fc48cb4b0ad 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -489,9 +489,9 @@ test_expect_success '--convert-graft-file' '
 	printf "%s\n%s %s\n\n# comment\n%s\n" \
 		$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
 		>.git/info/grafts &&
-	git status 2>stderr &&
+	GIT_ADVICE=1 git status 2>stderr &&
 	test_grep "hint:.*grafts is deprecated" stderr &&
-	git replace --convert-graft-file 2>stderr &&
+	GIT_ADVICE=1 git replace --convert-graft-file 2>stderr &&
 	test_grep ! "hint:.*grafts is deprecated" stderr &&
 	test_path_is_missing .git/info/grafts &&
 
@@ -502,7 +502,7 @@ test_expect_success '--convert-graft-file' '
 	: create invalid graft file and verify that it is not deleted &&
 	test_when_finished "rm -f .git/info/grafts" &&
 	echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
-	test_must_fail git replace --convert-graft-file 2>err &&
+	test_env GIT_ADVICE=1 test_must_fail git replace --convert-graft-file 2>err &&
 	test_grep "$EMPTY_BLOB $EMPTY_TREE" err &&
 	test_grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
 '
diff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh
index ccc620477d4..7c9f5b623f1 100755
--- a/t/t6436-merge-overwrite.sh
+++ b/t/t6436-merge-overwrite.sh
@@ -104,7 +104,7 @@ test_expect_success 'will not overwrite unstaged changes in renamed file' '
 	cp important other.c &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	then
-		test_must_fail git merge c1a >out 2>err &&
+		test_env GIT_ADVICE=1 test_must_fail git merge c1a >out 2>err &&
 		test_grep "would be overwritten by merge" err &&
 		test_cmp important other.c &&
 		test_path_is_missing .git/MERGE_HEAD
@@ -140,7 +140,7 @@ test_expect_success 'will not overwrite untracked file in leading path' '
 	rm -rf sub &&
 	cp important sub &&
 	cp important sub2 &&
-	test_must_fail git merge sub 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git merge sub 2>out &&
 	test_cmp out expect &&
 	test_path_is_missing .git/MERGE_HEAD &&
 	test_cmp important sub &&
@@ -175,7 +175,7 @@ test_expect_success 'will not overwrite untracked file on unborn branch' '
 	git rm -fr . &&
 	git checkout --orphan new &&
 	cp important c0.c &&
-	test_must_fail git merge c0 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git merge c0 2>out &&
 	test_cmp out expect
 '
 
diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh
index 7a3f1cb27c1..9265cebca75 100755
--- a/t/t6437-submodule-merge.sh
+++ b/t/t6437-submodule-merge.sh
@@ -113,11 +113,11 @@ test_expect_success 'merging should conflict for non fast-forward' '
 	 git checkout -b test-nonforward-a b &&
 	  if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	  then
-		test_must_fail git merge c 2>actual &&
+		test_env GIT_ADVICE=1 test_must_fail git merge c 2>actual &&
 		sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
 		grep "$sub_expect" actual
 	  else
-		test_must_fail git merge c 2> actual
+		test_env GIT_ADVICE=1 test_must_fail git merge c 2> actual
 	  fi)
 '
 
@@ -154,11 +154,11 @@ test_expect_success 'merging should conflict for non fast-forward (resolution ex
 	  git rev-parse --short sub-d > ../expect) &&
 	  if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	  then
-		test_must_fail git merge c >actual 2>sub-actual &&
+		test_env GIT_ADVICE=1 test_must_fail git merge c >actual 2>sub-actual &&
 		sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
 		grep "$sub_expect" sub-actual
 	  else
-		test_must_fail git merge c 2> actual
+		test_env GIT_ADVICE=1 test_must_fail git merge c 2> actual
 	  fi &&
 	 grep $(cat expect) actual > /dev/null &&
 	 git reset --hard)
@@ -181,11 +181,11 @@ test_expect_success 'merging should fail for ambiguous common parent' '
 	 ) &&
 	 if test "$GIT_TEST_MERGE_ALGORITHM" = ort
 	 then
-		test_must_fail git merge c >actual 2>sub-actual &&
+		test_env GIT_ADVICE=1 test_must_fail git merge c >actual 2>sub-actual &&
 		sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-c)" &&
 		grep "$sub_expect" sub-actual
 	 else
-		test_must_fail git merge c 2> actual
+		test_env GIT_ADVICE=1 test_must_fail git merge c 2> actual
 	 fi &&
 	grep $(cat expect1) actual > /dev/null &&
 	grep $(cat expect2) actual > /dev/null &&
@@ -227,7 +227,7 @@ test_expect_success 'merging should fail for changes that are backwards' '
 	git commit -a -m "f" &&
 
 	git checkout -b test-backward e &&
-	test_must_fail git merge f 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git merge f 2>actual &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     then
 		sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short sub-d)" &&
@@ -535,7 +535,7 @@ test_expect_success 'merging should fail with no merge base' '
 	git checkout -b b init &&
 	git add sub &&
 	git commit -m "b" &&
-	test_must_fail git merge a 2>actual &&
+	test_env GIT_ADVICE=1 test_must_fail git merge a 2>actual &&
 	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     then
 		sub_expect="go to submodule (sub), and either merge commit $(git -C sub rev-parse --short HEAD^1)" &&
diff --git a/t/t6439-merge-co-error-msgs.sh b/t/t6439-merge-co-error-msgs.sh
index 0cbec57cdab..dcc7d45ac75 100755
--- a/t/t6439-merge-co-error-msgs.sh
+++ b/t/t6439-merge-co-error-msgs.sh
@@ -40,13 +40,13 @@ Aborting
 EOF
 
 test_expect_success 'untracked files overwritten by merge (fast and non-fast forward)' '
-	test_must_fail git merge branch 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git merge branch 2>out &&
 	test_cmp out expect &&
 	git commit --allow-empty -m empty &&
 	(
 		GIT_MERGE_VERBOSITY=0 &&
 		export GIT_MERGE_VERBOSITY &&
-		test_must_fail git merge branch 2>out2
+		test_env GIT_ADVICE=1 test_must_fail git merge branch 2>out2
 	) &&
 	echo "Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed." >>expect &&
 	test_cmp out2 expect &&
@@ -69,7 +69,7 @@ test_expect_success 'untracked files or local changes ovewritten by merge' '
 	git add two &&
 	git add three &&
 	git add four &&
-	test_must_fail git merge branch 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git merge branch 2>out &&
 	test_cmp out expect
 '
 
@@ -91,7 +91,7 @@ test_expect_success 'cannot switch branches because of local changes' '
 	git checkout main &&
 	echo uno >rep/one &&
 	echo dos >rep/two &&
-	test_must_fail git checkout branch 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&
 	test_cmp out expect
 '
 
@@ -105,7 +105,7 @@ EOF
 
 test_expect_success 'not uptodate file porcelain checkout error' '
 	git add rep/one rep/two &&
-	test_must_fail git checkout branch 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&
 	test_cmp out expect
 '
 
@@ -136,7 +136,7 @@ test_expect_success 'not_uptodate_dir porcelain checkout error' '
 	git checkout main &&
 	>rep/untracked-file &&
 	>rep2/untracked-file &&
-	test_must_fail git checkout branch 2>out &&
+	test_env GIT_ADVICE=1 test_must_fail git checkout branch 2>out &&
 	test_cmp out ../expect
 '

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 5/7] t7000: add GIT_ADVICE=1 to advice tests

From: Derrick Stolee <derrickstolee@github.com>

In addition, two more edits were made while in the neighborhood:

 1. In t7002, a redirected stderr was ignored and is now checked as empty.

 2. In t7060 and 7500, the output of "git status" has paranthetical messages
    that appear only when advice is enabled, even though it is sent to stdout.

 3. In t7400, a command was checked for failure with "!" but is now checked
    via test_must_fail.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t7002-mv-sparse-checkout.sh   | 85 +++++++++++++++++----------------
 t/t7004-tag.sh                  |  2 +-
 t/t7060-wtstatus.sh             | 11 +++--
 t/t7201-co.sh                   |  2 +-
 t/t7400-submodule-basic.sh      |  2 +-
 t/t7402-submodule-rebase.sh     |  3 +-
 t/t7406-submodule-update.sh     |  2 +-
 t/t7512-status-help.sh          |  4 +-
 t/t7520-ignored-hook-warning.sh |  8 ++--
 9 files changed, 61 insertions(+), 58 deletions(-)

Show changes to 9 files +61 −58

t/t7002-mv-sparse-checkout.sh, t/t7004-tag.sh, t/t7060-wtstatus.sh, t/t7201-co.sh, t/t7400-submodule-basic.sh, t/t7402-submodule-rebase.sh, t/t7406-submodule-update.sh, t/t7512-status-help.sh, t/t7520-ignored-hook-warning.sh

diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
index 57969ce805a..3b194bfa2f7 100755
--- a/t/t7002-mv-sparse-checkout.sh
+++ b/t/t7002-mv-sparse-checkout.sh
@@ -55,13 +55,13 @@ test_expect_success 'mv refuses to move sparse-to-sparse' '
 	git reset --hard &&
 	git sparse-checkout set --no-cone a &&
 	touch b &&
-	test_must_fail git mv b e 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv b e 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo b >>expect &&
 	echo e >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse b e 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse b e 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -72,7 +72,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
 
 	# tracked-to-untracked
 	touch b &&
-	git mv -k b e 2>stderr &&
+	GIT_ADVICE=1 git mv -k b e 2>stderr &&
 	test_path_exists b &&
 	test_path_is_missing e &&
 	cat sparse_error_header >expect &&
@@ -81,7 +81,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse b e 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse b e 2>stderr &&
 	test_must_be_empty stderr &&
 	test_path_is_missing b &&
 	test_path_exists e &&
@@ -89,7 +89,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
 	# tracked-to-tracked
 	git reset --hard &&
 	touch b &&
-	git mv -k b c 2>stderr &&
+	GIT_ADVICE=1 git mv -k b c 2>stderr &&
 	test_path_exists b &&
 	test_path_is_missing c &&
 	cat sparse_error_header >expect &&
@@ -98,7 +98,7 @@ test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse b c 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse b c 2>stderr &&
 	test_must_be_empty stderr &&
 	test_path_is_missing b &&
 	test_path_exists c
@@ -110,14 +110,14 @@ test_expect_success 'mv refuses to move non-sparse-to-sparse' '
 	git sparse-checkout set a &&
 
 	# tracked-to-untracked
-	test_must_fail git mv a e 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv a e 2>stderr &&
 	test_path_exists a &&
 	test_path_is_missing e &&
 	cat sparse_error_header >expect &&
 	echo e >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse a e 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse a e 2>stderr &&
 	test_must_be_empty stderr &&
 	test_path_is_missing a &&
 	test_path_exists e &&
@@ -125,14 +125,14 @@ test_expect_success 'mv refuses to move non-sparse-to-sparse' '
 	# tracked-to-tracked
 	rm e &&
 	git reset --hard &&
-	test_must_fail git mv a c 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv a c 2>stderr &&
 	test_path_exists a &&
 	test_path_is_missing c &&
 	cat sparse_error_header >expect &&
 	echo c >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse a c 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse a c 2>stderr &&
 	test_must_be_empty stderr &&
 	test_path_is_missing a &&
 	test_path_exists c
@@ -145,12 +145,12 @@ test_expect_success 'mv refuses to move sparse-to-non-sparse' '
 
 	# tracked-to-untracked
 	touch b &&
-	test_must_fail git mv b e 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv b e 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo b >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse b e 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse b e 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -164,7 +164,7 @@ test_expect_success 'recursive mv refuses to move (possible) sparse' '
 	mkdir sub/dir2 &&
 	touch sub/d sub/dir2/e &&
 
-	test_must_fail git mv sub sub2 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub sub2 2>stderr &&
 	cat sparse_error_header >expect &&
 	cat >>expect <<-\EOF &&
 	sub/d
@@ -174,7 +174,7 @@ test_expect_success 'recursive mv refuses to move (possible) sparse' '
 	EOF
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse sub sub2 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub sub2 2>stderr &&
 	test_must_be_empty stderr &&
 	git commit -m "moved sub to sub2" &&
 	git rev-parse HEAD~1:sub >expect &&
@@ -193,7 +193,7 @@ test_expect_success 'recursive mv refuses to move sparse' '
 	mkdir sub/dir2 &&
 	touch sub/dir2/e &&
 
-	test_must_fail git mv sub sub2 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub sub2 2>stderr &&
 	cat sparse_error_header >expect &&
 	cat >>expect <<-\EOF &&
 	sub/dir2/e
@@ -201,7 +201,7 @@ test_expect_success 'recursive mv refuses to move sparse' '
 	EOF
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
-	git mv --sparse sub sub2 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub sub2 2>stderr &&
 	test_must_be_empty stderr &&
 	git commit -m "moved sub to sub2" &&
 	git rev-parse HEAD~1:sub >expect &&
@@ -216,8 +216,9 @@ test_expect_success 'can move files to non-sparse dir' '
 	git sparse-checkout set a b c w !/x y/ &&
 	mkdir -p w x/y &&
 
-	git mv a w/new-a 2>stderr &&
-	git mv b x/y/new-b 2>stderr &&
+	GIT_ADVICE=1 git mv a w/new-a 2>stderr &&
+	test_must_be_empty stderr &&
+	GIT_ADVICE=1 git mv b x/y/new-b 2>stderr &&
 	test_must_be_empty stderr
 '
 
@@ -228,7 +229,7 @@ test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
 	git sparse-checkout set a !/x y/ !x/y/z &&
 	mkdir -p x/y/z &&
 
-	test_must_fail git mv a x/y/z/new-a 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv a x/y/z/new-a 2>stderr &&
 	echo x/y/z/new-a | cat sparse_error_header - sparse_hint >expect &&
 	test_cmp expect stderr
 '
@@ -237,7 +238,7 @@ test_expect_success 'refuse to move out-of-cone directory without --sparse' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
-	test_must_fail git mv folder1 sub 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv folder1 sub 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo folder1/file1 >>expect &&
 	cat sparse_hint >>expect &&
@@ -248,7 +249,7 @@ test_expect_success 'can move out-of-cone directory with --sparse' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
-	git mv --sparse folder1 sub 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse folder1 sub 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_dir sub/folder1 &&
@@ -259,7 +260,7 @@ test_expect_success 'refuse to move out-of-cone file without --sparse' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
-	test_must_fail git mv folder1/file1 sub 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv folder1/file1 sub 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo folder1/file1 >>expect &&
 	cat sparse_hint >>expect &&
@@ -270,7 +271,7 @@ test_expect_success 'can move out-of-cone file with --sparse' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
-	git mv --sparse folder1/file1 sub 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse folder1/file1 sub 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_file sub/file1
@@ -284,7 +285,7 @@ test_expect_success 'refuse to move sparse file to existing destination' '
 	git add folder1 sub/file1 &&
 	git sparse-checkout set --cone sub &&
 
-	test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv --sparse folder1/file1 sub 2>stderr &&
 	echo "fatal: destination exists, source=folder1/file1, destination=sub/file1" >expect &&
 	test_cmp expect stderr
 '
@@ -298,7 +299,7 @@ test_expect_success 'move sparse file to existing destination with --force and -
 	git add folder1 sub/file1 &&
 	git sparse-checkout set --cone sub &&
 
-	git mv --sparse --force folder1/file1 sub 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse --force folder1/file1 sub 2>stderr &&
 	test_must_be_empty stderr &&
 	echo "overwrite" >expect &&
 	test_cmp expect sub/file1
@@ -308,13 +309,13 @@ test_expect_success 'move clean path from in-cone to out-of-cone' '
 	test_when_finished "cleanup_sparse_checkout" &&
 	setup_sparse_checkout &&
 
-	test_must_fail git mv sub/d folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/d folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/d" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse sub/d folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub/d folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_missing sub/d &&
@@ -330,18 +331,18 @@ test_expect_success 'move clean path from in-cone to out-of-cone overwrite' '
 	echo "sub/file1 overwrite" >sub/file1 &&
 	git add sub/file1 &&
 
-	test_must_fail git mv sub/file1 folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/file1 folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/file1" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/file1 folder1 2>stderr &&
 	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
 	>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse -f sub/file1 folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse -f sub/file1 folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_missing sub/file1 &&
@@ -366,18 +367,18 @@ test_expect_success 'move clean path from in-cone to out-of-cone file overwrite'
 	echo "sub/file1 overwrite" >sub/file1 &&
 	git add sub/file1 &&
 
-	test_must_fail git mv sub/file1 folder1/file1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/file1 folder1/file1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/file1" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/file1 folder1/file1 2>stderr &&
 	echo "fatal: destination exists in the index, source=sub/file1, destination=folder1/file1" \
 	>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse -f sub/file1 folder1/file1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse -f sub/file1 folder1/file1 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_missing sub/file1 &&
@@ -403,19 +404,19 @@ test_expect_success 'move directory with one of the files overwrite' '
 	echo test >sub/dir/file1 &&
 	git add sub/dir/file1 &&
 
-	test_must_fail git mv sub/dir folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/dir/e" >>expect &&
 	echo "folder1/dir/file1" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	test_must_fail git mv --sparse sub/dir folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv --sparse sub/dir folder1 2>stderr &&
 	echo "fatal: destination exists in the index, source=sub/dir/file1, destination=folder1/dir/file1" \
 	>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse -f sub/dir folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse -f sub/dir folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_missing sub/dir/file1 &&
@@ -438,13 +439,13 @@ test_expect_success 'move dirty path from in-cone to out-of-cone' '
 	setup_sparse_checkout &&
 	echo "modified" >>sub/d &&
 
-	test_must_fail git mv sub/d folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/d folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/d" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse sub/d folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub/d folder1 2>stderr &&
 	cat dirty_error_header >expect &&
 	echo "folder1/d" >>expect &&
 	cat dirty_hint >>expect &&
@@ -462,13 +463,13 @@ test_expect_success 'move dir from in-cone to out-of-cone' '
 	setup_sparse_checkout &&
 	mkdir sub/dir/deep &&
 
-	test_must_fail git mv sub/dir folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/dir/e" >>expect &&
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse sub/dir folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub/dir folder1 2>stderr &&
 	test_must_be_empty stderr &&
 
 	test_path_is_missing sub/dir &&
@@ -487,7 +488,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	echo "modified" >>sub/dir/e2 &&
 	echo "modified" >>sub/dir/e3 &&
 
-	test_must_fail git mv sub/dir folder1 2>stderr &&
+	test_env GIT_ADVICE=1 test_must_fail git mv sub/dir folder1 2>stderr &&
 	cat sparse_error_header >expect &&
 	echo "folder1/dir/e" >>expect &&
 	echo "folder1/dir/e2" >>expect &&
@@ -495,7 +496,7 @@ test_expect_success 'move partially-dirty dir from in-cone to out-of-cone' '
 	cat sparse_hint >>expect &&
 	test_cmp expect stderr &&
 
-	git mv --sparse sub/dir folder1 2>stderr &&
+	GIT_ADVICE=1 git mv --sparse sub/dir folder1 2>stderr &&
 	cat dirty_error_header >expect &&
 	echo "folder1/dir/e2" >>expect &&
 	echo "folder1/dir/e3" >>expect &&
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b1316e62f46..bc216d012cb 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1852,7 +1852,7 @@ test_expect_success 'recursive tagging should give advice' '
 	hint: 	git tag -f nested annotated-v4.0^{}
 	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
-	git tag -m nested nested annotated-v4.0 2>actual &&
+	GIT_ADVICE=1 git tag -m nested nested annotated-v4.0 2>actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index aaeb4a53344..8dfb6885156 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -56,9 +56,10 @@ EOF
 		git rm foo &&
 		git commit -m delete &&
 		test_must_fail git merge main &&
-		test_must_fail git commit --dry-run >../actual &&
+		test_env GIT_ADVICE=1 test_must_fail \
+			git commit --dry-run >../actual &&
 		test_cmp ../expect ../actual &&
-		git status >../actual &&
+		test_env GIT_ADVICE=1 git status >../actual &&
 		test_cmp ../expect ../actual
 	)
 '
@@ -151,7 +152,7 @@ Unmerged paths:
 
 no changes added to commit (use "git add" and/or "git commit -a")
 EOF
-	git status --untracked-files=no >actual &&
+	GIT_ADVICE=1 git status --untracked-files=no >actual &&
 	test_cmp expected actual
 '
 
@@ -185,7 +186,7 @@ Unmerged paths:
 
 no changes added to commit (use "git add" and/or "git commit -a")
 EOF
-	git status --untracked-files=no >actual &&
+	GIT_ADVICE=1 git status --untracked-files=no >actual &&
 	test_cmp expected actual
 '
 
@@ -210,7 +211,7 @@ Unmerged paths:
 
 Untracked files not listed (use -u option to show untracked files)
 EOF
-	git status --untracked-files=no >actual &&
+	GIT_ADVICE=1 git status --untracked-files=no >actual &&
 	test_cmp expected actual &&
 	git reset --hard &&
 	git checkout main
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 2d984eb4c6a..9ee2374e3d2 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD' '
 	rev=$(git rev-parse --short renamer^) &&
 	git checkout -f renamer &&
 	git clean -f &&
-	git checkout renamer^ 2>messages &&
+	GIT_ADVICE=1 git checkout renamer^ 2>messages &&
 	grep "HEAD is now at $rev" messages &&
 	test_line_count -gt 1 messages &&
 	H=$(git rev-parse --verify HEAD) &&
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 098d8833b65..95e4bacd19e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -219,7 +219,7 @@ test_expect_success 'submodule add to .gitignored path fails' '
 		echo "*" > .gitignore &&
 		git add --force .gitignore &&
 		git commit -m"Ignore everything" &&
-		! git submodule add "$submodurl" submod >actual 2>&1 &&
+		test_env GIT_ADVICE=1 test_must_fail git submodule add "$submodurl" submod >actual 2>&1 &&
 		test_cmp expect actual
 	)
 '
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index aa2fdc31d1a..b155bd6e1c3 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -116,7 +116,8 @@ test_expect_success 'rebasing submodule that should conflict' '
 	test_tick &&
 	git commit -m fourth &&
 
-	test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&
+	test_env GIT_ADVICE=1 test_must_fail git rebase \
+		--onto HEAD^^ HEAD^ HEAD^0 2>actual_output &&
 	git ls-files -s submodule >actual &&
 	(
 		cd submodule &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 297c6c3b5cc..560eeea9c99 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -206,7 +206,7 @@ test_expect_success 'submodule update should fail due to local changes' '
 	 (cd submodule &&
 	  compare_head
 	 ) &&
-	 test_must_fail git submodule update submodule 2>../actual.raw
+	 test_env GIT_ADVICE=1 test_must_fail git submodule update submodule 2>../actual.raw
 	) &&
 	sed "s/^> //" >expect <<-\EOF &&
 	> error: Your local changes to the following files would be overwritten by checkout:
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index cdd5f2c6979..de277257d50 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -847,7 +847,7 @@ EOF
 test_expect_success 'status shows cherry-pick with invalid oid' '
 	mkdir .git/sequencer &&
 	test_write_lines "pick invalid-oid" >.git/sequencer/todo &&
-	git status --untracked-files=no >actual 2>err &&
+	GIT_ADVICE=1 git status --untracked-files=no >actual 2>err &&
 	git cherry-pick --quit &&
 	test_must_be_empty err &&
 	test_cmp expected actual
@@ -856,7 +856,7 @@ test_expect_success 'status shows cherry-pick with invalid oid' '
 test_expect_success 'status does not show error if .git/sequencer is a file' '
 	test_when_finished "rm .git/sequencer" &&
 	test_write_lines hello >.git/sequencer &&
-	git status --untracked-files=no 2>err &&
+	GIT_ADVICE=1 git status --untracked-files=no 2>err &&
 	test_must_be_empty err
 '
 
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index 3b63c34a309..21e088894c3 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -12,27 +12,27 @@ test_expect_success setup '
 '
 
 test_expect_success 'no warning if hook is not ignored' '
-	git commit --allow-empty -m "more" 2>message &&
+	GIT_ADVICE=1 git commit --allow-empty -m "more" 2>message &&
 	test_grep ! -e "hook was ignored" message
 '
 
 test_expect_success POSIXPERM 'warning if hook is ignored' '
 	test_hook --disable pre-commit &&
-	git commit --allow-empty -m "even more" 2>message &&
+	GIT_ADVICE=1 git commit --allow-empty -m "even more" 2>message &&
 	test_grep -e "hook was ignored" message
 '
 
 test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '
 	test_config advice.ignoredHook false &&
 	test_hook --disable pre-commit &&
-	git commit --allow-empty -m "even more" 2>message &&
+	GIT_ADVICE=1 git commit --allow-empty -m "even more" 2>message &&
 	test_grep ! -e "hook was ignored" message
 '
 
 test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
 	test_hook --remove pre-commit &&
 	test_unconfig advice.ignoredHook &&
-	git commit --allow-empty -m "even more" 2>message &&
+	GIT_ADVICE=1 git commit --allow-empty -m "even more" 2>message &&
 	test_grep ! -e "hook was ignored" message
 '

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 6/7] t7508/12: set GIT_ADVICE=1 across all tests

From: Derrick Stolee <derrickstolee@github.com>

The output of 'git status' changes depending on the availability of advice, even though the messages are to stdout. Since this test script is all about testing the output of 'git status' including the existence (or lack of) these messages, set the GIT_ADVICE environment globally across the script.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t7508-status.sh      | 4 ++++
 t/t7512-status-help.sh | 4 ++++
 2 files changed, 8 insertions(+)

Show changes to 2 files +8 −0

t/t7508-status.sh, t/t7512-status-help.sh

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 773383fefb5..7158ee57f37 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -9,6 +9,10 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+# 'git status' output changes depending on the availability of advice,
+# so force its output to enable advice, even though it goes to stdout.
+GIT_ADVICE=1 && export GIT_ADVICE
+
 test_expect_success 'status -h in broken repository' '
 	git config --global advice.statusuoption false &&
 	mkdir broken &&
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index de277257d50..1d9676bb3e2 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -17,6 +17,10 @@ TEST_PASSES_SANITIZE_LEAK=true
 
 set_fake_editor
 
+# 'git status' output changes depending on the availability of advice,
+# so force its output to enable advice, even though it goes to stdout.
+GIT_ADVICE=1 && export GIT_ADVICE
+
 test_expect_success 'prepare for conflicts' '
 	git config --global advice.statusuoption false &&
 	test_commit init main.txt init &&

-- 
gitgitgadget

Derrick Stolee via GitGitGadget· Aug 21, 2024, 11:02 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 7/7] advice: refuse to output if stderr not TTY

From: Derrick Stolee <derrickstolee@github.com>

The advice system is intended to help end users around corner cases or other difficult spots when using the Git tool. As such, they are added without considering the possibility that they could break scripts or external tools that execute Git processes and then parse the output.

I will not debate the merit of tools parsing stderr, but instead attempt to be helpful to tool authors by avoiding these behavior changes across Git versions.

In b79deeb5544 (advice: add --no-advice global option, 2024-05-03), the --no-advice option was presented as a way to help tool authors specify that they do not want any advice messages. As part of this implementation, the GIT_ADVICE environment variable is given as a way to communicate the desire for advice (=1) or no advice (=0) and pass that along to all child processes.

However, both the --no-advice option and the GIT_ADVICE environment variable require the tool author to change how they interact with Git to gain this protection.

If Git instead disables the advice system when stderr is not a terminal, then tool authors benefit immediately.

It is important, though, to let interested users force advice to be enabled, even when redirecting stderr to a non-terminal file. Be sure to test this by ensuring GIT_ADVICE=1 forces advice to be written to non-terminals.

The changes leading up to this already set GIT_ADVICE=1 in all other test scripts that care about the advice being output (or not).

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/config/advice.txt |  9 ++++++---
 advice.c                        |  4 +++-
 t/t0018-advice.sh               | 18 +++++++++++++-----
 3 files changed, 22 insertions(+), 9 deletions(-)

Show changes to 3 files +22 −9

Documentation/config/advice.txt, advice.c, t/t0018-advice.sh

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 0ba89898207..4946a8aff8d 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -1,8 +1,11 @@
 advice.*::
 	These variables control various optional help messages designed to
-	aid new users.  When left unconfigured, Git will give the message
-	alongside instructions on how to squelch it.  You can tell Git
-	that you do not need the help message by setting these to `false`:
+	aid new users. These are only output to `stderr` when it is a
+	terminal.
++
+When left unconfigured, Git will give the message alongside instructions
+on how to squelch it.  You can tell Git that you do not need the help
+message by setting these to `false`:
 +
 --
 	addEmbeddedRepo::
diff --git a/advice.c b/advice.c
index 6b879d805c0..05cf467b680 100644
--- a/advice.c
+++ b/advice.c
@@ -133,7 +133,9 @@ int advice_enabled(enum advice_type type)
 	static int globally_enabled = -1;
 
 	if (globally_enabled < 0)
-		globally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1);
+		globally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, -1);
+	if (globally_enabled < 0)
+		globally_enabled = isatty(2);
 	if (!globally_enabled)
 		return 0;
 
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index fac52322a7f..c63ef070a76 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -8,7 +8,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'advice should be printed when config variable is unset' '
+test_expect_success TTY 'advice should be printed when config variable is unset' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
 	hint: Disable this message with "git config advice.nestedTag false"
@@ -17,7 +17,7 @@ test_expect_success 'advice should be printed when config variable is unset' '
 	test_cmp expect actual
 '
 
-test_expect_success 'advice should be printed when config variable is set to true' '
+test_expect_success TTY 'advice should be printed when config variable is set to true' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
 	EOF
@@ -26,13 +26,13 @@ test_expect_success 'advice should be printed when config variable is set to tru
 	test_cmp expect actual
 '
 
-test_expect_success 'advice should not be printed when config variable is set to false' '
+test_expect_success TTY 'advice should not be printed when config variable is set to false' '
 	test_config advice.nestedTag false &&
 	test-tool advise "This is a piece of advice" 2>actual &&
 	test_must_be_empty actual
 '
 
-test_expect_success 'advice should not be printed when --no-advice is used' '
+test_expect_success TTY 'advice should not be printed when --no-advice is used' '
 	q_to_tab >expect <<-\EOF &&
 	On branch trunk
 
@@ -54,7 +54,7 @@ test_expect_success 'advice should not be printed when --no-advice is used' '
 	test_cmp expect actual
 '
 
-test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '
+test_expect_success TTY 'advice should not be printed when GIT_ADVICE is set to false' '
 	q_to_tab >expect <<-\EOF &&
 	On branch trunk
 
@@ -76,6 +76,8 @@ test_expect_success 'advice should not be printed when GIT_ADVICE is set to fals
 	test_cmp expect actual
 '
 
+# This test also verifies that GIT_ADVICE=1 ignores the requirement
+# that stderr is a terminal.
 test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '
 	q_to_tab >expect <<-\EOF &&
 	On branch trunk
@@ -99,4 +101,10 @@ test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '
 	test_cmp expect actual
 '
 
+test_expect_success 'advice should not be printed when stderr is not a terminal' '
+	test_config advice.nestedTag true &&
+	test-tool advise "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
 test_done

-- 
gitgitgadget

Jeff King· Aug 21, 2024, 15:40 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

On Wed, Aug 21, 2024 at 11:02:25AM +0000, Derrick Stolee via GitGitGadget wrote:

Show 8 quoted lines

> Advice is supposed to be for humans, not machines. Why do we output it when
> stderr is not a terminal? Let's stop doing that.
> 
> I'm labeling this as an RFC because I believe there is some risk with this
> change. In particular, this does change behavior to reduce the output that
> some scripts may depend upon. But this output is not intended to be locked
> in and we add or edit advice messages without considering this impact, so
> there is risk in the existing system already.

Playing devil's advocate for a moment: what about programs that read stderr but intend to relay the output to the user?

For example, programs running on the server side of a push are spawned by receive-pack with their stderr fed into a muxer that ships it to the client, who then dumps it to the user's terminal. Would we ever want to see their advice?

My guess is "conceivably yes", though I don't know of a specific example (and in fact, I've seen the "your hook was ignored because it's not executable" advice coming from a server, which was actually more of an annoyance on the client side).

Ditto for upload-pack. Another possible place where it matters: interfaces that wrap Git and collect the output to show to the user. I don't use git-gui, but I'd imagine it does this in some places.

Looking over patch 7, I think the escape hatch for all of these cases would be setting GIT_ADVICE=1. Which isn't too bad, but it does require some action. I'm not sure if it is worth it (but then, I am not all that sympathetic to the script you mentioned that was trying to be too clever about parsing stderr).

-Peff

Junio C Hamano· Aug 21, 2024, 16:39 UTC · re: Jeff King · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

> Playing devil's advocate for a moment: what about programs that read
> stderr but intend to relay the output to the user?
>
> For example, programs running on the server side of a push are spawned
> by receive-pack with their stderr fed into a muxer that ships it to the
> client, who then dumps it to the user's terminal. Would we ever want to
> see their advice?
>
> My guess is "conceivably yes", though I don't know of a specific example
> (and in fact, I've seen the "your hook was ignored because it's not
> executable" advice coming from a server, which was actually more of an
> annoyance on the client side).

Ah, I should have waited to think about the topic before reading what you wrote. Yes, this is a huge downside.

Show 5 quoted lines

> Looking over patch 7, I think the escape hatch for all of these cases
> would be setting GIT_ADVICE=1. Which isn't too bad, but it does require
> some action. I'm not sure if it is worth it (but then, I am not all that
> sympathetic to the script you mentioned that was trying to be too clever
> about parsing stderr).

This too.

Junio C Hamano· Aug 21, 2024, 16:36 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Advice is supposed to be for humans, not machines. Why do we output it when
> stderr is not a terminal? Let's stop doing that.

Last night while skimming the series on my phone (read: not a real review at all), I found it very annoying that GIT_ADVICE=1 had to be sprinkled all over the place. I wonder if we want to instead set and export it in t/test-lib.sh and turn it off as needed?

The end-to-end tests we have are primarily to guarantee the continuity of the end-user experience by humans, and ensuring that an advice message is given when appropriate and it does not get shown otherwise is very much inherent part of them. An alternative workaround to counteract the breakage this series causes of course is to run everything under test_terminal and it probably is much more kosher philosophically ;-), but compared to that, globally disabling the "if (!isatty(2))" while running the tests, and temporarily lifting that disabling during tests of the new feature added by this series would be easier to reason about, I would suspect.

Show 5 quoted lines

> This series is motivated by an internal tool breaking due to the advice
> message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index
> expands, 2024-07-08). This tool is assuming that any output to stderr is an
> error, and in this case is attempting to parse it to determine what kind of
> error (warning, error, or failure).

The "anything on stderr is an error" attitude needs to be fixed regardless of where it comes from (tcl/tk scripts have, or at least used to have, the tendency, which I found annoying), but regardless, I thought we added a mechanism to squelch all advice messages for this exact purpose at f0e21837 (Merge branch 'jl/git-no-advice', 2024-05-16). Why isn't the tool using the mechanism that already exists?

I would have supported the behaviour proposed by this series 100% if it were on the table when we were introducing the advise mechanism, but unfortunately nobody seemed have suggested it back then. I am willing to go with an "experiment" to change the behaviour, deliberately breaking "backward compatibility", if we have a wide support here during the review period. FWIW, I think any scripts that scrape the advice messages are already broken.

Patrick Steinhardt· Aug 22, 2024, 06:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

On Wed, Aug 21, 2024 at 09:36:56AM -0700, Junio C Hamano wrote:

Show 43 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > Advice is supposed to be for humans, not machines. Why do we output it when
> > stderr is not a terminal? Let's stop doing that.
> 
> Last night while skimming the series on my phone (read: not a real
> review at all), I found it very annoying that GIT_ADVICE=1 had to be
> sprinkled all over the place.  I wonder if we want to instead set
> and export it in t/test-lib.sh and turn it off as needed?
> 
> The end-to-end tests we have are primarily to guarantee the
> continuity of the end-user experience by humans, and ensuring that
> an advice message is given when appropriate and it does not get
> shown otherwise is very much inherent part of them.  An alternative
> workaround to counteract the breakage this series causes of course
> is to run everything under test_terminal and it probably is much
> more kosher philosophically ;-), but compared to that, globally
> disabling the "if (!isatty(2))" while running the tests, and
> temporarily lifting that disabling during tests of the new feature
> added by this series would be easier to reason about, I would
> suspect.
> 
> > This series is motivated by an internal tool breaking due to the advice
> > message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index
> > expands, 2024-07-08). This tool is assuming that any output to stderr is an
> > error, and in this case is attempting to parse it to determine what kind of
> > error (warning, error, or failure).
> 
> The "anything on stderr is an error" attitude needs to be fixed
> regardless of where it comes from (tcl/tk scripts have, or at least
> used to have, the tendency, which I found annoying), but regardless,
> I thought we added a mechanism to squelch all advice messages for
> this exact purpose at f0e21837 (Merge branch 'jl/git-no-advice',
> 2024-05-16).  Why isn't the tool using the mechanism that already
> exists?
> 
> I would have supported the behaviour proposed by this series 100% if
> it were on the table when we were introducing the advise mechanism,
> but unfortunately nobody seemed have suggested it back then.  I am
> willing to go with an "experiment" to change the behaviour,
> deliberately breaking "backward compatibility", if we have a wide
> support here during the review period.  FWIW, I think any scripts
> that scrape the advice messages are already broken.

I continue to believe that the biggest issue in this context is that there is no proper interface between Git and its caller that would allow the caller to learn about errors in a machine-parseable way. Matching error messages against regular expressions is bad, and can easily be broken by the output changing in whatever way. This may be because the error message itself was changed, or it may be because we have started to show advice messages. It's extremely fragile, and from my point of view there is no good way to classify errors right now.

I won't argue that checking whether stderr is empty or not is good -- it almost certainly feels wrong to me. But that's only one small part of a more widespread issue. Having structured error handling in Git, e.g. via a new structure that represents errors as discussed a couple of months ago [1] would go a long way. I didn't quite like the approach chosen by that patch series, but think that the idea certainly has merit.

The other question is why advice is being shown in the first place. In theory, all one should ever use in scripted usecases are plumbing tools. And as plumbing tools are explicitly not designed for users, they should never show advice in the first place. I guess chances are high though that the scripts in question used porcelain. That is also understandable though: our plumbing tools are often not as powerful as the porcelain ones, which has been lamented on the mailing list several times.

So I certainly get the sentiment of this patch series, but feel like we continue to work around the underlying problems. Those are rooted rather deep though, so fixing them is nothing we can do in a release or two, but rather on the order of years. Meanwhile I guess we have to find short-term solutions.

Patrick

[1]: https://lore.kernel.org/git/pull.1666.git.git.1708241612.gitgitgadget@gmail.com/

Gabor Gombas· Aug 22, 2024, 06:03 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

Hi,

On Wed, Aug 21, 2024 at 11:02:25AM +0000, Derrick Stolee via GitGitGadget wrote:

> Advice is supposed to be for humans, not machines. Why do we output it when
> stderr is not a terminal? Let's stop doing that.

Really bad idea. "/some/script 2>&1 | tee /some/where | less" is a common, generic debug construct (with countless variations of the exact commands in the pipe - this is Unix, after all). If /some/script happens to run git, then I _do_ want to see all the diagnostic messages it might produce, both recorded at /some/where, and displayed by "less".

Regards, Gabor

Derrick Stolee· Aug 22, 2024, 13:15 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

On 8/21/24 7:02 AM, Derrick Stolee via GitGitGadget wrote:

Show 5 quoted lines

> Advice is supposed to be for humans, not machines. Why do we output it when
> stderr is not a terminal? Let's stop doing that.
> 
> I'm labeling this as an RFC because I believe there is some risk with this
> change.

Thanks, all, for the feedback about the risk of making such a change. I agree that we should not pursue this direction.

The main issues are:

  1. Some tools create a wrapper around Git and may want to supply the
     advice to the user by parsing stderr.

  2. The advice system has been on for a long time and we cannot know
     where other dependencies could be for it.

I'll abandon this RFC, but plan on the following action items:

  * Document GIT_ADVICE in Documentation/git.exe.

  * Modify Documentation/config/advice.txt to mention GIT_ADVICE and
    recommend that automated tools calling Git commands set it to zero.

  * If we have a place to recommend best practices for automation
    executing Git commands, then I would add GIT_ADVICE=0 as a
    recommendation there. I couldn't find one myself. Do we have one?

Thanks! -Stolee

Junio C Hamano· Aug 22, 2024, 16:25 UTC · re: Derrick Stolee · lore

Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY

Derrick Stolee <stolee@gmail.com> writes:

Show 14 quoted lines

> On 8/21/24 7:02 AM, Derrick Stolee via GitGitGadget wrote:
>> Advice is supposed to be for humans, not machines. Why do we output it when
>> stderr is not a terminal? Let's stop doing that.
>> I'm labeling this as an RFC because I believe there is some risk
>> with this
>> change. 
>
> Thanks, all, for the feedback about the risk of making such a change. I
> agree that we should not pursue this direction.
>
> The main issues are:
>
>  1. Some tools create a wrapper around Git and may want to supply the
>     advice to the user by parsing stderr.

Or they may just pass it through to the user without even parsing.

Show 9 quoted lines

>  2. The advice system has been on for a long time and we cannot know
>     where other dependencies could be for it.
>
> I'll abandon this RFC, but plan on the following action items:
>
>  * Document GIT_ADVICE in Documentation/git.exe.
>
>  * Modify Documentation/config/advice.txt to mention GIT_ADVICE and
>    recommend that automated tools calling Git commands set it to zero.

FWIW, not documenting it was very much deliberate to discourage folks placing it in their ~/.login file. I am OK with the above as long as "this is for tools" is stressed well enough.