Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. 
> > */
> > +static int process_protocol_version(void)
> > +{
> > +   switch (determine_protocol_version_client(packet_buffer)) {
> > +   case protocol_v1:
> > +   return 1;
> > +   case protocol_v0:
> > +   return 0;
> > +   default:
> > +   die("server is speaking an unknown protocol");
> > +   }
> > +}
> 
> For the purpose of "technology demonstration" v1 protocol, it is OK
> to discard the result of "determine_pvc()" like the above code, but
> in a real application, we would do a bit more than just ignoring an
> extra "version #" packet that appears at the beginning, no?
> 
> It would be sensible to design how the result of determien_pvc()
> call is propagated to the remainder of the program in this patch and
> implement it.  Perhaps add a new global (like server_capabilities
> already is) and store the value there, or something?  Or pass a
> pointer to enum protocol_version as a return-location parameter to
> this helper function so that the process_capabilities() can pass a
> pointer to its local variable?

Yes, once we actually implement a v2 we would need to not throw away the
result of 'determine_pvc()' and instead do control flow based on the
resultant version.  I was trying to implement 'v1' as simply as possible
so that I wouldn't have to do a large amount of refactoring when
proposing this transition, though it seems Jonathan ended up doing more
than I planned, as I figured we didn't really know what the code will
need to be refactored to, in order to handle another protocol version.
I would suspect that we maybe wouldn't want to determine which version a
server is speaking in 'get_remote_heads()' but rather at some point
before that so we can branch off to do v2 like things, for example,
capability discovery and not ref discovery.

If you do think we need to do more of that refactoring now, before a v2,
I can most certainly work on that.


> 
> >  static void process_capabilities(int *len)
> >  {
> > @@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> > size_t src_len,
> >  */
> > int responded = 0;
> > int len;
> > -   int state = EXPECTING_FIRST_REF;
> > +   int state = EXPECTING_PROTOCOL_VERSION;
> >  
> > *list = NULL;
> >  
> > while ((len = read_remote_ref(in, _buf, _len, ))) {
> > switch (state) {
> > +   case EXPECTING_PROTOCOL_VERSION:
> > +   if (process_protocol_version()) {
> > +   state = EXPECTING_FIRST_REF;
> > +   break;
> > +   }
> > +   state = EXPECTING_FIRST_REF;
> > +   /* fallthrough */
> > case EXPECTING_FIRST_REF:
> > process_capabilities();
> > if (process_dummy_ref()) {

-- 
Brandon Williams

Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. 
> > */
> > +static int process_protocol_version(void)
> > +{
> > +   switch (determine_protocol_version_client(packet_buffer)) {
> > +   case protocol_v1:
> > +   return 1;
> > +   case protocol_v0:
> > +   return 0;
> > +   default:
> > +   die("server is speaking an unknown protocol");
> > +   }
> > +}
> 
> checkpatch.pl yells at me:
> 
> ERROR: switch and case should be at the same indent
> 
> and we would probably want to teach "make style" the same, if we
> already don't.

'make style' actually already understands this, I just forgot it run it
on this change :)

-- 
Brandon Williams

Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Junio C Hamano]

Brandon Williams  writes:

> +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
> +static int process_protocol_version(void)
> +{
> + switch (determine_protocol_version_client(packet_buffer)) {
> + case protocol_v1:
> + return 1;
> + case protocol_v0:
> + return 0;
> + default:
> + die("server is speaking an unknown protocol");
> + }
> +}

For the purpose of "technology demonstration" v1 protocol, it is OK
to discard the result of "determine_pvc()" like the above code, but
in a real application, we would do a bit more than just ignoring an
extra "version #" packet that appears at the beginning, no?

It would be sensible to design how the result of determien_pvc()
call is propagated to the remainder of the program in this patch and
implement it.  Perhaps add a new global (like server_capabilities
already is) and store the value there, or something?  Or pass a
pointer to enum protocol_version as a return-location parameter to
this helper function so that the process_capabilities() can pass a
pointer to its local variable?

>  static void process_capabilities(int *len)
>  {
> @@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> size_t src_len,
>*/
>   int responded = 0;
>   int len;
> - int state = EXPECTING_FIRST_REF;
> + int state = EXPECTING_PROTOCOL_VERSION;
>  
>   *list = NULL;
>  
>   while ((len = read_remote_ref(in, _buf, _len, ))) {
>   switch (state) {
> + case EXPECTING_PROTOCOL_VERSION:
> + if (process_protocol_version()) {
> + state = EXPECTING_FIRST_REF;
> + break;
> + }
> + state = EXPECTING_FIRST_REF;
> + /* fallthrough */
>   case EXPECTING_FIRST_REF:
>   process_capabilities();
>   if (process_dummy_ref()) {

Re: [PATCH v2 6/9] connect: teach client to recognize v1 server response

[Junio C Hamano]

Brandon Williams  writes:

> +/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
> +static int process_protocol_version(void)
> +{
> + switch (determine_protocol_version_client(packet_buffer)) {
> + case protocol_v1:
> + return 1;
> + case protocol_v0:
> + return 0;
> + default:
> + die("server is speaking an unknown protocol");
> + }
> +}

checkpatch.pl yells at me:

ERROR: switch and case should be at the same indent

and we would probably want to teach "make style" the same, if we
already don't.

[PATCH v2 6/9] connect: teach client to recognize v1 server response

[Brandon Williams]

Teach a client to recognize that a server understands protocol v1 by
looking at the first pkt-line the server sends in response.  This is
done by looking for the response "version 1" send by upload-pack or
receive-pack.

Signed-off-by: Brandon Williams 
---
 connect.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/connect.c b/connect.c
index 8e2e276b6..1805debf3 100644
--- a/connect.c
+++ b/connect.c
@@ -12,6 +12,7 @@
 #include "sha1-array.h"
 #include "transport.h"
 #include "strbuf.h"
+#include "protocol.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
@@ -129,9 +130,23 @@ static int read_remote_ref(int in, char **src_buf, size_t 
*src_len,
return len;
 }
 
-#define EXPECTING_FIRST_REF 0
-#define EXPECTING_REF 1
-#define EXPECTING_SHALLOW 2
+#define EXPECTING_PROTOCOL_VERSION 0
+#define EXPECTING_FIRST_REF 1
+#define EXPECTING_REF 2
+#define EXPECTING_SHALLOW 3
+
+/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
+static int process_protocol_version(void)
+{
+   switch (determine_protocol_version_client(packet_buffer)) {
+   case protocol_v1:
+   return 1;
+   case protocol_v0:
+   return 0;
+   default:
+   die("server is speaking an unknown protocol");
+   }
+}
 
 static void process_capabilities(int *len)
 {
@@ -224,12 +239,19 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
 */
int responded = 0;
int len;
-   int state = EXPECTING_FIRST_REF;
+   int state = EXPECTING_PROTOCOL_VERSION;
 
*list = NULL;
 
while ((len = read_remote_ref(in, _buf, _len, ))) {
switch (state) {
+   case EXPECTING_PROTOCOL_VERSION:
+   if (process_protocol_version()) {
+   state = EXPECTING_FIRST_REF;
+   break;
+   }
+   state = EXPECTING_FIRST_REF;
+   /* fallthrough */
case EXPECTING_FIRST_REF:
process_capabilities();
if (process_dummy_ref()) {
-- 
2.14.1.992.g2c7b836f3a-goog