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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Apr 1, 2016 at 1:14 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:
>> From: Stefan Beller <sbeller@google.com>
>> Date: Thu, 31 Mar 2016 11:04:03 -0700
>> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
>>
>> This function asks for the value of a configuration and
>> after using the value does not have to retain ownership
>> of the value.  git_config_get_string_const() however is
>> a function to get a copy of the value, but we forget to
>> free it before we return.
>>
>> Because we only need to peek the value without retaining
>> a pointer to it, use git_config_get_value() to peek into
>> the value cached in the config API layer.
>>
>> As git_config_get_value() does not insist the value to be
>> a string, we'd need to do the "nonbool" check ourselves.

Nicer commit message.

> Unfortunately, I don't think this is quite right. In the original, we
> relied on git_config_get_string_const to notice a non-string value, at
> which point it would die:
>
>   $ git -c notes.mergeStrategy notes merge whatever
>   error: Missing value for 'notes.mergeStrategy'
>   fatal: unable to parse 'notes.mergeStrategy' from command-line config
>
> But in your patch:
>
>> +     if (!value)
>> +             return config_error_nonbool(key);
>
> We just return an error from git_config_get_notes_strategy(). If this
> were a callback to git_config(), that would be fine (as we would
> auto-die then in the caller), but it's not. It is called directly for a
> specific key. One of the callers treats a non-zero return as "we don't
> have that variable", and the other ignores the return value completely.
>
> So I think you'd want something more like:
>
>   if (!value) {
>         config_error_nonbool(key);
>         git_die_config(key);
>   }

Yep, I had noted the bit about having to die() manually when reviewing
the earlier patch, but it slipped from memory when composing the reply
to the current version of the patch.

> This is why I wondered if the minor "do not allocate" tweak was worth
> the trouble, when git_config_get_string() just handles this for us.

Which again suggests a new function which does this work but doesn't
copy the string (despite the already quite large set of similar
functions).