| Message ID | 20240614102616.GB222445@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | aa0595fbd635bc2d16a4921244cf2bc94ca7761e |
| Headers | show |
| Series | allow overriding remote.*.url | expand |
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; > } > } > }
diff --git a/remote.c b/remote.c index dcb5492c85..fd9d58f820 100644 --- a/remote.c +++ b/remote.c @@ -35,7 +35,7 @@ static int valid_remote(const struct remote *remote) return (!!remote->url) || (!!remote->foreign_vcs); } -static const char *alias_url(const char *url, struct rewrites *r) +static char *alias_url(const char *url, struct rewrites *r) { int i, j; struct counted_string *longest; @@ -56,7 +56,7 @@ static const char *alias_url(const char *url, struct rewrites *r) } } if (!longest) - return url; + return NULL; return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len); } @@ -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); } 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); } @@ -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; } 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; } } }
The alias_url() function may return either a newly allocated string (which the caller must take ownership of), or the original const "url" parameter that was passed in. This often works OK because callers are generally passing in a "url" that they expect to retain ownership of anyway. So whether we got back the original or a new string, we're always interested in storing it forever. But I suspect there are some possible leaks here (e.g., add_url_alias() may end up discarding the original "url"). Whether there are active leaks or not, this is a confusing setup that makes further refactoring of memory ownership harder. 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. Signed-off-by: Jeff King <peff@peff.net> --- Just to be clear, I think there's probably still a leak in add_url_alias() even after this patch. It's just a step on the way to fixing. remote.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)