Re: [PATCH 3/7] clone: free or UNLEAK further pointers when finished

[Jeff King <peff@peff.net>]

On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:

> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	repo_name = argv[0];
>  
>  	path = get_repo_path(repo_name, &is_bundle);
> -	if (path)
> +	if (path) {
> +		free(path);
>  		repo = absolute_pathdup(repo_name);

You mentioned that "path" gets reused again later. Should we use
FREE_AND_NULL() to make sure that nobody tries to look at it in the
meantime?

> @@ -1393,6 +1394,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&reflog_msg);
>  	strbuf_release(&branch_top);
>  	strbuf_release(&key);
> +	free_refs(mapped_refs);
> +	free_refs((void *)remote_head_points_at);

We should avoid casting away constness when possible (because it is
often a sign that sometimes the variable _isn't_ pointing to owned
memory). In this case, I think freeing is the right thing; our
guess_remote_head() returns a copy of the struct (which is non-const).
Should remote_head_points_at just be declared without const?

> +	free_refs((void *)refs);

This one is more questionable to me. It comes from
transport_get_remote_refs(), which does return a const pointer. And it
looks like that memory is owned by the transport struct. So presumably
we need to tell the transport code to clean itself up (or mark it with
UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
be freeing transport->remote_refs in transport_disconnect()? You'd want
to make sure that no other callers expect the ref list to live on past
the disconnect).

> +	UNLEAK(repo);

This one could be done with the idiom I mentioned earlier:

  repo = absolute_repo = absolute_pathdup(repo_name);
  ...
  free(absolute_repo);

but I think this is a perfect use of UNLEAK().

-Peff