On 02/27, Jonathan Nieder wrote:
> Brandon Williams wrote:
> > On 02/26, Jonathan Nieder wrote:
> >> Brandon Williams wrote:
>
> >>> +++ b/transport.c
> >>> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport
> >>> *transport,
> >>> args.cloning = transport->cloning;
> >>> args.update_shallow = data->options.update_shallow;
> >>>
> >>> - if (!data->got_remote_heads) {
> >>> - connect_setup(transport, 0);
> >>> - get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
> >>> - NULL, &data->shallow);
> >>> - data->got_remote_heads = 1;
> >>> - }
> >>> + if (!data->got_remote_heads)
> >>> + refs_tmp = get_refs_via_connect(transport, 0);
> >>
> >> The only difference between the old and new code is that the old code
> >> passes NULL as 'extra_have' and the new code passes &data->extra_have.
> >>
> >> That means this populates the data->extra_have oid_array. Does it
> >> matter?
> [...]
> > I don't think its a problem to have extra_have populated, least I
> > haven't seen anything to lead me to believe it would be a problem.
>
> Assuming it gets properly freed later, the only effect I can imagine
> is some increased memory usage.
>
> I'm inclined to agree with you that the simplicity is worth it. It
> seems worth mentioning in the commit message, though.
>
> [...]
> >>> @@ -541,14 +537,8 @@ static int git_transport_push(struct transport
> >>> *transport, struct ref *remote_re
> >>> struct send_pack_args args;
> >>> int ret;
> >>>
> >>> - if (!data->got_remote_heads) {
> >>> - struct ref *tmp_refs;
> >>> - connect_setup(transport, 1);
> >>> -
> >>> - get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
> >>> - NULL, &data->shallow);
> >>> - data->got_remote_heads = 1;
> >>> - }
> >>> + if (!data->got_remote_heads)
> >>> + get_refs_via_connect(transport, 1);
> >>
> >> not a new problem, just curious: Does this leak tmp_refs?
> >
> > Maybe, though its removed by this patch.
>
> Sorry for the lack of clarity. If it was leaked before, then it is
> still leaked now, via the discarded return value from
> get_refs_via_connect.
>
> Any idea how we can track that down? E.g. are there ways to tell leak
> checkers "just tell me about this particular allocation"?
Hmm I wonder if that code path is even used, because it just throws away
the result.
--
Brandon Williams
