Re: [PATCH v3 08/35] connect: discover protocol version outside of get_remote_heads
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
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, &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. > > > 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
On Tue, 6 Feb 2018 17:12:45 -0800 Brandon Williams 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. > 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.