From: David Turner <dturner@twopensource.com>
Date: Mon, 02 May 2016 18:18:41 GMT
Subject: Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list
Message-ID: <1462213121.4123.58.camel@twopensource.com>
In-Reply-To: <1461972887-22100-5-git-send-email-sbeller@google.com>

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.