From: Jeff King <peff@peff.net>
Date: Mon, 22 Jun 2009 21:32:04 GMT
Subject: Re: [PATCH] builtin-remote: (get_one_entry): use strbuf
Message-ID: <20090622213204.GB19364@coredump.intra.peff.net>
In-Reply-To: <1245659059-11852-1-git-send-email-bert.wesarg@googlemail.com>

On Mon, Jun 22, 2009 at 10:24:19AM +0200, Bert Wesarg wrote:

> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -1276,17 +1276,17 @@ static int update(int argc, const char **argv)
>  static int get_one_entry(struct remote *remote, void *priv)
>  {
>  	struct string_list *list = priv;
> +	struct strbuf url_buf = STRBUF_INIT;
> +	const char *url_str = NULL;
>  	const char **url;
>  	int i, url_nr;
> -	void **utilp;
>  
>  	if (remote->url_nr > 0) {
> -		utilp = &(string_list_append(remote->name, list)->util);
> -		*utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1);
> -		strcpy((char *) *utilp, remote->url[0]);
> -		strcat((char *) *utilp, " (fetch)");
> -	} else
> -		string_list_append(remote->name, list)->util = NULL;
> +		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
> +		url_str = strbuf_detach(&url_buf, NULL);
> +	}
> +	string_list_append(remote->name, list)->util = url_str;
> +

This causes const warnings due to the 'const' on url_str. One solution
is just s/const char/char/.

However, I think it is actually more readable to do away with url_str
entirely. The original if/else logic was much easier to follow than
realizing that url_str is initialized to NULL, and then never changed.
IOW:

  if (remote->url_nr > 0) {
          strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
          string_list_append(remote->name, list)->util =
                  strbuf_detach(&url_buf, NULL);
  }
  else
          string_list_append(remote->name, list)->util = NULL;

> @@ -1296,10 +1296,9 @@ static int get_one_entry(struct remote *remote, void *priv)
>  	}
>  	for (i = 0; i < url_nr; i++)
>  	{
> -		utilp = &(string_list_append(remote->name, list)->util);
> -		*utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1);
> -		strcpy((char *) *utilp, url[i]);
> -		strcat((char *) *utilp, " (push)");
> +		strbuf_addf(&url_buf, "%s (push)", url[i]);
> +		url_str = strbuf_detach(&url_buf, NULL);
> +		string_list_append(remote->name, list)->util = url_str;
>  	}

And then this one is re-using url_str for a totally unrelated thing,
but could just be:

  string_list_append(remote->name, list)->util =
          strbuf_detach(&url_buf, NULL);

But that is somewhat nit-picking. As long as the const-warning goes
away, I will be happy enough.

-Peff