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

[Jonathan Tan]

On Thu, 22 Feb 2018 10:17:39 -0800
Brandon Williams  wrote:

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

Thanks - I indeed didn't notice that the implementation of
get_remote_heads() is modified too in this patch.

My initial comment was about just the renaming of "shallow" to
"shallow_points", but yes, you're right - I see in the implementation
that it is indeed named "shallow_points" there, so this change is
justified, especially since you're already changing the signature of
this function.

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

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:45 -0800
> Brandon Williams  wrote:
> 
> > -   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> > +
> > +   packet_reader_init(, fd[0], NULL, 0,
> > +  PACKET_READ_CHOMP_NEWLINE |
> > +  PACKET_READ_GENTLE_ON_EOF);
> > +
> > +   switch (discover_version()) {
> > +   case protocol_v1:
> > +   case protocol_v0:
> > +   get_remote_heads(, , 0, NULL, );
> > +   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.

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

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

[Jonathan Tan]

On Tue,  6 Feb 2018 17:12:45 -0800
Brandon Williams  wrote:

> - get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> +
> + packet_reader_init(, fd[0], NULL, 0,
> +PACKET_READ_CHOMP_NEWLINE |
> +PACKET_READ_GENTLE_ON_EOF);
> +
> + switch (discover_version()) {
> + case protocol_v1:
> + case protocol_v0:
> + get_remote_heads(, , 0, NULL, );
> + 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.

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