Junio C Hamano <gitster@xxxxxxxxx> writes:
> Linus Arver <linusa@xxxxxxxxxx> 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)).
