[PATCH 7/7] advice: refuse to output if stderr not TTY
- From
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Aug 21, 2024, 11:02 UTC
- Message-ID
- <25d769903b2ab4a4c454929bf6378751bd366a37.1724238153.git.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1776.git.1724238152.gitgitgadget@gmail.com>
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(-)
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