{"thread":{"id":"57330","subject":"BUG: Various \"advice.*\" config doesn't work","startedAt":"2022-01-28T10:15:52Z","lastAt":"2022-01-31T17:46:36Z","messageCount":5,"participants":["Ævar Arnfjörð Bjarmason","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"447182","messageId":"220128.867dakcgj0.gmgdl@evledraar.gmail.com","threadId":"57330","inReplyTo":null,"subject":"BUG: Various \"advice.*\" config doesn't work","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-01-28T09:33:29Z","receivedAt":"2022-01-28T10:15:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"$subject, which I started looking at because in 4f3e57ef13d\n(submodule--helper: advise on fatal alternate error, 2019-12-02) the\nscaffolding was set up to read config, but nothing actually did so. So\n\"advice.submoduleAlternateErrorStrategyDie=false\" has never worked. That\ncan be tested with:\n\ndiff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\nindex a3892f494b6..8918be9ef5c 100755\n--- a/t/t7408-submodule-reference.sh\n+++ b/t/t7408-submodule-reference.sh\n@@ -193,7 +193,7 @@ test_expect_success 'missing nested submodule alternate fails clone and submodul\n \t\tcd supersuper-clone &&\n \t\tcheck_that_two_of_three_alternates_are_used &&\n \t\t# update of the submodule fails\n-\t\ttest_must_fail git submodule update --init --recursive\n+\t\tgit -c advice.submoduleAlternateErrorStrategyDie=false submodule update --init --recursive\n \t)\n '\n \nMore generally, the advice API should be made to panic in\nadvice_if_enabled() if nothing has read the advice config, which would\nturns up a lot of other such issues. I can't think of an easy way to\ncheck for that. We could add a BUG() on:\n\n    if (!the_repository->config && !the_repository->config->hash_initialized)\n\nIn advice_enabled(), but that wouldn't catch things that have read the\nconfig, but which aren't actually using it to check the advice config.\n\nProbably the inverse would be a good approach, adding a\nadvice.default=false to turn them all off by default, and then BUG() if\nwe ever end up emitting advice anywhere (except if other specific config\ntold us to enable it).\n"},{"id":"447340","messageId":"YfgLeVw0rrk7Q5/+@nand.local","threadId":"57330","inReplyTo":"220128.867dakcgj0.gmgdl@evledraar.gmail.com","subject":"Re: BUG: Various \"advice.*\" config doesn't work","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-31T16:16:57Z","receivedAt":"2022-01-31T16:17:55Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jan 28, 2022 at 10:33:29AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> $subject, which I started looking at because in 4f3e57ef13d\n> (submodule--helper: advise on fatal alternate error, 2019-12-02) the\n> scaffolding was set up to read config, but nothing actually did so. So\n> \"advice.submoduleAlternateErrorStrategyDie=false\" has never worked. That\n> can be tested with:\n>\n> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh\n> index a3892f494b6..8918be9ef5c 100755\n> --- a/t/t7408-submodule-reference.sh\n> +++ b/t/t7408-submodule-reference.sh\n> @@ -193,7 +193,7 @@ test_expect_success 'missing nested submodule alternate fails clone and submodul\n>  \t\tcd supersuper-clone &&\n>  \t\tcheck_that_two_of_three_alternates_are_used &&\n>  \t\t# update of the submodule fails\n> -\t\ttest_must_fail git submodule update --init --recursive\n> +\t\tgit -c advice.submoduleAlternateErrorStrategyDie=false submodule update --init --recursive\n>  \t)\n>  '\n>\n> More generally, the advice API should be made to panic in\n> advice_if_enabled() if nothing has read the advice config, which would\n> turns up a lot of other such issues. I can't think of an easy way to\n> check for that. We could add a BUG() on:\n>\n>     if (!the_repository->config && !the_repository->config->hash_initialized)\n\nI haven't spent much time looking in this area, but isn't the only thing\nthat reads advice.* config the `git_default_advice_config()` callback?\n\nIf so, we could check whether or not it has been called (being careful\nto handle the case where we called git_config(), but had an empty\nconfiguration, so the callback itself was never run). If it hasn't, then\nwe could BUG(), though I'd probably err on the side of just reading it\nright before calling advise().\n\nIn general, I wonder if making advise() public is a good idea. I think\nthere are good uses of it, like in builtin/branch.c, where we want to\nprint advice to the terminal unconditionally.\n\nBut having something like (in builtin/add.c:add_files()):\n\n    if (advice_enabled(ADVICE_ADD_IGNORED_FILES))\n        advise(_(\"...\"));\n\nfeels like it opens the door to call advise() by default if we happened\nto forget to read the configuration. I think that is a good candidate to\nbe replaced with advice_if_enabled().\n\nI'm not sure if that is true in general, though. Take a look at this\nexample (from branch.c:create_branch()):\n\n    if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {\n        error(_(upstream_missing), start_name);\n        advise(_(upstream_advice));\n        exit(1);\n    }\n    die(_(upstream_missing), start_name);\n\nThis also makes it possible to call advise() when we shouldn't have. But\nhow should we rewrite this code? Wanting to either error() (and then\ncall exit(1)) or die() based on whether or not we're going to print the\nadvice makes it tricky.\n\nYou could imagine something like this:\n\n    if (advice(ADVICE_SET_UPSTREAM_FAILURE, _(upstream_advice))) {\n      error(_(upstream_missing), start_name);\n      exit(1);\n    } else {\n      die(_(upstream_missing), start_name);\n    }\n\n(where this advice() takes the place of advice_if_enabled(), and has the\ndynamic read-advice.*-if-we-haven't-already behavior). But that switches\nthe order of the output, which may or may not be desirable. I haven't\nlooked further to see if there are more tricky cases like this.\n\n> Probably the inverse would be a good approach, adding a\n> advice.default=false to turn them all off by default, and then BUG() if\n> we ever end up emitting advice anywhere (except if other specific config\n> told us to enable it).\n\nMaybe, though I still think BUG() is a bit extreme, and we could\naccomplish the same by having the advice API just read the config if it\nhasn't done so already before emitting advice.\n\nThanks,\nTaylor\n"},{"id":"447346","messageId":"xmqq1r0nke71.fsf@gitster.g","threadId":"57330","inReplyTo":"YfgLeVw0rrk7Q5/+@nand.local","subject":"Re: BUG: Various \"advice.*\" config doesn't work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T17:28:02Z","receivedAt":"2022-01-31T17:28:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> But having something like (in builtin/add.c:add_files()):\n>\n>     if (advice_enabled(ADVICE_ADD_IGNORED_FILES))\n>         advise(_(\"...\"));\n>\n> feels like it opens the door to call advise() by default if we happened\n> to forget to read the configuration.\n\nTrue.\n\n> I think that is a good candidate to\n> be replaced with advice_if_enabled().\n\nMeaning advice_enabled() will lazily load the configuration?  If so,\nthen what you saw in builtin/add.c::add_files() would automatically\nbecome just as safe as advice_if_enabled(), no?\n\n> I'm not sure if that is true in general, though. Take a look at this\n> example (from branch.c:create_branch()):\n>\n>     if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {\n>         error(_(upstream_missing), start_name);\n>         advise(_(upstream_advice));\n>         exit(1);\n>     }\n>     die(_(upstream_missing), start_name);\n>\n> This also makes it possible to call advise() when we shouldn't have. But\n> how should we rewrite this code? Wanting to either error() (and then\n> call exit(1)) or die() based on whether or not we're going to print the\n> advice makes it tricky.\n\nI am puzzled why you think the above \"check, do things, give a\npiece of advice, and do even more things\" needs to be rewritten.\n\nEverything you are showing above becomes a problem only when\nadvice_enabled() does not work reliably, due to a bug that fails to\nread the configuration.\n\n> Maybe, though I still think BUG() is a bit extreme, and we could\n> accomplish the same by having the advice API just read the config if it\n> hasn't done so already before emitting advice.\n\nCalling things like git_config(git_default_config) with side-effects\non other global variables are definitely a no-no, but as long as it\nreacts to configuration variables only under advice.* namespace,\nthat might be OK.\n\nThanks.\n"},{"id":"447348","messageId":"YfgdsnfQ05ywre8l@nand.local","threadId":"57330","inReplyTo":"xmqq1r0nke71.fsf@gitster.g","subject":"Re: BUG: Various \"advice.*\" config doesn't work","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-31T17:34:42Z","receivedAt":"2022-01-31T17:35:21Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jan 31, 2022 at 09:28:02AM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > But having something like (in builtin/add.c:add_files()):\n> >\n> >     if (advice_enabled(ADVICE_ADD_IGNORED_FILES))\n> >         advise(_(\"...\"));\n> >\n> > feels like it opens the door to call advise() by default if we happened\n> > to forget to read the configuration.\n>\n> True.\n>\n> > I think that is a good candidate to\n> > be replaced with advice_if_enabled().\n>\n> Meaning advice_enabled() will lazily load the configuration?  If so,\n> then what you saw in builtin/add.c::add_files() would automatically\n> become just as safe as advice_if_enabled(), no?\n\nThe change I was wondering aloud about was having advice_enabled()\nlazily load the configuration.\n\nSo what is written in add_files() above would become safe (if it wasn't\nalready), and could easily be replaced with advise_if_enabled().\n\n> > I'm not sure if that is true in general, though. Take a look at this\n> > example (from branch.c:create_branch()):\n> >\n> >     if (advice_enabled(ADVICE_SET_UPSTREAM_FAILURE)) {\n> >         error(_(upstream_missing), start_name);\n> >         advise(_(upstream_advice));\n> >         exit(1);\n> >     }\n> >     die(_(upstream_missing), start_name);\n> >\n> > This also makes it possible to call advise() when we shouldn't have. But\n> > how should we rewrite this code? Wanting to either error() (and then\n> > call exit(1)) or die() based on whether or not we're going to print the\n> > advice makes it tricky.\n>\n> I am puzzled why you think the above \"check, do things, give a\n> piece of advice, and do even more things\" needs to be rewritten.\n>\n> Everything you are showing above becomes a problem only when\n> advice_enabled() does not work reliably, due to a bug that fails to\n> read the configuration.\n\nWhat I'm more or less trying to point out is that an unguarded advise()\nfunction defeats the purpose of the advice API, which should exist to\navoid mistakes like these (where advice is printed to the user when they\nhave already opted out of that advice).\n\nI was thinking that it would be nice to have advise() take an\nadvice_type enum and have it behave like advise_if_enabled(). But there\nare spots that you really do want to print advice unconditionally. And\nthat spot in create_branch() is one of those where it isn't clear that\nthe change I'm proposing works.\n\n(BTW, I would definitely disagree that I'm saying anything \"needs\" to be\nrewritten here. This is all just thinking aloud about what the advice\nAPI can and should help callers with.)\n\n> > Maybe, though I still think BUG() is a bit extreme, and we could\n> > accomplish the same by having the advice API just read the config if it\n> > hasn't done so already before emitting advice.\n>\n> Calling things like git_config(git_default_config) with side-effects\n> on other global variables are definitely a no-no, but as long as it\n> reacts to configuration variables only under advice.* namespace,\n> that might be OK.\n\nYes, I definitely agree we should absolutely not be calling\ngit_default_config() as part of \"lazily load the advice.*\nconfiguration\".\n\n> Thanks.\n\nThanks,\nTaylor\n"},{"id":"447350","messageId":"xmqqv8xziyrr.fsf@gitster.g","threadId":"57330","inReplyTo":"YfgdsnfQ05ywre8l@nand.local","subject":"Re: BUG: Various \"advice.*\" config doesn't work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T17:46:32Z","receivedAt":"2022-01-31T17:46:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> What I'm more or less trying to point out is that an unguarded advise()\n> function defeats the purpose of the advice API, which should exist to\n> avoid mistakes like these (where advice is printed to the user when they\n> have already opted out of that advice).\n\nadvise() falls into the same category as die(), warning(), and\nerror().  It is a way to consistently format (and with a better\ndesign to line-wrap, than its older brethren) messages.\n\nWe do not have \"die_if(condition, format, ...)\", but if there were\nsuch a thing, that would be a good analogy to advice_if_enabled().\n\nAnd a short-hand of die_if() would not make die() any unnecessary.\n\n"}]}