Re: [PATCH 02/11] remote: refactor alias_url() memory ownership

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

Jeff King <peff@peff.net> writes:

> ... So instead of
> returning the original string, return NULL, forcing callers to decide
> what to do with it explicitly. We can then build further cleanups on top
> of that.

OK.

> @@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
>  static void add_pushurl_alias(struct remote_state *remote_state,
>  			      struct remote *remote, const char *url)
>  {
> -	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
> -	if (pushurl != url)
> -		add_pushurl(remote, pushurl);
> +	char *alias = alias_url(url, &remote_state->rewrites_push);
> +	if (alias)
> +		add_pushurl(remote, alias);

OK, that's an obviously equivalent rewrite.

>  static void add_url_alias(struct remote_state *remote_state,
>  			  struct remote *remote, const char *url)
>  {
> -	add_url(remote, alias_url(url, &remote_state->rewrites));
> +	char *alias = alias_url(url, &remote_state->rewrites);
> +	add_url(remote, alias ? alias : url);
>  	add_pushurl_alias(remote_state, remote, url);
>  }

This is also an obviously equivalent rewrite.

Looking at how remote_clear() deals with the .url[] and .pushurl[]
elements, add_url() makes the remote structure take ownership, which
is perfectly fine when we got a non-NULL alias back (i.e. it is a
new piece of string allocated just for us).  Depending on who owns
the incoming url parameter, we might need strdup(url) here, but
since we haven't heard crashes due to freeing remote->url[] elements
that should not be freed, presumably url is a piece memory the
caller is giving us the ownership?  In any case, I imagine that
untangling that ownership mess is left to the later steps of the
series.

> @@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state)
>  		if (!remote_state->remotes[i])
>  			continue;
>  		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
> -			remote_state->remotes[i]->pushurl[j] =
> -				alias_url(remote_state->remotes[i]->pushurl[j],
> -					  &remote_state->rewrites);
> +			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
> +						&remote_state->rewrites);
> +			if (alias)
> +				remote_state->remotes[i]->pushurl[j] = alias;

Does this change behaviour?  No.  We used to (1) grab the current
value, (2) replace it if it is an alias, or (3) otherwise replace it
with itself.  We just do not do the obvious noop anymore.

The story is the same with the last remaing hunk of this patch.
Looking good.

Thanks.

>  		}
>  		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
>  		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
> +			char *alias;
>  			if (add_pushurl_aliases)
>  				add_pushurl_alias(
>  					remote_state, remote_state->remotes[i],
>  					remote_state->remotes[i]->url[j]);
> -			remote_state->remotes[i]->url[j] =
> -				alias_url(remote_state->remotes[i]->url[j],
> +			alias = alias_url(remote_state->remotes[i]->url[j],
>  					  &remote_state->rewrites);
> +			if (alias)
> +				remote_state->remotes[i]->url[j] = alias;
>  		}
>  	}
>  }