Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

From: Ben Peart <peartben@gmail.com>
Date: Sep 14, 2018, 18:05 UTC
Message-ID: <80988727-085f-cdd9-e5bb-a90a55e97024@gmail.com>
In-Reply-To: <xmqq5zz8c84y.fsf@gitster-ct.c.googlers.com>

On 9/14/2018 1:15 PM, Junio C Hamano wrote:

Show 18 quoted lines

> Ben Peart <benpeart@microsoft.com> writes:
> 
>> diff --git a/config.c b/config.c
>> index 3461993f0a..3555c63f28 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
>>   int git_config_get_fsmonitor(void)
>>   {
>>   	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
>> -		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
>> +		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
> 
> Sorry for not noticing earlier, but unlike 4/4 that changed
> getenv(VAR) to git_env_bool(VAR, 0) "while at it", this leaves it to
> getenv(VAR), meaning "if it is set to any non-empty string, it is
> true".  Is there a reason for this discrepancy?
>

The difference here is that core.fsmonitor isn't a boolean value. It is a string to a command that is executed so it can't be moved over to get_env_bool().

Show 6 quoted lines

> I _think_ the renaming should be done without getting mixed with
> other changes like the git_env_bool() done in 4/4.  The idea to use
> git_env_bool() in stead of getenv() may be a good one, but then we
> should consistently do so when appropriate, and that would make a
> fine theme for another topic.
>