Re: [PATCH/RFC] alias.c: Replace git_config with git_config_get_string

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jun 17, 2014, 18:51 UTC
Message-ID: <53A08E37.1040104@gmail.com>
In-Reply-To: <20140617054357.GC29957@sigill.intra.peff.net>

On 06/16/2014 10:43 PM, Jeff King wrote:

Show 10 quoted lines

> 
>> +	v = git_config_get_string(alias_key);
>> +	if (!v)
>> +		config_error_nonbool(alias_key);
> 
> What does a NULL output from git_config_get_string mean? I think with
> the current code, it means "no such key was found".  In which case, you
> should be returning NULL here (there is no such alias), not complaining
> with config_error_nonbool.
>

Yes, you surmised correctly. I totally skipped the fact that git_config() can return null for values indicating a boolean value. I will correct it in the next patch.

> Again, this is going to depend on your strategy for storing booleans
> that I mentioned elsewhere.

I have read your other two replies related to it. I suggest the following approach for git_config_get_string(), it will return,

1. Return null if no value was found for the entered key.

2. Empty string (""), returned for NULL values denoting boolean true in some cases.
   I think it would be much better than converting NULL to "true" or something else
   internally in the function.
   We can easily handle such cases as the above with a strcmp like,

+ v = git_config_get_string(alias_key); + if (!strcmp(v, "")) + config_error_nonbool(alias_key);

What do you think about this approach?

Thanks for the suggestion, I was pulling my hair out due this bug for last two days.

Cheers, Tanay Abhra.