Re: [PATCH 4/5] grep: introduce new config option to include untracked files

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2024, 14:32 UTC
Message-ID: <xmqqil1is35i.fsf@gitster.g>
In-Reply-To: <923c5f84c44a195d6ba08503e28851af@manjaro.org>

Dragan Simic <dsimic@manjaro.org> writes:

Show 9 quoted lines

>>> +	if (use_index && !cached)
>>> +		git_config_get_bool("grep.includeuntracked", &untracked);
>> Can this ever return an error?  E.g.
>> 	[grep] includeuntracked = "not really"
>
> After a brief inspection of the code in cache.c, git_config_get_bool()
> always returns either 0 or 1, so we should be fine.  Thus, any
> strangeness in a configuration file would end up not enabling
> this option.

If that were the case, then it is not "fine".

When the user triggered an operation which *requires* us to parse and interpret the meaning of an entry in their configuration file correctly in order to carry it out, and if that entry has a value that we do not consider valid, we should notice and complain, before saying "Nah, this I do not understand, so I'll do one of the two things I would have done if the value were understandable and would not tell the user which one I did".

What makes it fine int his case is that git_config_get_bool() dies when the given value is not a Boolean ;-). The returned value from the function can be used to tell if the variable does not exist and the caller should decide to stuff some default value to &untracked but in this case you do not need to.