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

[Tanay Abhra <tanayabh@gmail.com>]

On 6/25/2014 1:36 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra <tanayabh@gmail.com> wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  notes.c | 20 ++++++--------------
>>  1 file changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/notes.c b/notes.c
>> index 5fe691d..fc92eec 100644
>> --- a/notes.c
>> +++ b/notes.c
>> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
>>         free(globs_copy);
>>  }
>>
>> -static int notes_display_config(const char *k, const char *v, void *cb)
>> -{
>> -       int *load_refs = cb;
>> -
>> -       if (*load_refs && !strcmp(k, "notes.displayref")) {
>> -               if (!v)
>> -                       config_error_nonbool(k);
>> -               string_list_add_refs_by_glob(&display_notes_refs, v);
>> -       }
>> -
>> -       return 0;
>> -}
>> -
>>  const char *default_notes_ref(void)
>>  {
>>         const char *notes_ref = NULL;
>> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
>>  void init_display_notes(struct display_notes_opt *opt)
>>  {
>>         char *display_ref_env;
>> +       const char *value;
>>         int load_config_refs = 0;
>>         display_notes_refs.strdup_strings = 1;
>>
>> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
>>                         load_config_refs = 1;
>>         }
>>
>> -       git_config(notes_display_config, &load_config_refs);
>> +       if (load_config_refs && !git_config_get_string("notes.displayref", &value)) {
>> +               if (!value)
>> +                       config_error_nonbool("notes.displayref");
>> +               string_list_add_refs_by_glob(&display_notes_refs, value);
> 
> Although you correctly diagnose a NULL 'value', you then invoke
> string_list_add_refs_by_glob() with that NULL, which will result in a
> crash.
> 
> This is not a new error. It dates back to 894a9d33 (Support showing
> notes from more than one notes tree; 2010-03-12), but your rewrite
> should not retain the brokenness. Whether you fix it in this patch or
> a lead-in fix-up patch, the fix deserves mention in the commit
> message.

Done. Thanks.

>> +       }
>>
>>         if (opt) {
>>                 struct string_list_item *item;
>> --
>> 1.9.0.GIT
>