Taylor Blau [off-list ref] 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.
On Mon, Jan 31, 2022 at 09:28:02AM -0800, Junio C Hamano wrote:
Taylor Blau [off-list ref] writes:
quoted
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.
quoted
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?
The change I was wondering aloud about was having advice_enabled()
lazily load the configuration.
So what is written in add_files() above would become safe (if it wasn't
already), and could easily be replaced with advise_if_enabled().
quoted
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.
What I'm more or less trying to point out is that an unguarded advise()
function defeats the purpose of the advice API, which should exist to
avoid mistakes like these (where advice is printed to the user when they
have already opted out of that advice).
I was thinking that it would be nice to have advise() take an
advice_type enum and have it behave like advise_if_enabled(). But there
are spots that you really do want to print advice unconditionally. And
that spot in create_branch() is one of those where it isn't clear that
the change I'm proposing works.
(BTW, I would definitely disagree that I'm saying anything "needs" to be
rewritten here. This is all just thinking aloud about what the advice
API can and should help callers with.)
quoted
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.
Yes, I definitely agree we should absolutely not be calling
git_default_config() as part of "lazily load the advice.*
configuration".
Thanks.
Thanks,
Taylor