Karsten Blees [off-list ref] 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. 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.
Then, allowing the invalidation of a config variable at any time raises
a lot of tricky cases (if a command uses a configuration variable twice,
do we really want to implement and test the behavior for all combination
of old/new values for both usages?). More tricky cases usually means
more bugs on the user-side ...
When the code really want the freshest value, it's cheap to re-query the
config cache anyway.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Am 27.06.2014 13:55, schrieb Matthieu Moy:
Karsten Blees [off-list ref] writes:
quoted
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).