On Mon, May 2, 2016 at 11:18 AM, David Turner <dturner@xxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html