On 02/21, Jonathan Tan wrote:

On Tue,  6 Feb 2018 17:12:45 -0800
Brandon Williams [off-list ref] wrote:

quoted

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

quoted

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