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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Mon, 06 Feb 2023 18:16:21 +0100

On Thu, Feb 02 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>
>> 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.

I think you're mistaken, or I'm misunderstanding you. For e.g. the 29th
test in t4301-merge-tree-write-tree.sh where we do:

	git push read-only side1 side2 side3

We'll only invoke "get_local_heads()" once, not once for each of the
refs. That's part of the reason for this refactoring: It *looks* as
though we might do it N times, but in practice we always get hte
"remote", so we only leak it once.