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

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

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/20240424035857.84583-1-james@jamesliu.io/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
-- 
gitgitgadget