Re: [PATCH v3 08/35] connect: discover protocol version outside of get_remote_heads

From: Brandon Williams <bmwill@google.com>
Date: Feb 22, 2018, 18:17 UTC
Message-ID: <20180222181739.GC185096@google.com>
In-Reply-To: <20180221141135.c78d72b422f985aae5375bca@google.com>

On 02/21, Jonathan Tan wrote:

Show 27 quoted lines

> On Tue,  6 Feb 2018 17:12:45 -0800
> Brandon Williams <bmwill@google.com> wrote:
> 
> > -	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> > +
> > +	packet_reader_init(&reader, fd[0], NULL, 0,
> > +			   PACKET_READ_CHOMP_NEWLINE |
> > +			   PACKET_READ_GENTLE_ON_EOF);
> > +
> > +	switch (discover_version(&reader)) {
> > +	case protocol_v1:
> > +	case protocol_v0:
> > +		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
> > +		break;
> > +	case protocol_unknown_version:
> > +		BUG("unknown protocol version");
> > +	}
> 
> This inlining is repeated a few times, which raises the question: if the
> intention is to keep the v0/1 logic separately from v2, why not have a
> single function that wraps them all? Looking at the end result (after
> all the patches in this patch set are applied), it seems that the v2
> version does not have extra_have or shallow parameters, which is a good
> enough reason for me (I don't think functions that take in many
> arguments and then selectively use them is a good idea). I think that
> other reviewers will have this question too, so maybe discuss this in
> the commit message.

Yes this sort of switch statement appears a few times but really there isn't a good way to "have one function to wrap it all" with the current state of the code. That sort of change would take tons of refactoring to get into a state where we could do that, and is outside the scope of this series.

Show 19 quoted lines

> 
> > diff --git a/remote.h b/remote.h
> > index 1f6611be2..2016461df 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);
> >  void free_refs(struct ref *ref);
> >  
> >  struct oid_array;
> > -extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> > +struct packet_reader;
> > +extern struct ref **get_remote_heads(struct packet_reader *reader,
> >  				     struct ref **list, unsigned int flags,
> >  				     struct oid_array *extra_have,
> > -				     struct oid_array *shallow);
> > +				     struct oid_array *shallow_points);
> 
> This change probably does not belong in this patch, especially since
> remote.c is unchanged.

Yes this hunk is needed, the signature of get_remote_heads changes. It may be difficult to see that due to the fact that we don't really have a clear story on how header files are divided up within the project.

-- 
Brandon Williams