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