Re: [PATCH v2 5/6] trailer: rename _DEFAULT enums to _UNSPECIFIED

From: Linus Arver <linusa@google.com>
Date: Sep 22, 2023, 18:23 UTC
Message-ID: <owly8r8yt6cr.fsf@fine.c.googlers.com>
In-Reply-To: <xmqq1qf1la0q.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 28 quoted lines

> Linus Arver <linusa@google.com> writes:
>
>>> It gets tempting to initialize a variable to the default and arrange
>>> the rest of the system so that the variable set to the default
>>> triggers the default activity.  Such an obvious solution however
>>> cannot be used when (1) being left unspecified to use the default
>>> value and (2) explicitly set by the user to a value that happens to
>>> be the same as the default have to behave differently.  I am not
>>> sure if that applies to the trailers system, though.
>>>
>>> Thanks.
>>
>> I get the feeling that you wrote the "Such an obvious ... differently"
>> sentence after writing the last sentence in that paragraph, because when
>> you say
>>
>>     I am not
>>     sure if that applies to the trailers system, though.
>>
>> I read the "that" (emphasis added) in there as referring to the solution
>> described in the first sentence, and not the conditions (1) and (2) you
>> enumerated. IOW, you are OK with this patch.
>
> "that" refers to "the reason not to use such an obvious solution".
> I do not know if trailer subsystem wants to treat "left unspecified"
> and "set to the value that happens to be the same as the default" in
> a different way.  If it does want to do so, then I do not see a
> strong reason not to use the "obvious solution".

Currently we set the defaults (these take effect absent any configuration or CLI options) in trailer.c like this:

    static void ensure_configured(void)
    {
            if (configured)
                    return;

            /* Default config must be setup first */
            default_conf_info.where = WHERE_END;
            default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
            default_conf_info.if_missing = MISSING_ADD;
            git_config(git_trailer_default_config, NULL);
            git_config(git_trailer_config, NULL);
            configured = 1;
    }

So technically we already sort of do the "obvious solution". And then these get overriden by configuration (if any), and finally any CLI options that are passed in (e.g., "--where after"). The reason why I prefer the *_UNSPECIFIED style in this patch for these enums though is because of this (and other similar functions) in trailer.c:

    int trailer_set_where(enum trailer_where *item, const char *value)
    {
            if (!value)
                    *item = WHERE_DEFAULT;
            else if (!strcasecmp("after", value))
                    *item = WHERE_AFTER;
            else if (!strcasecmp("before", value))
                    *item = WHERE_BEFORE;
            else if (!strcasecmp("end", value))
                    *item = WHERE_END;
            else if (!strcasecmp("start", value))
                    *item = WHERE_START;
            else
                    return -1;
            return 0;
    }

and this function is used as a callback to the "--where" flag, such that the WHERE_DEFAULT gets chosen if "--no-where" is where. I prefer the WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use the default value WHERE_END in default_conf_info, or it could mean that we fall back to the configuration variables (where it could be something else)).

Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED

Re: [PATCH v2 5/6] trailer: rename _DEFAULT enums to _UNSPECIFIED