Re: [PATCH 0/2] advice: add "all" option to disable all hints

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 24, 2024, 13:52 UTC
Message-ID: <7d1d2892-d486-4e91-b100-e44c1d76893c@gmail.com>
In-Reply-To: <ZiirKgXQPLmtrwLT@tanuki>

Hi Patrick

On 24/04/2024 07:48, Patrick Steinhardt wrote:

Show 9 quoted lines

> 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?

Show 5 quoted lines

> 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

Show 16 quoted lines

> 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