Re: [PATCH] transport: report refs only if transport does

From: Jeff King <peff@peff.net>
Date: Aug 2, 2018, 16:30 UTC
Message-ID: <20180802163007.GA15984@sigill.intra.peff.net>
In-Reply-To: <20180731232343.184463-1-jonathantanmy@google.com>

On Tue, Jul 31, 2018 at 04:23:43PM -0700, Jonathan Tan wrote:

Show 21 quoted lines

> > > Because transport_fetch_refs() filters the refs sent to the transport,
> > > it cannot just report the transport's result directly, but first needs
> > > to readd the excluded refs, pretending that they are fetched. However,
> > > this results in a wrong result if the transport did not report the refs
> > > that they have fetched in "fetched_refs" - the excluded refs would be
> > > added and reported, presenting an incomplete picture to the caller.
> > 
> > This part leaves me confused. If we are not fetching them, then why do
> > we need to pretend that they are fetched?
> 
> The short answer is that we need:
>  (1) the complete list of refs that was passed to
>      transport_fetch_refs(),
>  (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if
>      relevant), and
>  (3) with updated OIDs if ref-in-want was used.
> 
> The fetched_refs out param already fulfils (2) and (3), and this patch
> makes it fulfil (1). As for calling them fetched_refs, perhaps that is a
> misnomer, but they do appear in FETCH_HEAD even though they are not
> truly fetched.

Thanks for this explanation. It does make more sense to me now, and I agree that a lot of my confusion was from calling it "fetched_refs" (and the comment saying "reported as fetched, but not actually fetched").

Show 5 quoted lines

> Which raises the question...if completeness is so important, why not
> reuse the input list of refs and document that transport_fetch_refs()
> can mutate the input list? You ask the same question below, so I'll put
> the answer after quoting your paragraph.
> [...]

Thanks, the answer here was enlightening as well.

I see you posted a patch to go back to mutating the list, and that seems reasonable to me. I'm fine with a separate "out" list, too. Its purpose and expectations just need to be reflected in the name (and possibly in a comment).

-Peff