Junio C Hamano <gitster@xxxxxxxxx> writes:
> The use of strintern() comes originally from 3df8fd62 ...
> ..., so they may
> know how safe the change on the config side would be (I still do
> not understand why you'd want to do this in the first place, though,
> especially if you are protecting the callsites with mutex).
The risks of turning code that uses strintern() to use strdup() are
* you will leak the allocated string unless you explicitly free the
string you now own.
* you may consume too much memory if you are creating too many
copies of the same string (e.g. if you need filename for each
line in a file in an application, the memory consumption can
become 1000-fold).
* the code may be taking advantage of the fact that two such
strings can be compared for (in)equality simply by comparing
their addresses, which you would need to adjust to use !strcmp()
and the like.
I just checked to make sure that the last one is not the case for
our codebase, and you said you didn't see the second one is the case,
so the change may be a safe one to make.
One more thing. Do not use strdup() without checking its return
value for failure. It would be an easy fix to use xstrdup() instead.
Thanks.
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..d7f73d8745 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
>> enum config_scope scope,
>> struct key_value_info *out)
>> {
>> - out->filename = strintern(cs->name);
>> + out->filename = strdup(cs->name);
>> out->origin_type = cs->origin_type;
>> out->linenr = cs->linenr;
>> out->scope = scope;
>> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>>
>> strbuf_release(&top->value);
>> strbuf_release(&top->var);
>> + free(kvi.filename);
>>
>> return ret;
>> }
