On Tue, Feb 25, 2020 at 09:40:28AM -0800, Junio C Hamano wrote:
> "Heba Waly via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
