advice: refuse to output if stderr not TTY
cover advice: refuse to output if stderr not TTY
Advice is supposed to be for humans, not machines. Why do we output it when stderr is not a terminal? Let's stop doing that.
I'm labeling this as an RFC because I believe there is some risk with this change. In particular, this does change behavior to reduce the output that some scripts may depend upon. But this output is not intended to be locked in and we add or edit advice messages without considering this impact, so there is risk in the existing system already.
This series is motivated by an internal tool breaking due to the advice message added to Git 2.46.0 by 9479a31d603 (advice: warn when sparse index expands, 2024-07-08). This tool is assuming that any output to stderr is an error, and in this case is attempting to parse it to determine what kind of error (warning, error, or failure).
I've recommended that the tool author remove the advice message for now, but I'd like to help other tool authors avoid this surprise.
I read the thread for the --no-advice option [1] looking to see if this was presented as an option, but did not see it as part of that review. I hope that this is not considered a breaking change for users, but I could see the argument for that.
[1]
https://lore.kernel.org/git/[email protected]/t/#u
* Patches 1-5 are preparation patches to make the test library work to test the advice system after the final patch. These are split by test file name to reduce the size of the patches, but could be squashed into a megapatch if necessary. This is usually a simple addition of the GIT_ADVICE=1 environment variable, but there were some changes made to those lines to be more correct as necessary.
* Patch 6 highlights the fact that 'git status' uses advice_enabled() to determine if it should print certain parenthetical results. See format_tracking_info() in remote.c for an example. This output doesn't use the advise() method, but instead appends to a string buffer that is later sent to stdout. (If we think this part of the change is too risky, then we could move the isatty() out of advice_enabled() and into advise(), but that would not match the existing behavior of what is blocked by --no-advice.)
* Patch 7 modifies advice_enabled() to disable when isatty(2) is false and GIT_ADVICE is unset.
Thanks, - Stolee
Derrick Stolee (7):
t1000-2000: add GIT_ADVICE=1 for advice tests
t3000-4000: add GIT_ADVICE=1 to advice tests
t5000: add GIT_ADVICE=1 to advice tests
t6000: add GIT_ADVICE=1 to advice tests
t7000: add GIT_ADVICE=1 to advice tests
t7508/12: set GIT_ADVICE=1 across all tests
advice: refuse to output if stderr not TTY
Documentation/config/advice.txt | 9 ++- advice.c | 4 +- t/lib-httpd.sh | 2 +- t/t0018-advice.sh | 18 +++-- t/t1092-sparse-checkout-compatibility.sh | 18 ++--- t/t2020-checkout-detach.sh | 25 ++++--- t/t2024-checkout-dwim.sh | 5 +- t/t2060-switch.sh | 4 +- t/t2204-add-ignored.sh | 8 +-- t/t2400-worktree-add.sh | 12 ++-- t/t3200-branch.sh | 4 +- t/t3404-rebase-interactive.sh | 2 +- t/t3501-revert-cherry-pick.sh | 2 +- t/t3507-cherry-pick-conflict.sh | 4 +- t/t3510-cherry-pick-sequence.sh | 6 +- t/t3600-rm.sh | 12 ++-- t/t3602-rm-sparse-checkout.sh | 18 ++--- t/t3700-add.sh | 6 +- t/t3705-add-sparse-checkout.sh | 32 ++++----- t/t4150-am.sh | 14 ++-- t/t5505-remote.sh | 5 +- t/t5520-pull.sh | 4 +- t/t5541-http-push-smart.sh | 6 +- t/t6001-rev-list-graft.sh | 4 +- t/t6050-replace.sh | 6 +- t/t6436-merge-overwrite.sh | 6 +- t/t6437-submodule-merge.sh | 16 ++--- t/t6439-merge-co-error-msgs.sh | 12 ++-- t/t7002-mv-sparse-checkout.sh | 85 ++++++++++++----------- t/t7004-tag.sh | 2 +- t/t7060-wtstatus.sh | 11 +-- t/t7201-co.sh | 2 +- t/t7400-submodule-basic.sh | 2 +- t/t7402-submodule-rebase.sh | 3 +- t/t7406-submodule-update.sh | 2 +- t/t7500-commit-template-squash-signoff.sh | 3 +- t/t7508-status.sh | 4 ++ t/t7512-status-help.sh | 8 ++- t/t7520-ignored-hook-warning.sh | 8 +-- 39 files changed, 214 insertions(+), 180 deletions(-)
base-commit: bb9c16bd4f1a9a00799e10c81ee6506cf468c0c7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1776%2Fderrickstolee%2Fadvice-tty-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1776/derrickstolee/advice-tty-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1776
Signature · 1 line
gitgitgadget
PATCH 1/7 t1000-2000: add GIT_ADVICE=1 for advice tests
From: Derrick Stolee <[email protected]>
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.
--- 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(-)
t/t1092-sparse-checkout-compatibility.sh +9 -9
@@ -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 '
t/t2020-checkout-detach.sh +15 -10
@@ -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 &&
t/t2024-checkout-dwim.sh +3 -2
@@ -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 &&
t/t2060-switch.sh +2 -2
@@ -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 '
t/t2204-add-ignored.sh +4 -4
@@ -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 )
t/t2400-worktree-add.sh +6 -6
@@ -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 &&
t/t7500-commit-template-squash-signoff.sh +2 -1
@@ -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 '
Signature · 1 line
gitgitgadget
PATCH 2/7 t3000-4000: add GIT_ADVICE=1 to advice tests
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- 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(-)
t/t3200-branch.sh +2 -2
@@ -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 '
t/t3404-rebase-interactive.sh +1 -1
@@ -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
t/t3501-revert-cherry-pick.sh +1 -1
@@ -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 '
t/t3507-cherry-pick-conflict.sh +2 -2
@@ -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 "
t/t3510-cherry-pick-sequence.sh +3 -3
@@ -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 '
t/t3600-rm.sh +6 -6
@@ -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 '
t/t3602-rm-sparse-checkout.sh +9 -9
@@ -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 '
t/t3700-add.sh +3 -3
@@ -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' '
t/t3705-add-sparse-checkout.sh +16 -16
@@ -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 '
t/t4150-am.sh +7 -7
@@ -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 &&
Signature · 1 line
gitgitgadget
PATCH 3/7 t5000: add GIT_ADVICE=1 to advice tests
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- 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(-)
t/lib-httpd.sh +1 -1
@@ -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 &&
t/t5505-remote.sh +3 -2
@@ -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 ||
t/t5520-pull.sh +2 -2
@@ -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 '
t/t5541-http-push-smart.sh +4 -2
@@ -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 &&
Signature · 1 line
gitgitgadget
PATCH 4/7 t6000: add GIT_ADVICE=1 to advice tests
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- 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(-)
t/t6001-rev-list-graft.sh +2 -2
@@ -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 '
t/t6050-replace.sh +3 -3
@@ -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 '
t/t6436-merge-overwrite.sh +3 -3
@@ -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 '
t/t6437-submodule-merge.sh +8 -8
@@ -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)" &&
t/t6439-merge-co-error-msgs.sh +6 -6
@@ -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 '
Signature · 1 line
gitgitgadget
PATCH 5/7 t7000: add GIT_ADVICE=1 to advice tests
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- 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(-)
t/t7002-mv-sparse-checkout.sh +43 -42
@@ -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 &&
t/t7004-tag.sh +1 -1
@@ -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 '
t/t7060-wtstatus.sh +6 -5
@@ -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
t/t7201-co.sh +1 -1
@@ -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) &&
t/t7400-submodule-basic.sh +1 -1
@@ -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 ) '
t/t7402-submodule-rebase.sh +2 -1
@@ -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 &&
t/t7406-submodule-update.sh +1 -1
@@ -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:
t/t7512-status-help.sh +2 -2
@@ -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 '
t/t7520-ignored-hook-warning.sh +4 -4
@@ -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 '
Signature · 1 line
gitgitgadget
PATCH 6/7 t7508/12: set GIT_ADVICE=1 across all tests
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- t/t7508-status.sh | 4 ++++ t/t7512-status-help.sh | 4 ++++ 2 files changed, 8 insertions(+)
t/t7508-status.sh +4 -0
@@ -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 &&
t/t7512-status-help.sh +4 -0
@@ -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 &&
Signature · 1 line
gitgitgadget
PATCH 7/7 advice: refuse to output if stderr not TTY
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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).
--- Documentation/config/advice.txt | 9 ++++++--- advice.c | 4 +++- t/t0018-advice.sh | 18 +++++++++++++----- 3 files changed, 22 insertions(+), 9 deletions(-)
Documentation/config/advice.txt +6 -3
@@ -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::
advice.c +3 -1
@@ -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 ;
t/t0018-advice.sh +13 -5
@@ -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
Signature · 1 line
gitgitgadget
Quoted 8 lines · Derrick Stolee via GitGitGadget
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
Quoted 2 lines · Derrick Stolee via GitGitGadget
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.
Quoted 5 lines
This series is motivated by an internal tool breaking due to the advice message added to Git 2.46.0 by
9479a31d603(advice: warn when sparse index expands, 2024-07-08). This tool is assuming that any output to stderr is an error, and in this case is attempting to parse it to determine what kind of error (warning, error, or failure).
The "anything on stderr is an error" attitude needs to be fixed regardless of where it comes from (tcl/tk scripts have, or at least used to have, the tendency, which I found annoying), but regardless, I thought we added a mechanism to squelch all advice messages for this exact purpose at f0e21837 (Merge branch 'jl/git-no-advice', 2024-05-16). Why isn't the tool using the mechanism that already exists?
I would have supported the behaviour proposed by this series 100% if it were on the table when we were introducing the advise mechanism, but unfortunately nobody seemed have suggested it back then. I am willing to go with an "experiment" to change the behaviour, deliberately breaking "backward compatibility", if we have a wide support here during the review period. FWIW, I think any scripts that scrape the advice messages are already broken.
Quoted 8 lines · Jeff King
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?
Quoted 4 lines
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.
Quoted 5 lines
Looking over patch 7, I think the escape hatch for all of these cases would be setting GIT_ADVICE=1. Which isn't too bad, but it does require some action. I'm not sure if it is worth it (but then, I am not all that sympathetic to the script you mentioned that was trying to be too clever about parsing stderr).
This too.
Hi,
Quoted 2 lines · Derrick Stolee via GitGitGadget
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
Quoted 36 lines · Junio C Hamano
Quoted 2 lines · Derrick Stolee via GitGitGadget
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.shand 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_terminaland 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.Quoted 5 lines
This series is motivated by an internal tool breaking due to the advice message added to Git 2.46.0 by
9479a31d603(advice: warn when sparse index expands, 2024-07-08). This tool is assuming that any output to stderr is an error, and in this case is attempting to parse it to determine what kind of error (warning, error, or failure).The "anything on stderr is an error" attitude needs to be fixed regardless of where it comes from (tcl/tk scripts have, or at least used to have, the tendency, which I found annoying), but regardless, I thought we added a mechanism to squelch all advice messages for this exact purpose at
f0e21837(Merge branch 'jl/git-no-advice', 2024-05-16). Why isn't the tool using the mechanism that already exists?
Quoted 7 lines
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
Quoted 5 lines · Derrick Stolee via GitGitGadget
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
Quoted 7 lines · Derrick Stolee
Quoted 5 lines · Derrick Stolee via GitGitGadget
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.
Quoted 7 lines
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.
Quoted 9 lines
2. The advice system has been on for a long time and we cannot know where other dependencies could be for it.
I'll abandon this RFC, but plan on the following action items:
* Document
GIT_ADVICEinDocumentation/git.exe.* Modify
Documentation/config/advice.txtto mentionGIT_ADVICEand 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.