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

[Junio C Hamano <gitster@pobox.com>]

Heba Waly <heba.waly@gmail.com> writes:

> I wasn't very happy about having to keep the list of config keys in
> memory, but that was a good enough solution for now.

If you force your programmers to specify the advice_type as a small
integer, and the setting is stored in the config as string keys,
somebody MUST have a table to convert from one to the other.  So I
am not sure if it is even sensible to feel unhappy about having to
have a list in the first place.  Are we looking for some kind of
miracles ;-)?

On the other hand, it does bother my sense of aesthetics a lot it
our code forces our programmers to give a small integer to us, only
so that we convert that integer to a string and use the string to
look up a value in a hashtable, every time the program wants a
lookup.  Performance-wise, that's not a huge downside.  It just rubs
my sense of code hygiene the wrong way.

Especially when the primary way for our programmers to specify which
advice they are talking about is by passing an integer, and if we
need to have a table indexed by that integer in the program anyway.

We could instead do something like:

    /* advice.h */
    #ifndef _ADVICE_H_
    #define _ADVICE_H_ 1
    extern const char ADVICE_ADD_EMBEDDED_REPO[];
    extern const char ADVICE_AM_WORK_DIR[];
    ...
    #endif

    /* advice.c */
    const char ADVICE_ADD_EMBEDDED_REPO[] = "advice.addEmbeddedRepo";
    const char ADVICE_ADD_AM_WORK_DIR[] = "advice.amWorkDir";
    ...

and the callers can still do

    advise_if_enabled(ADVICE_NESTED_TAG,
		      _(message_advice_nested_tag), tag, object_ref);

with the benefit of compiler catching a silly typo, without having
to have any "enum-to-string" table while letting the config API
layer do any caching transparently.  As these calls will never be
placed in a performance critical codepath, that might be more
appropriate.  

I dunno.