Re: BUG: Various "advice.*" config doesn't work
On Fri, Jan 28, 2022 at 10:33:29AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 25 quoted lines
> $subject, which I started looking at because in 4f3e57ef13d
> (submodule--helper: advise on fatal alternate error, 2019-12-02) the
> scaffolding was set up to read config, but nothing actually did so. So
> "advice.submoduleAlternateErrorStrategyDie=false" has never worked. That
> can be tested with:
>
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index a3892f494b6..8918be9ef5c 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -193,7 +193,7 @@ test_expect_success 'missing nested submodule alternate fails clone and submodul
> cd supersuper-clone &&
> check_that_two_of_three_alternates_are_used &&
> # update of the submodule fails
> - test_must_fail git submodule update --init --recursive
> + git -c advice.submoduleAlternateErrorStrategyDie=false submodule update --init --recursive
> )
> '
>
> More generally, the advice API should be made to panic in
> advice_if_enabled() if nothing has read the advice config, which would
> turns up a lot of other such issues. I can't think of an easy way to
> check for that. We could add a BUG() on:
>
> if (!the_repository->config && !the_repository->config->hash_initialized)
I haven't spent much time looking in this area, but isn't the only thing that reads advice.* config the `git_default_advice_config()` callback?
If so, we could check whether or not it has been called (being careful to handle the case where we called git_config(), but had an empty configuration, so the callback itself was never run). If it hasn't, then we could BUG(), though I'd probably err on the side of just reading it right before calling advise().
In general, I wonder if making advise() public is a good idea. I think there are good uses of it, like in builtin/branch.c, where we want to print advice to the terminal unconditionally.
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. I think that is a good candidate to be replaced with advice_if_enabled().
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.
You could imagine something like this:
if (advice(ADVICE_SET_UPSTREAM_FAILURE, _(upstream_advice))) {
error(_(upstream_missing), start_name);
exit(1);
} else {
die(_(upstream_missing), start_name);
}(where this advice() takes the place of advice_if_enabled(), and has the dynamic read-advice.*-if-we-haven't-already behavior). But that switches the order of the output, which may or may not be desirable. I haven't looked further to see if there are more tricky cases like this.
> Probably the inverse would be a good approach, adding a
> advice.default=false to turn them all off by default, and then BUG() if
> we ever end up emitting advice anywhere (except if other specific config
> told us to enable it).
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.
Thanks, Taylor
Ævar Arnfjörð Bjarmason
