Re: BUG: Various "advice.*" config doesn't work

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2022, 17:28 UTC
Message-ID: <xmqq1r0nke71.fsf@gitster.g>
In-Reply-To: <YfgLeVw0rrk7Q5/+@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 7 quoted lines

> But having something like (in builtin/add.c:add_files()):
>
>     if (advice_enabled(ADVICE_ADD_IGNORED_FILES))
>         advise(_("..."));
>
> feels like it opens the door to call advise() by default if we happened
> to forget to read the configuration.

True.

> I think that is a good candidate to
> be replaced with advice_if_enabled().

Meaning advice_enabled() will lazily load the configuration? If so, then what you saw in builtin/add.c::add_files() would automatically become just as safe as advice_if_enabled(), no?

Show 14 quoted lines

> I'm not sure if that is true in general, though. Take a look at this
> example (from branch.c:create_branch()):
>
>     if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {
>         error(_(upstream_missing), start_name);
>         advise(_(upstream_advice));
>         exit(1);
>     }
>     die(_(upstream_missing), start_name);
>
> This also makes it possible to call advise() when we shouldn't have. But
> how should we rewrite this code? Wanting to either error() (and then
> call exit(1)) or die() based on whether or not we're going to print the
> advice makes it tricky.

I am puzzled why you think the above "check, do things, give a piece of advice, and do even more things" needs to be rewritten.

Everything you are showing above becomes a problem only when advice_enabled() does not work reliably, due to a bug that fails to read the configuration.

> Maybe, though I still think BUG() is a bit extreme, and we could
> accomplish the same by having the advice API just read the config if it
> hasn't done so already before emitting advice.

Calling things like git_config(git_default_config) with side-effects on other global variables are definitely a no-no, but as long as it reacts to configuration variables only under advice.* namespace, that might be OK.

Thanks.