Re: [PATCHv2 1/4] notes: don't leak memory in git_config_get_notes_strategy

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2016, 21:07 UTC
Message-ID: <xmqqegarsmil.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cTeVgyNHa5uHUMDZ6ifxyjk2wvm=jGd9tP1+BuBKKVR+g@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 21 quoted lines

>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -744,13 +744,14 @@ static int merge_commit(struct notes_merge_options *o)
>>  static int git_config_get_notes_strategy(const char *key,
>>                                          enum notes_merge_strategy *strategy)
>>  {
>> -       const char *value;
>> +       char *value;
>>
>> -       if (git_config_get_string_const(key, &value))
>> +       if (git_config_get_string(key, &value))
>>                 return 1;
>>         if (parse_notes_merge_strategy(value, strategy))
>>                 git_die_config(key, "unknown notes merge strategy %s", value);
>>
>> +       free(value);
>>         return 0;
>>  }
>
> Hmm, I thought Peff's suggestion of using git_config_get_value() was
> accepted as superior since it avoids the allocation altogether, thus
> no need for free() and no leak.

I agree that this caller can avoid taking ownership of value by using git_config_get_value() and that would be a cleaner solution here.

This is a tangent, but am I the only one who finds that the naming of functions in config-get API is confusing? Just wondering if we should rename the ones that keeps the memory ownership to the config subsystem with s/get/peek/ or something.