Re: [PATCH v2 6/7] gc: don't run "reflog expire" when keeping reflogs

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 18, 2019, 06:04 UTC
Message-ID: <xmqq1s344t11.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <877ed0bfl9.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 13 quoted lines

>>> +       if (gc_config_is_timestamp_never("gc.reflogexpire") &&
>>> +           gc_config_is_timestamp_never("gc.reflogexpireunreachable"))
>>
>> Nit. configset api normalizes the key internally, so we can safely
>> write gc.reflogExpireUnreachable here, which is a bit easier to read.
>
> I didn't know that, do we want to do that?
>
> I'd think that as a matter of coding style always sticking to lower case
> in the C code made more sense, because e.g. you might #define a config
> key, and then use it both in git_config_*() as well as via strcmp() in
> some callback. Mixing the two would lead to confusion and possible bugs
> as we'd refactor the former of those patterns to the latter.

When parsing end-user supplied value in a "const char *var", with

    #define REU "gc.reflogexpireunreacahble"

strcmp(var, REU) is wrong. You must use strcasecmp(var, REU) because the end-user supplied value may be in camelCase.

And once the code uses strcasecmp(var, REU), using camelCase

    #define REU "gc.reflogExpireUnreacahble"

to compare against would not break it, either. The only difference is that being able to see the word boundary may make it easier to read.

So even when git_config-*() and str*cmp() are used in a mixed way, I do not think it is a good justification to make the string constant all lowercase (assuming that camel is easier than all lowercase to read, that is).

Parsing three-level names is tricky. Without breaking it down into components first, you cannot use strcmp nor strcasecmp; comparing the whole variable name with either function is wrong.