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

On Mon, May 2, 2016 at 11:18 AM, David Turner [off-list ref] wrote:

On Fri, 2016-04-29 at 16:34 -0700, Stefan Beller wrote:

quoted

+             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:

quoted

+                     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.

quoted

+     struct string_list list = STRING_LIST_INIT_NODUP;

Maybe move the scope of list into the while loop below?

quoted

      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