From: Jacob Keller Date: Thu, 03 Jul 2025 16:26:01 GMT Subject: Re: [PATCH] send-pack: clean up extra_have oid array Message-ID: <72331edd-8e03-4415-ada2-2be8fbee922b@intel.com> In-Reply-To: <20250703154047.GD1309870@coredump.intra.peff.net> On 7/3/2025 8:40 AM, Jeff King wrote: > On Tue, Jul 01, 2025 at 10:40:21AM -0700, Junio C Hamano wrote: > >> There is an early exit from the function that would bypass these >> clean-up. Perhaps something like this on top? >> >> builtin/send-pack.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git c/builtin/send-pack.c w/builtin/send-pack.c >> index b28da7ddd7..6ce9f6665a 100644 >> --- c/builtin/send-pack.c >> +++ w/builtin/send-pack.c >> @@ -305,9 +305,10 @@ int cmd_send_pack(int argc, >> flags |= MATCH_REFS_MIRROR; >> >> /* match them up */ >> - if (match_push_refs(local_refs, &remote_refs, &rs, flags)) >> - return -1; >> - >> + if (match_push_refs(local_refs, &remote_refs, &rs, flags)) { >> + ret = -1; >> + goto cleanup; >> + } >> if (!is_empty_cas(&cas)) >> apply_push_cas(&cas, remote, remote_refs); >> >> @@ -340,6 +341,7 @@ int cmd_send_pack(int argc, >> /* stable plumbing output; do not modify or localize */ >> fprintf(stderr, "Everything up-to-date\n"); >> >> +cleanup: >> string_list_clear(&push_options, 0); >> free_refs(remote_refs); >> free_refs(local_refs); > > This made me wonder if the remote_refs out-parameter is valid after > match_push_refs() returns failure (especially since we do not initialize > it at the top of the function). > > I think the answer is "yes"; it is both an in-parameter and an > out-parameter, and will have been earlier set up via get_remote_heads(). > So even on the failure case, match_push_refs() will leave it untouched > and it is still valid (and needs to be cleaned up). > This was my assumption too.