Re: Bug in fetch-pack.c, please confirm

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2015, 19:01 UTC
Message-ID: <xmqqtwxgizg9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150319185517.GB8788@peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

>>  - The only caller of everything_local(), do_fetch_pack(), returns
>>    this list of ref, whose element has bogus new_sha1 values, to its
>>    caller.  It does not look at the elements and acts on them.
>
> I'm not sure what the final sentence means. Should it be "It does not
> look at the elements nor act on them"?

Yes. "It does not look at the new_sha1. It does not use the information to change its behaviour. Both of the previous two statements are true." is what I meant.

> do_fetch_pack actually does pass them down to find_common. But the
> latter does not look at ref->new_sha1, so we're OK.

Show 7 quoted lines

>>  - The other caller of fetch_pack() is fetch_refs_via_pack() in the
>>    transport layer, which is a helper that implements "git fetch".
>>    It only cares about whether the returned list is empty (i.e.
>>    failed to fetch anything).
>
> So I thought I would follow this up by adding a free_refs() call in
> fetch_refs_via_pack. After all, we just leak that list, right?

Yeah, I agree.

> I'm working up a few patches in that area, which I'll send out in a few
> minutes. Once that is done, then I think the explanation you give above
> would be correct.

If a follow-up is coming then I'd just drop this one.  Thanks.