* [PATCH 1/7] t1000-2000: add GIT_ADVICE=1 for advice tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 2/7] t3000-4000: add GIT_ADVICE=1 to " Derrick Stolee via GitGitGadget
` (9 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/7] t3000-4000: add GIT_ADVICE=1 to advice tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 1/7] t1000-2000: add GIT_ADVICE=1 for advice tests Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 3/7] t5000: " Derrick Stolee via GitGitGadget
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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/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(-)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/7] t5000: add GIT_ADVICE=1 to advice tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 1/7] t1000-2000: add GIT_ADVICE=1 for advice tests Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 2/7] t3000-4000: add GIT_ADVICE=1 to " Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 4/7] t6000: " Derrick Stolee via GitGitGadget
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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.
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(-)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 4/7] t6000: add GIT_ADVICE=1 to advice tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2024-08-21 11:02 ` [PATCH 3/7] t5000: " Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 5/7] t7000: " Derrick Stolee via GitGitGadget
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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/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(-)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 5/7] t7000: add GIT_ADVICE=1 to advice tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2024-08-21 11:02 ` [PATCH 4/7] t6000: " Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 6/7] t7508/12: set GIT_ADVICE=1 across all tests Derrick Stolee via GitGitGadget
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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.
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(-)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 6/7] t7508/12: set GIT_ADVICE=1 across all tests
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (4 preceding siblings ...)
2024-08-21 11:02 ` [PATCH 5/7] t7000: " Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 11:02 ` [PATCH 7/7] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (4 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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(+)
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 7/7] advice: refuse to output if stderr not TTY
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (5 preceding siblings ...)
2024-08-21 11:02 ` [PATCH 6/7] t7508/12: set GIT_ADVICE=1 across all tests Derrick Stolee via GitGitGadget
@ 2024-08-21 11:02 ` Derrick Stolee via GitGitGadget
2024-08-21 15:40 ` [PATCH 0/7] [RFC] " Jeff King
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2024-08-21 11:02 UTC (permalink / raw)
To: git; +Cc: gitster, Johannes.Schindelin, ps, james, Derrick Stolee,
Derrick Stolee
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (6 preceding siblings ...)
2024-08-21 11:02 ` [PATCH 7/7] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
@ 2024-08-21 15:40 ` Jeff King
2024-08-21 16:39 ` Junio C Hamano
2024-08-21 16:36 ` Junio C Hamano
` (2 subsequent siblings)
10 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2024-08-21 15:40 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, Johannes.Schindelin, ps, james, Derrick Stolee
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.
>
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 15:40 ` [PATCH 0/7] [RFC] " Jeff King
@ 2024-08-21 16:39 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2024-08-21 16:39 UTC (permalink / raw)
To: Jeff King
Cc: Derrick Stolee via GitGitGadget, git, Johannes.Schindelin, ps,
james, Derrick Stolee
Jeff King <peff@peff.net> writes:
> 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.
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (7 preceding siblings ...)
2024-08-21 15:40 ` [PATCH 0/7] [RFC] " Jeff King
@ 2024-08-21 16:36 ` Junio C Hamano
2024-08-22 6:19 ` Patrick Steinhardt
2024-08-22 6:03 ` Gabor Gombas
2024-08-22 13:15 ` Derrick Stolee
10 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2024-08-21 16:36 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, Johannes.Schindelin, ps, james, Derrick Stolee
"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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 16:36 ` Junio C Hamano
@ 2024-08-22 6:19 ` Patrick Steinhardt
0 siblings, 0 replies; 15+ messages in thread
From: Patrick Steinhardt @ 2024-08-22 6:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, Johannes.Schindelin, james,
Derrick Stolee
On Wed, Aug 21, 2024 at 09:36:56AM -0700, Junio C Hamano wrote:
> "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/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (8 preceding siblings ...)
2024-08-21 16:36 ` Junio C Hamano
@ 2024-08-22 6:03 ` Gabor Gombas
2024-08-22 13:15 ` Derrick Stolee
10 siblings, 0 replies; 15+ messages in thread
From: Gabor Gombas @ 2024-08-22 6:03 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, Johannes.Schindelin, ps, james, Derrick Stolee
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-21 11:02 [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY Derrick Stolee via GitGitGadget
` (9 preceding siblings ...)
2024-08-22 6:03 ` Gabor Gombas
@ 2024-08-22 13:15 ` Derrick Stolee
2024-08-22 16:25 ` Junio C Hamano
10 siblings, 1 reply; 15+ messages in thread
From: Derrick Stolee @ 2024-08-22 13:15 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, Johannes.Schindelin, ps, james, peff, gombasgg
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.
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/7] [RFC] advice: refuse to output if stderr not TTY
2024-08-22 13:15 ` Derrick Stolee
@ 2024-08-22 16:25 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2024-08-22 16:25 UTC (permalink / raw)
To: Derrick Stolee
Cc: Derrick Stolee via GitGitGadget, git, Johannes.Schindelin, ps,
james, peff, gombasgg
Derrick Stolee <stolee@gmail.com> writes:
> 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.
> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread