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.

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

[Brandon Williams]

In order to prepare for the addition of protocol_v2 push the protocol
version discovery outside of 'get_remote_heads()'.  This will allow for
keeping the logic for processing the reference advertisement for
protocol_v1 and protocol_v0 separate from the logic for protocol_v2.

Signed-off-by: Brandon Williams 
---
 builtin/fetch-pack.c | 16 +++-
 builtin/send-pack.c  | 17 +++--
 connect.c| 27 ++-
 connect.h|  3 +++
 remote-curl.c| 20 ++--
 remote.h |  5 +++--
 transport.c  | 24 +++-
 7 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 366b9d13f..85d4faf76 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -4,6 +4,7 @@
 #include "remote.h"
 #include "connect.h"
 #include "sha1-array.h"
+#include "protocol.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct fetch_pack_args args;
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
+   struct packet_reader reader;
 
packet_trace_identity("fetch-pack");
 
@@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
if (!conn)
return args.diag_url ? 0 : 1;
}
-   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");
+   }
 
ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
 , pack_lockfile_ptr);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fc4f0bb5f..83cb125a6 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -14,6 +14,7 @@
 #include "sha1-array.h"
 #include "gpg-interface.h"
 #include "gettext.h"
+#include "protocol.h"
 
 static const char * const send_pack_usage[] = {
N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
int progress = -1;
int from_stdin = 0;
struct push_cas_option cas = {0};
+   struct packet_reader reader;
 
struct option options[] = {
OPT__VERBOSITY(),
@@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.verbose ? CONNECT_VERBOSE : 0);
}
 
-   get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,
-_have, );
+   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(, _refs, REF_NORMAL,
+_have, );
+   break;
+   case protocol_unknown_version:
+   BUG("unknown protocol version");
+   }
 
transport_verify_remote_names(nr_refspecs, refspecs);
 
diff --git a/connect.c b/connect.c
index 00e90075c..db3c9d24c 100644
--- a/connect.c
+++ b/connect.c
@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
 }
 
-static enum protocol_version discover_version(struct packet_reader *reader)
+enum protocol_version discover_version(struct packet_reader *reader)
 {
enum protocol_version version = protocol_unknown_version;
 
@@ -234,7 +234,7 @@ enum get_remote_heads_state {
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **get_remote_heads(struct packet_reader *reader,
  struct ref **list, unsigned int flags,
  struct oid_array *extra_have,
  struct oid_array *shallow_points)
@@ -242,24 +242,17 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
-   struct packet_reader reader;
const char *arg;
 
-   packet_reader_init(, in, src_buf, src_len,
-  PACKET_READ_CHOMP_NEWLINE |
-  PACKET_READ_GENTLE_ON_EOF);
-
-   discover_version();
-
*list = NULL;
 
while