Re: [PATCH v6 18/19] push: refactor refspec_append_mapped() for subsequent leak-fix

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 2, 2023, 22:10 UTC
Message-ID: <xmqqk00zsoqh.fsf@gitster.g>
In-Reply-To: <patch-v6-18.19-aa33f7e05c8-20230202T094704Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 13 quoted lines

> The set_refspecs() caller of refspec_append_mapped() (added in [1])
> left open the question[2] of whether the "remote" we lazily fetch
> might be NULL in the "[...]uniquely name our ref?" case, as
> remote_get() can return NULL.
>
> If we got past the "[...]uniquely name our ref?" case we'd have
> already segfaulted if we tried to dereference it as
> "remote->push.nr". In these cases the config mechanism & previous
> remote validation will have bailed out earlier.
>
> Let's refactor this code to clarify that, we'll now BUG() out if we
> can't get a "remote", and will no longer retrieve it for these common
> cases where we don't need it.

Another thing this does, if I am not mistaken, and the above does not mention, is, that the old code called get_local_heads() for each and every ref from the command line, and the second and subsequent calls to the function and assignment to local_refs would leak the entire local_refs linked list. This step does not release the linked list at the end, but at least it stops leaking it in each iteration of the loop.