[RFC] advice: refuse to output if stderr not TTY #1776
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
advice-tty
branch
from
af1b675 to
1546d1b
Compare
derrickstolee
force-pushed
the
advice-tty
branch
from
1546d1b to
add8e20
Compare
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>
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>
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>
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>
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>
derrickstolee
force-pushed
the
advice-tty
branch
from
add8e20 to
808e57d
Compare
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>
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 b79deeb (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>
derrickstolee
force-pushed
the
advice-tty
branch
from
808e57d to
25d7699
Compare
derrickstolee
changed the title
|
/submit |
|
Submitted as pull.1776.git.1724238152.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff King wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, Gabor Gombas wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): 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/ |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): 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
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 9479a31 (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
Thanks, - Stolee
cc: gitster@pobox.com
cc: Johannes.Schindelin@gmx.de
cc: ps@pks.im
cc: james@jamesliu.io
cc: Jeff King peff@peff.net
cc: Gabor Gombas gombasgg@gmail.com