| Message ID | 20240424035857.84583-1-james@jamesliu.io (mailing list archive) |
|---|---|
| Headers | show |
| Series | advice: add "all" option to disable all hints | expand |
James Liu <james@jamesliu.io> writes: > This patch series adds an "all" advice hint type that can be used as a > convenience option for disabling all advice hints. Hmph. I thought this was rejected already and not in so distant past, but I am not finding a discussion thread in the archive. The design to support the advice.* variables to let individual ones to be squelched, without allowing "all", is very much deliberate. A user may think they are familiar with all the advices already, but with "all", they'll never get a chance to learn new ones added after they set the configuration. Looking from the other direction, a new advice message is a way for us to tell our users something important for them to know. For example, we may plan to improve a high-level Porcelain command so that it will eventually error out when given an input that used to be accepted but behaved in a way that newbies felt confusing. In the first step of such a transition, we will introduce a new (and hopefully better) way to achieve what the user wants to do, and add an advice message to tell the user, when they trigger the codepath whose behaviour will change in the future, in the old way that will eventually go away. Do not close that communication channel on us. Thanks.
On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote: > James Liu <james@jamesliu.io> writes: > > > This patch series adds an "all" advice hint type that can be used as a > > convenience option for disabling all advice hints. > > Hmph. I thought this was rejected already and not in so distant > past, but I am not finding a discussion thread in the archive. > > The design to support the advice.* variables to let individual ones > to be squelched, without allowing "all", is very much deliberate. > > A user may think they are familiar with all the advices already, but > with "all", they'll never get a chance to learn new ones added after > they set the configuration. Looking from the other direction, a new > advice message is a way for us to tell our users something important > for them to know. For example, we may plan to improve a high-level > Porcelain command so that it will eventually error out when given an > input that used to be accepted but behaved in a way that newbies > felt confusing. In the first step of such a transition, we will > introduce a new (and hopefully better) way to achieve what the user > wants to do, and add an advice message to tell the user, when they > trigger the codepath whose behaviour will change in the future, in > the old way that will eventually go away. > > Do not close that communication channel on us. While I agree that it might not be a good idea to set it for our users, the usecase mentioned by this patch series is scripting. And here I very much agree with the sentiment that it makes sense to give an easy knob to disable all advice (disclosure: James is part of the Gitaly team at GitLab, and that is where this feature comes from, so I am very much biased). It has happened multiple times to us that new advices were introduced that then subsequently caused regressions in Gitaly because the output of Git commands now looks different. While addressing such breakage is easy enough to do, it does add up if we have to run Git with every single advice that we may hit turned off individually. Now one could say that we shouldn't execute porcelain tools in our scripts because it is kind of expected that their output may change at any point in time, and that is certainly true. But the reality is that there aren't always good plumbing alternatives available. Furthermore, we are often forced to parse fragile error messages from such porcelain tools because Git doesn't provide a better way to dissect errors. Error codes are mostly meaningless and there is no other data channel available to us than the error message. These are problems that run deeper than "We want to disable advices", and we eventually want to address those over time. But it's a long road to get there, and meanwhile disabling all advice would be something that helps us make our scripted uses of Git at least a tiny bit more stable. Patrick
On 2024-04-24 08:28, Junio C Hamano wrote: > James Liu <james@jamesliu.io> writes: > >> This patch series adds an "all" advice hint type that can be used as a >> convenience option for disabling all advice hints. > > Hmph. I thought this was rejected already and not in so distant > past, but I am not finding a discussion thread in the archive. > > The design to support the advice.* variables to let individual ones > to be squelched, without allowing "all", is very much deliberate. > > A user may think they are familiar with all the advices already, but > with "all", they'll never get a chance to learn new ones added after > they set the configuration. Looking from the other direction, a new > advice message is a way for us to tell our users something important > for them to know. For example, we may plan to improve a high-level > Porcelain command so that it will eventually error out when given an > input that used to be accepted but behaved in a way that newbies > felt confusing. In the first step of such a transition, we will > introduce a new (and hopefully better) way to achieve what the user > wants to do, and add an advice message to tell the user, when they > trigger the codepath whose behaviour will change in the future, in > the old way that will eventually go away. I'd agree with Junio. Having "advice.all" is like a chainsaw, very useful, but also quite dangerous if not used properly.
Hi Patrick On 24/04/2024 07:48, Patrick Steinhardt wrote: > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote: >> Do not close that communication channel on us. > > While I agree that it might not be a good idea to set it for our users, > the usecase mentioned by this patch series is scripting. And here I very > much agree with the sentiment that it makes sense to give an easy knob > to disable all advice (disclosure: James is part of the Gitaly team at > GitLab, and that is where this feature comes from, so I am very much > biased). Maybe an environment variable would be a better fit for turning advice off in scripts? > It has happened multiple times to us that new advices were introduced > that then subsequently caused regressions in Gitaly because the output > of Git commands now looks different. While addressing such breakage is > easy enough to do, it does add up if we have to run Git with every > single advice that we may hit turned off individually. I'm sure you've considered these suggestions but (a) would it be possible for Gitaly to filter out lines beginning with "hint: " when it captures the output of commands and (b) would it be possible to have a script that parses advice_setting in advice.c to find the list of advice names so Gitaly can disable them? I think (a) would still leave some advice output from code that uses advice_enabled() rather than advise_if_enabled() but it should get rid of most of the advice messages. Best Wishes Phillip > Now one could say that we shouldn't execute porcelain tools in our > scripts because it is kind of expected that their output may change at > any point in time, and that is certainly true. But the reality is that > there aren't always good plumbing alternatives available. > > Furthermore, we are often forced to parse fragile error messages from > such porcelain tools because Git doesn't provide a better way to dissect > errors. Error codes are mostly meaningless and there is no other data > channel available to us than the error message. > > These are problems that run deeper than "We want to disable advices", > and we eventually want to address those over time. But it's a long road > to get there, and meanwhile disabling all advice would be something that > helps us make our scripted uses of Git at least a tiny bit more stable. > > Patrick
On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote: > Hi Patrick > > On 24/04/2024 07:48, Patrick Steinhardt wrote: > > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote: > > > Do not close that communication channel on us. > > > > While I agree that it might not be a good idea to set it for our users, > > the usecase mentioned by this patch series is scripting. And here I very > > much agree with the sentiment that it makes sense to give an easy knob > > to disable all advice (disclosure: James is part of the Gitaly team at > > GitLab, and that is where this feature comes from, so I am very much > > biased). > > Maybe an environment variable would be a better fit for turning advice off > in scripts? Sure, an environment variable would work just fine for our purposes. It would probably also address the concern that users may disable all advice and then miss out on some information. > > It has happened multiple times to us that new advices were introduced > > that then subsequently caused regressions in Gitaly because the output > > of Git commands now looks different. While addressing such breakage is > > easy enough to do, it does add up if we have to run Git with every > > single advice that we may hit turned off individually. > > I'm sure you've considered these suggestions but (a) would it be possible > for Gitaly to filter out lines beginning with "hint: " when it captures the > output of commands and (b) would it be possible to have a script that parses > advice_setting in advice.c to find the list of advice names so Gitaly can > disable them? I think (a) would still leave some advice output from code > that uses advice_enabled() rather than advise_if_enabled() but it should get > rid of most of the advice messages. Filtering out advices would be doable. But we probably wouldn't want to do so unconditionally whenever we execute Git commands. Which would bring us back to the point where we have to address these new advices whenever they come up, and it wouldn't be much less verbose than to just disable advices we face. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> Do not close that communication channel on us. > > While I agree that it might not be a good idea to set it for our users, > the usecase mentioned by this patch series is scripting. And here I very > much agree with the sentiment that it makes sense to give an easy knob > to disable all advice (disclosure: James is part of the Gitaly team at > GitLab, and that is where this feature comes from, so I am very much > biased). Of course, as you mention, the kosher way is to use the plumbing where there is no advice messages. If certain functionalitly is lacking in the set of plumbing commands, adding them would benefit everybody, not just Gitaly. If this is for scripting, make it easy for scripts to use while making it very very hard for users to trigger in their interactive environments. A configuration variable, by design, is a very bad choice to do so, as it is "set it once and forget about it", which defeats the whole purpose of advice messages. An environment variable is very much in the same category in that you can set it in ~/.login or ~/.profile and forget about it. A "git" wide command line option, similar to "--no-pager" or "--literal-pathspecs", is probably an acceptable avenue. Thanks.
Patrick Steinhardt <ps@pks.im> writes: > Filtering out advices would be doable. But we probably wouldn't want to > do so unconditionally whenever we execute Git commands. Can you elaborate? Would you only sometimes show advice messages that come from "git" you invoke? Based on what criteria would it decide whether to filter or not to? Is it purely per location in your program (i.e., "every time the control flow reaches this call to an equivalent of run_command(), we would filter the "hint:" messages")? In an invocation where you would not filter, what effect does users' setting of advice.* configuration variables have, and what effect does a new and unseen kind of advice messages have?
Hello all, On 2024-04-24 16:07, Patrick Steinhardt wrote: > On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote: >> On 24/04/2024 07:48, Patrick Steinhardt wrote: >> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote: >> > > Do not close that communication channel on us. >> > >> > While I agree that it might not be a good idea to set it for our users, >> > the usecase mentioned by this patch series is scripting. And here I very >> > much agree with the sentiment that it makes sense to give an easy knob >> > to disable all advice (disclosure: James is part of the Gitaly team at >> > GitLab, and that is where this feature comes from, so I am very much >> > biased). >> >> Maybe an environment variable would be a better fit for turning advice >> off >> in scripts? > > Sure, an environment variable would work just fine for our purposes. It > would probably also address the concern that users may disable all > advice and then miss out on some information. Sounds good to me. I'd support the addition of a new environment variable for this purpose, perhaps GIT_NO_ADVICE or similar.
On 2024-04-24 18:14, Dragan Simic wrote: > On 2024-04-24 16:07, Patrick Steinhardt wrote: >> On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote: >>> On 24/04/2024 07:48, Patrick Steinhardt wrote: >>> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote: >>> > > Do not close that communication channel on us. >>> > >>> > While I agree that it might not be a good idea to set it for our users, >>> > the usecase mentioned by this patch series is scripting. And here I very >>> > much agree with the sentiment that it makes sense to give an easy knob >>> > to disable all advice (disclosure: James is part of the Gitaly team at >>> > GitLab, and that is where this feature comes from, so I am very much >>> > biased). >>> >>> Maybe an environment variable would be a better fit for turning >>> advice off >>> in scripts? >> >> Sure, an environment variable would work just fine for our purposes. >> It >> would probably also address the concern that users may disable all >> advice and then miss out on some information. > > Sounds good to me. I'd support the addition of a new environment > variable for this purpose, perhaps GIT_NO_ADVICE or similar. Actually, after reading Junio's comment, [1] I'd rather support a new command-line option. This is quite similar to disabling the pager, so a new command-line option would IMHO fit rather well into the grand scheme. [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
On Wed, Apr 24, 2024 at 07:31:04AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> Do not close that communication channel on us. > > > > While I agree that it might not be a good idea to set it for our users, > > the usecase mentioned by this patch series is scripting. And here I very > > much agree with the sentiment that it makes sense to give an easy knob > > to disable all advice (disclosure: James is part of the Gitaly team at > > GitLab, and that is where this feature comes from, so I am very much > > biased). > > Of course, as you mention, the kosher way is to use the plumbing > where there is no advice messages. If certain functionalitly is > lacking in the set of plumbing commands, adding them would benefit > everybody, not just Gitaly. > > If this is for scripting, make it easy for scripts to use while > making it very very hard for users to trigger in their interactive > environments. A configuration variable, by design, is a very bad > choice to do so, as it is "set it once and forget about it", which > defeats the whole purpose of advice messages. An environment > variable is very much in the same category in that you can set it in > ~/.login or ~/.profile and forget about it. > > A "git" wide command line option, similar to "--no-pager" or > "--literal-pathspecs", is probably an acceptable avenue. Fair enough. So adding a global config like `--no-advice` it is. Patrick
On Wed, Apr 24, 2024 at 07:59:16AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Filtering out advices would be doable. But we probably wouldn't want to > > do so unconditionally whenever we execute Git commands. > > Can you elaborate? Would you only sometimes show advice messages > that come from "git" you invoke? Based on what criteria would it > decide whether to filter or not to? Is it purely per location in > your program (i.e., "every time the control flow reaches this call > to an equivalent of run_command(), we would filter the "hint:" > messages")? > > In an invocation where you would not filter, what effect does users' > setting of advice.* configuration variables have, and what effect > does a new and unseen kind of advice messages have? The reason here is mostly that I do not know whether this can be rigged. I cannot state without a doubt that no command may output "hint:" at the start of a line that may _not_ actually be an advice. Think for example (and I know this example is dumb because advice goes to stderr, not stdout) git-cat-file(1) with a blob that contains a line saying "hint:". Patrick
Patrick Steinhardt <ps@pks.im> writes: >> > Filtering out advices would be doable. But we probably wouldn't want to >> > do so unconditionally whenever we execute Git commands. >> ... > The reason here is mostly that I do not know whether this can be rigged. > I cannot state without a doubt that no command may output "hint:" at the > start of a line that may _not_ actually be an advice. Ah, I mistook what you said to be "we may want to keep the advice messages in certain situations while filtering out others". Surely, we may send stuff to the standard error stream that happens to begin with "hint:" that is not created via advice.c:vadvise(). I do not however offhand know why anybody would want to filter such lines out when they are filtering out all advice messages. When we emit a line that begins with "hint:", I do not think of a case when we do not mean as a "hint", i.e., in the same spirit as real advice messages.