Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 14, 2024, 17:04 UTC
- Message-ID
- <xmqq1q4zh2vh.fsf@gitster.g>
- In-Reply-To
- <20240614102722.GC222445@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> 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().
Show 7 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 6 quoted lines
> @@ -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 ...
Show 7 quoted lines
> @@ -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.
Show 11 quoted lines
> @@ -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).
Show 10 quoted lines
> @@ -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.