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

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

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

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