Taylor Blau <me@xxxxxxxxxxxx> 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.
