Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string

From: Karsten Blees <karsten.blees@gmail.com>
Date: Jun 27, 2014, 16:57 UTC
Message-ID: <53ADA26E.8030801@gmail.com>
In-Reply-To: <vpqr42afty5.fsf@anie.imag.fr>

Am 27.06.2014 13:55, schrieb Matthieu Moy:

Show 12 quoted lines

> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> If for some reason a config string is accessed after config_cache_free()
>> (which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
>> will continue to run with some invalid configuration). This is IMO much worse
>> than failing with segfault.
> 
> I disagree. In most case, continuing to use the old config value is the
> right thing.
> 
> First, config_cache_free() is called whenever _any_ configuration
> variable is set.

This is illogical. An API should do the right thing, and if the right thing is to keep the old values, as you pointed out, then setting a config value shouldn't invalidate the cache (at least not automatically).

I can think of only git-clone and git-init that may want to refresh the global config cache, otherwise, config_cache_free() should probably _never_ be called. Least of all as a side effect of git_config_set_multivar_in_file(), which is used to write all kinds of unrelated files that just happen to have config-file format (e.g. '.gitmodules', '.git/sequencer/opts').

> So, setting "core.foo" also invalidates "core.bar" in
> the cache, while a user of "core.bar" could continue working with the
> old, unchanged value.

But a user of an xstrdup()ed copy of "core.foo" would also continue to work with the old value. So if everyone keeps copies of config strings, invalidating the cache when setting a value has no effect. We could just as well not invalidate the cache and not xstrdup() strings.

Perhaps we should see this as an opportunity to get rid of all the memory leaks in current config code (each string value defined in system / global config and overridden locally will currently be leaked with the 'standard' xstrdup() pattern).