Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> diff --git a/remote.c b/remote.c
> index fd9d58f820..f7c846865f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r)
>  static void add_url(struct remote *remote, const char *url)
>  {
>  	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> -	remote->url[remote->url_nr++] = url;
> +	remote->url[remote->url_nr++] = xstrdup(url);
>  }
>  
>  static void add_pushurl(struct remote *remote, const char *pushurl)
>  {
>  	ALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);
> -	remote->pushurl[remote->pushurl_nr++] = pushurl;
> +	remote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);
>  }

OK.  This makes it easier to reason about why these elements are
freed in remote_clear().

>  static void add_pushurl_alias(struct remote_state *remote_state,
> @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
>  	char *alias = alias_url(url, &remote_state->rewrites_push);
>  	if (alias)
>  		add_pushurl(remote, alias);
> +	free(alias);
>  }

OK.  I wondered if we want to strdup(url) in my review on the
previous step, but now we are making the add_url() responsible
for making a copy, we instead do the opposite, i.e. free alias
that was allocated for us because we no longer need it.

>  static void add_url_alias(struct remote_state *remote_state,
> @@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state,
>  	char *alias = alias_url(url, &remote_state->rewrites);
>  	add_url(remote, alias ? alias : url);
>  	add_pushurl_alias(remote_state, remote, url);
> +	free(alias);
>  }

Likewise.

> @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,
>  	else
>  		frag = to_free = repo_default_branch_name(the_repository, 0);
>  
> -	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
> +	add_url_alias(remote_state, remote, buf.buf);

It is curious that you delay ...

> @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,
>  	refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag);
>  	remote->fetch_tags = 1; /* always auto-follow */
>  
> +	strbuf_release(&buf);
>  	free(to_free);
>  }

... strbuf_release() of the buf to the very end of the function.  

In the original, buf became invalid by doing strbuf_detach(), so we
could do strbuf_release() immediately after add_url_alias() returns
to us if we wanted to.

> @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,
>  	else if (!strcmp(subkey, "prunetags"))
>  		remote->prune_tags = git_config_bool(key, value);
>  	else if (!strcmp(subkey, "url")) {
> -		char *v;
> -		if (git_config_string(&v, key, value))
> -			return -1;
> -		add_url(remote, v);
> +		if (!value)
> +			return config_error_nonbool(key);
> +		add_url(remote, value);

OK.  config_string() does (1) check for "I exist hence I am true"
boolean, and (2) give us a duplicate of the value.  We do not want
the latter, so we do the former ourselves here.  The same story
repeats for pushurl below (ellided).

> @@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state)
>  		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
>  			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
>  						&remote_state->rewrites);
> -			if (alias)
> +			if (alias) {
> +				free((char *)remote_state->remotes[i]->pushurl[j]);
>  				remote_state->remotes[i]->pushurl[j] = alias;
> +			}
>  		}

OK, this is the replacement codepath we saw earlier.  Makes sense
for this and the .url[] side on the other hunk (it is curious that
this has .pushurl[] before .url[], which is the opposite order of
how everybody else deals with them, as pushurl came later).

Thanks.