Re: [WIP 06/15] transport: use get_refs_via_connect to get refs

From: Brandon Williams <bmwill@google.com>
Date: Dec 7, 2017, 18:40 UTC
Message-ID: <20171207184009.GA152943@google.com>
In-Reply-To: <xmqqmv2vfqxh.fsf@gitster.mtv.corp.google.com>

On 12/06, Junio C Hamano wrote:

Show 34 quoted lines

> Brandon Williams <bmwill@google.com> writes:
> 
> > Remove code duplication and use the existing 'get_refs_via_connect()'
> > function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> > 'git_transport_push()'.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  transport.c | 18 ++++--------------
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> >
> > diff --git a/transport.c b/transport.c
> > index d75ff0514..7c969f285 100644
> > --- a/transport.c
> > +++ 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 updated version is equivalent to the original as long as
> transport->data->extra_have is empty at this point.  Were we
> deliberately sending NULL, instead of &data->extra_have, in the
> original, or is it a mere oversight?
> 
> The same comment applies to the other hunk of this patch.

extra_have is only ever used by the push logic, so they're shouldn't be any harm is passing it through on the fetch side, especially since upload-pack doesn't send .have lines.

The push side is what uses the .have lines. From a quick look through the code it seems like get_refs_via_connect is always called before git_transport_push, so the extra check to make sure ref's have been retrieved doesn't get executed. But if it ever did get executed, we would silently ignore a server's .have lines.

-- 
Brandon Williams