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

Commit Message

Derrick Stolee Aug. 21, 2024, 11:02 a.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
---
 Documentation/config/advice.txt |  9 ++++++---
 advice.c                        |  4 +++-
 t/t0018-advice.sh               | 18 +++++++++++++-----
 3 files changed, 22 insertions(+), 9 deletions(-)

Patch

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