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

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

quoted

quoted

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").

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