Re: [PATCH v5 2/3] advice: revamp advise API

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Feb 25, 2020 at 09:40:28AM -0800, Junio C Hamano wrote:
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > +static int get_config_value(enum advice_type type)
> > +{
> > +	int value = 1;
> > +	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
> > +
> > +	git_config_get_bool(key, &value);
> > +	free(key);
> > +	return value;
> > +}
> 
> So, in this hypothetical but quite realistic example:
> 
> 	if (advice_enabled(ADVICE_FOO)) {
> 		char *foo = expensive_preparation();
> 		advice_if_enabled(ADVICE_FOO, "use of %s is discouraged", foo);
> 	}
> 
> we end up formulating the "advice.*" key twice and ask git_config_get_bool()
> about the same key twice?
> 
> > +void advise_if_enabled(enum advice_type type, const char *advice, ...)
> > +{
> > +	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
> > +	va_list params;
> > +
> > +	if (!advice_enabled(type))
> > +		return;
> 
> Oh, no, make the number of calls to xstrfmr() three times, not
> twice, as I said in the previous example.
> 
> I wonder if it would make the implementation better to do these:
> 
>  - Rename advice_config_keys[] to advice_setting[] that does not
>    imply it is only about the keys;
> 
>  - This table will know, for each enum advice_type, which
>    configuration variable enables it, *and* if it is enabled.
> 
> i.e.
> 
>         static struct {
>                 const char *config_key;
>                 int disabled;
>         } advice_setting[] = {
>                 [ADVICE_ADD_EMBEDED_REPO] = { "addEmbeddedRepo" },
>                 [ADVICE_AM_WORK_DIR]      = { "amWorkDir" },
>                 ...
>                 [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
>         };
> 
> 
> Side Note: you have AMWORKDIR that is unreadable.  If the config
>            name uses camelCase by convention, the UPCASED_WORDS
>            should be separated with underscore at the same word
>            boundary.
> 
> Then, upon the first call to advice_enabled(), call git_config()
> with a callback like
> 
> 	static int populate_advice_settings(const char *var, const char *value, void *cb)
> 	{
> 		int advice_type;
> 		const char *name;
> 
> 		if (!skip_prefix(var, "advice.", &name))
> 			return 0;
> 		advice_type = find_advice_type_by_name(advice_setting, name);
> 		if (advice_type < 0)
> 			return 0; /* unknown advice.* variable */
> 		/* advice.foo=false means advice.foo is disabled */
> 		advice_setting[advice_type].disabled = !git_config_bool(var, value);
> 	}
> 
> only once.  Your get_config_value() would then become a mere lookup
> in advice_setting[] array, e.g.
> 
> 	int advice_enabled(unsigned advice_type)
> 	{
> 		static int initialized;
> 
> 		if (!initialized) {
> 			initialized = 1;
> 			git_config(populate_advice_settings, NULL);
> 		}
> 		if (ARRAY_SIZE(advice_setting) <= advice_type)
> 			BUG("OOB advice type requested???");
> 		return !advice_setting[advice_type].disabled;
> 	}
> 
> with your "push-update-rejected has two names" twist added.

I'm a little confused about the need to cache the result of
git_config_get_bool() - isn't that a lookup from a hashmap which is
already populated at setup time, and therefore inexpensive? I would
think the only expensive part here is the xstrfmt() calls, which it
seems like would be easy to do away with by storing the fully-qualified
advice key in the array instead. What am I missing?

 - Emily