Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 26, 2024, 13:50 UTC
- Message-ID
- <ZvVmitkT-QUneqvb@pks.im>
- In-Reply-To
- <20240924215225.GF1143820@coredump.intra.peff.net>
On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote:
Show 28 quoted lines
> 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?
Show 11 quoted lines
> 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