On 02/21, Jonathan Tan wrote:
> On Tue, 6 Feb 2018 17:12:45 -0800
> Brandon Williams <bmw...@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.
>
> > 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
