Jeff King <peff@xxxxxxxx> 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.
