Re: [PATCH] config.c: change the function signature of `git_config_string()`

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jul 22, 2014, 11:41 UTC
Message-ID: <53CE4DCE.1010908@gmail.com>
In-Reply-To: <20140722110720.GA386@peff.net>

On 7/22/2014 4:37 PM, Jeff King wrote:

Show 21 quoted lines

> On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:
> 
>> `git_config_string()` output parameter `dest` is declared as a const
>> which is unnecessary as the caller of the function is given a strduped
>> string which can be modified without causing any harm.
>>
>> Thus, remove the const from the function signature.
> 
> You are correct that it is unnecessary. However, this patch alone is not
> sufficient because of the way const-ness in C works. If I have:
> 
>   static const char *some_global;
> 
> then with your patch, calling:
> 
>   git_config_string(&some_global, var, value);
> 
> will complain that we are passing a pointer to "const char *", not a
> pointer to "char *". And indeed, compiling with your patch introduces a
> ton of compiler warnings.
>

I had also thought that the compiler would raise lot of warnings but it didn't on the first compile. Now I checked again and now it complains a lot, maybe it because I was tinkering with my config.mak, dunno.

Show 20 quoted lines

> We would have to convert each of the variables we pass to it to:
> 
>   static char *some_global;
> 
> That's not so bad, but:
> 
>   static char *some_global = "some_default_value";
> 
> is wrong. Such a global sometimes points to const storage (i.e.,
> initially), and sometimes to allocated storage (if it was loaded from
> config). We simply keep the latter as a const pointer (since we would
> not bother to free it at the end of the program anyway), and that
> decision influences git_config_string, which is just a helper for
> setting such variables anyway.
> 
> So I would not mind lifting this unnecessary restriction on
> git_config_string, but I do not see a way to do it without making the
> rest of the code much uglier (and I do not see a particular advantage in
> modifying git_config_string here that would make it worth the trouble).
>

Yes, you are right. This patch is the conclusion of discussion in [1]. I used the same function signature as git_config_string for git_config_get_string() which lead to some ugly casts like

+git_config_get_string("imap.folder", (const char**)&imap_folder);

in imap-send.c patch and others. What should we do about such cases, I used either an intermediate variable or casts but Junio commented that it would be better if the dest parameter was a non-const and that it was a weakness of the config-set API that demanded the dest to be a const pointer.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/253948/