| Message ID | pull.1776.git.1724238152.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | advice: refuse to output if stderr not TTY | expand |
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
"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.
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.
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
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 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
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.