Re: [PATCH v2 08/27] connect: discover protocol version outside of get_remote_heads

[Brandon Williams]

On 01/31, Derrick Stolee wrote:
> On 1/25/2018 6:58 PM, Brandon Williams wrote:
> > 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");
> 
> Is this really a BUG in the client, or a bug/incompatibility in the server?
> 
> Perhaps I'm misunderstanding, but it looks like discover_version() will
> die() on an unknown version (the die() is in
> protocol.c:determine_protocol_version_client()). So maybe that's why this is
> a BUG()?
> 
> If there is something to change here, this BUG() appears three more times.

Yes, I have it labeled as a BUG because discover_version can't return an
unknown protocol version.  If the server actually returns an unknown
protocol version then it should be handled in
protocol.c:determine_protocol_version_client() as you mentioned.

> 
> > +   }
> > 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;

Re: [PATCH v2 08/27] connect: discover protocol version outside of get_remote_heads

[Derrick Stolee]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

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");


Is this really a BUG in the client, or a bug/incompatibility in the server?

Perhaps I'm misunderstanding, but it looks like discover_version() will 
die() on an unknown version (the die() is in 
protocol.c:determine_protocol_version_client()). So maybe that's why 
this is a BUG()?


If there is something to change here, this BUG() appears three more times.


+   }
  
  	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