On Thu, Feb 02 2023, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
