On Wed, Feb 26, 2020 at 11:02 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Heba Waly <heba.waly@xxxxxxxxx> 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 ;-)? > The reason I had to add the list of keys wasn't the enums, but because it's needed by list_config_advices() which returns all the advice config variables names. This is used when the user runs `git help --config`. And as a result, I added the enum to utilize it in accessing the list and avoid hard coded strings in functions calls. > 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'm not against this approach as well, but as I mentioned above, we need a list of keys to be returned by list_config_advices(), that's why defining the constant strings will not be sufficient in our case. Thanks, Heba
