Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote:
> When we call get_remote_heads() for protocol v0, that may populate the
> "shallow" oid_array, which must be cleaned up to avoid a leak at the
> program exit. The same problem exists for both fetch-pack and send-pack,
> but not for the usual transport.c code paths, since we already do this
> cleanup in disconnect_git().
> 
> Fixing this lets us mark t5542 as leak-free for the send-pack side, but
> fetch-pack will need some more fixes before we can do the same for
> t5539.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/fetch-pack.c         | 1 +
>  builtin/send-pack.c          | 1 +
>  t/t5542-push-http-shallow.sh | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index cfc6951d23..ef4143eef3 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
>  	free_refs(fetched_refs);
>  	free_refs(remote_refs);
>  	list_objects_filter_release(&args.filter_options);
> +	oid_array_clear(&shallow);
>  	return ret;
>  }

I wonder about the early exit path we have when `finish_connect()`
returns non-zero. Should we make it go via the common cleanup path as
well, or do we not care in the error case?

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 81fc96d423..c49fe6c53c 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
>  	free_refs(remote_refs);
>  	free_refs(local_refs);
>  	refspec_clear(&rs);
> +	oid_array_clear(&shallow);
>  	return ret;
>  }

We also have an early exit in this function when `match_push_refs()`
returns non-zero.

Patrick