Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 17, 2024, 17:42 UTC
Message-ID: <xmqqv827a2ke.fsf@gitster.g>
In-Reply-To: <20240616045937.GB17750@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> 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 ;-).

Show 6 quoted lines

> 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.