Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list

[Stefan Beller <sbeller@google.com>]

On Mon, May 2, 2016 at 11:18 AM, David Turner <dturner@twopensource.com> wrote:
> On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:
>> +             if (skip_prefix(item->string, "symref", &val)) {
>> +                     if (!val)
>> +                             continue;
>
> This if should never happen (skip_prefix returns 0 in that case).  You
> probably meant !*val -- but:
>
>> +                     val++; /* skip the = */
>
> I think you should instead skip_prefix "symref=" because:
> (a) it saves some code.
> (b) it allows for capabilities like symref_foo to later be added.
>
>> +     struct string_list list = STRING_LIST_INIT_NODUP;
>
> Maybe move the scope of list into the while loop below?
>
>>       char *line = packet_read_line(0, NULL);
>>       while (line) {
>> -             parse_features(line);
>> +             string_list_append(&list, line);
>> +             parse_features(&list);
>> +             string_list_clear(&list, 1);
>>               line = packet_read_line(0, NULL);
>
> This is a bit convoluted in the one-feature-per-line case, but I guess
> I understand that for the sake of generality it's useful.

Thanks for the review,
Stefan