Skip to content

[RFC] advice: refuse to output if stderr not TTY #1776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Documentation/config/advice.txt
Original file line number Diff line number Diff line change
@@ -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::
4 changes: 3 additions & 1 deletion advice.c
Original file line number Diff line number Diff line change
@@ -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;

2 changes: 1 addition & 1 deletion t/lib-httpd.sh
Original file line number Diff line number Diff line change
@@ -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 &&
18 changes: 13 additions & 5 deletions t/t0018-advice.sh
Original file line number Diff line number Diff line change
@@ -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
18 changes: 9 additions & 9 deletions t/t1092-sparse-checkout-compatibility.sh
Original file line number Diff line number Diff line change
@@ -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
'

25 changes: 15 additions & 10 deletions t/t2020-checkout-detach.sh
Original file line number Diff line number Diff line change
@@ -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 &&

5 changes: 3 additions & 2 deletions t/t2024-checkout-dwim.sh
Original file line number Diff line number Diff line change
@@ -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 &&
4 changes: 2 additions & 2 deletions t/t2060-switch.sh
Original file line number Diff line number Diff line change
@@ -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
'

8 changes: 4 additions & 4 deletions t/t2204-add-ignored.sh
Original file line number Diff line number Diff line change
@@ -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
)
12 changes: 6 additions & 6 deletions t/t2400-worktree-add.sh
Original file line number Diff line number Diff line change
@@ -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 &&
4 changes: 2 additions & 2 deletions t/t3200-branch.sh
Original file line number Diff line number Diff line change
@@ -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
'

2 changes: 1 addition & 1 deletion t/t3404-rebase-interactive.sh
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion t/t3501-revert-cherry-pick.sh
Original file line number Diff line number Diff line change
@@ -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
'

Loading