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

From: Jeff King <peff@peff.net>
Date: Mar 19, 2015, 18:55 UTC
Message-ID: <20150319185517.GB8788@peff.net>
In-Reply-To: <xmqqfv90khpd.fsf@gitster.dls.corp.google.com>

On Thu, Mar 19, 2015 at 10:41:50AM -0700, Junio C Hamano wrote:

> Just to make sure we do not keep this hanging forever and eventually
> forget it, I'm planning to queue this.

Thanks for following up. A few minor nits (and maybe a more serious problem) on the explanation in the commit message:

> be complete., 2005-10-19) and ever since we instead stuffed a random
> bytes in ref->new_sha1 here.

s/a random/random/

Show 6 quoted lines

> It turns out that no codepath that comes after this assignment even
> looks at ref->new_sha1 at all.
> 
>  - 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.

s/list of ref/list of refs/ ? Or did you mean "the list we store in the 'ref' variable"?

I'm not sure what the final sentence means. Should it be "It does not look at the elements nor act on them"?

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.

>  - The only caller of do_fetch_pack(), fetch_pack(), returns this
>    list to its caller.  It does not look at the elements and acts on
>    them.

Ditto on the wording in the final sentence here (but it is correct in this case that we do not touch the list at all).

>  - 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?

If only it were so simple. It turns out that the list returned from fetch_pack() is _mostly_ a copy of the incoming refs list we give it. But in filter_refs(), if allow_tip_sha1_in_want is set, we actually add the unmatched "sought" refs here, too.

Which means we may be setting new_sha1 in one of these "sought" refs, and that broadens the scope of code that might be affected by the patch we have been discussing. However, I think the filter_refs code is wrong, and should be making a copy of the sought refs to put in the new list.

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.

-Peff