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

[Karsten Blees <karsten.blees@gmail.com>]

Am 27.06.2014 13:55, schrieb Matthieu Moy:
> 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).