Jeff King <peff@xxxxxxxx> writes: > On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote: > >> > 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. > > Yeah. Possibly the two should be squashed. I was trying to make this > patch a little less long/confusing, but maybe breaking things up just > posed new questions. :) No squashing is needed. It's just that [02/11] could go in either direction and the reader was held in suspense until [03/11] that picked one direction to go ;-). > Right. I had originally written it that way, since that would be the > mechanical conversion. But since there was already cleanup at the bottom > of the function, it felt more natural to shuffle it there. Which is > correct as long as there are no other references to buf nor early > returns. You can't see that from the context, but it is true in this > case. Yeah, either way it is correct, and I think the "cleanup at the end, where the single label is there for any new error code paths to jump to" pattern is a good approach going forward. Looking good.
